Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: create database tables and queries for notifications#13536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
dannykopping merged 5 commits intomainfromdk/system-notifications-database
Jun 28, 2024

Conversation

dannykopping
Copy link
Contributor

@dannykoppingdannykopping commentedJun 11, 2024•
edited
Loading

Creates two new tables:

  • notification_templates: templates are used to create notifications from, and specify a unique event in the system
    • right now this will just store immutable system notification templates, but in the future users will be able to configure their own notification templates (e.g. notify when a workspace's disk is 90% full)
  • notification_messages: each record corresponds to a separate notification which must be delivered to a user
    • this table is effectively a queue, and queue consumers can consume these records concurrently with safety (seeAcquireNotificationMessages)

coderd/database/migrations/000219_notifications.up.sql inserts a single template ("Workspace Deleted"), for which we are creating this vertical slice in this initial stack of PRs.

@dannykoppingGraphite App
Copy link
ContributorAuthor

dannykopping commentedJun 11, 2024•
edited
Loading

@dannykoppingdannykoppingforce-pushed thedk/system-notifications-database branch from1cb776a to25636ecCompareJune 11, 2024 13:15
@dannykoppingdannykopping changed the titlefeat: database tables & queriesfeat: create database tables and queries for notificationsJun 11, 2024
@dannykoppingdannykopping marked this pull request as ready for reviewJune 11, 2024 14:13
Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some comments below, but nothing blocking.

@dannykoppingdannykoppingforce-pushed thedk/system-notifications-database branch from1c4046c to538bc42CompareJune 12, 2024 09:08
@dannykoppingdannykoppingforce-pushed thedk/system-notifications-database branch from538bc42 to438bcbaCompareJune 12, 2024 10:51
@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJun 20, 2024
@johnstcnjohnstcn reopened thisJun 23, 2024
@johnstcnjohnstcn removed the staleThis issue is like stale bread. labelJun 23, 2024
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
This has the unfortunate side-effect of not allowing the AcquireNotificationMessagesRow.Payload type to be overriden to []byte as it was previously, but it does not change the semanticsSigned-off-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykoppingforce-pushed thedk/system-notifications-database branch from438bcba toc1a3010CompareJune 27, 2024 09:46
Copy link
ContributorAuthor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@spikecurtis I'm back from PTO. I believe I've addressed all of your comments.
Can you please have another look?

Copy link
Contributor

@spikecurtisspikecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM

One comment inline, but I don't need to review again.

WHEN (attempt_count + 1 < @max_attempts::int)
THEN NOW() + CONCAT(@retry_interval::int, ' seconds')::interval END
FROM (SELECT id, status, status_reason, failed_at
FROM new_values) AS subquery
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This query is also doing theWITH new_values thing where we then essentially renamenew_values tosubquery. Can be simplified to remove theWITH clause and unnest the values here in theFROM clause.

dannykopping reacted with thumbs up emoji
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykoppingenabled auto-merge (squash)June 28, 2024 09:14
@dannykoppingdannykopping merged commit0a221e8 intomainJun 28, 2024
28 checks passed
@dannykoppingdannykopping deleted the dk/system-notifications-database branchJune 28, 2024 09:21
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 28, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@spikecurtisspikecurtisspikecurtis approved these changes

@mtojekmtojekmtojek approved these changes

@kylecarbskylecarbsAwaiting requested review from kylecarbs

Assignees

@dannykoppingdannykopping

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@dannykopping@johnstcn@spikecurtis@mtojek
[8]ページ先頭
©2009-2025 Movatter.jp