8000 feat: implement thin vertical slice of system-generated notifications by dannykopping · Pull Request #13537 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

feat: implement thin vertical slice of system-generated notifications #13537

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
Jul 8, 2024
Merged

feat: implement thin vertical slice of system-generated notifications #13537

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
53c9cbb
feat: system-generated notifications
dannykopping Jun 11, 2024
4856aed
Fixing lint errors & minor tests
dannykopping Jun 11, 2024
cda6efb
Fixing dbauthz test
dannykopping Jun 11, 2024
86f937a
TestBufferedUpdates does not need a real db, altering test details sl…
dannykopping Jun 11, 2024
e8f1af2
Correct TestBufferedUpdates to count updated entries, use real db again
dannykopping Jun 12, 2024
a056f54
Use UUID for notifier IDs
dannykopping Jun 27, 2024
8c64d30
Small improvements from review suggestions
dannykopping Jun 27, 2024
ac149ec
Protect notifiers from modification during Stop()
dannykopping Jun 27, 2024
884fadf
Split out enqueuer as separate responsibility, get rid of singleton
dannykopping Jun 28, 2024
4e362e7
Remove unnecessary handler registry
dannykopping Jun 28, 2024
8097290
Remove unused context
dannykopping Jun 28, 2024
1b841ad
Centralise markdown rendering
dannykopping Jun 28, 2024
61f5bd6
Appease the linter
dannykopping Jun 28, 2024
3c8e33b
Only enqueue notification when not initiated by self
dannykopping Jul 1, 2024
757327c
Hide config flags which are unlikely to be modified by operators
dannykopping Jul 1, 2024
6f909ae
Remove unnecessary Labels struct
dannykopping Jul 1, 2024
8000
36698c5
Enable experiment as safe
dannykopping Jul 1, 2024
c5701a6
Correcting bad refactor
dannykopping Jul 1, 2024
9d4c312
Initialize Enqueuer on API startup
dannykopping Jul 1, 2024
9380d8e
Only start one notifier since all dispatches are concurrent anyway
dannykopping Jul 1, 2024
4b7214d
Fix docs
dannykopping Jul 1, 2024
6679ef1
Fix lint error
dannykopping Jul 1, 2024
337997d
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 2, 2024
ba5f7c6
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 3, 2024
0f29293
Review feedback
dannykopping Jul 3, 2024
7c6c486
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 4, 2024
c6e75c2
Fix lint failures
dannykopping Jul 4, 2024
aff9e6c
Review comments
dannykopping Jul 4, 2024
613e074
Avoid race by exposing number of pending updates
dannykopping Jul 4, 2024
faea7fc
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8000
Prev Previous commit
Next Next commit
TestBufferedUpdates does not need a real db, altering test details sl… 
…ightly

Signed-off-by: Danny Kopping <danny@coder.com>
  • Loading branch information
@dannykopping
dannykopping committed Jun 28, 2024
commit 86f937a7550aae8f8f63a90e72c12230d661ffa9
14 changes: 10 additions & 4 deletions coderd/notifications/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"
"time"

"github.com/coder/coder/v2/coderd/database/pubsub"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"golang.org/x/xerrors"
Expand Down Expand Up @@ -49,7 +50,9 @@ func TestBufferedUpdates(t *testing.T) {
t.Parallel()

// setup
ctx, logger, db, ps := setup(t)
ctx := context.Background()
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug)
db := dbmem.New()
interceptor := &bulkUpdateInterceptor{Store: db}

santa := &santaHandler{}
Expand All @@ -59,13 +62,16 @@ func TestBufferedUpdates(t *testing.T) {
require.NoError(t, err)
mgr.WithHandlers(handlers)

client := coderdtest.New(t, &coderdtest.Options{Database: db, Pubsub: ps})
client := coderdtest.New(t, &coderdtest.Options{Database: db, Pubsub: pubsub.NewInMemory()})
user := coderdtest.CreateFirstUser(t, client)

// given
if _, err := mgr.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, types.Labels{"nice": "true"}, ""); true {
require.NoError(t, err)
}
if _, err := mgr.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, types.Labels{"nice": "true"}, ""); true {
require.NoError(t, err)
}
if _, err := mgr.Enqueue(ctx, user.UserID, notifications.TemplateWorkspaceDeleted, types.Labels{"nice": "false"}, ""); true {
require.NoError(t, err)
}
Expand All @@ -76,13 +82,13 @@ func TestBufferedUpdates(t *testing.T) {
// then

// Wait for messages to be dispatched.
require.Eventually(t, func() bool { return santa.naughty.Load() == 1 && santa.nice.Load() == 1 }, testutil.WaitMedium, testutil.IntervalFast)
require.Eventually(t, func() bool { return santa.naughty.Load() == 1 && santa.nice.Load() == 2 }, testutil.WaitMedium, testutil.IntervalFast)

// Stop the manager which forces an update of buffered updates.
require.NoError(t, mgr.Stop(ctx))
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a race condition here, which is that above we wait for notifications to be sent to santa, but sending to santa happens before sending on the success and failure channels. So, triggering stop and the final bulk update might miss one or more success/failure updates and cause this test to flake.

Quartz can help here. The manager's bulk update and the notifier's processing loop depend on Tickers. If you convert them to use a quartz.TickerFunc, then you can control the mock clock and trigger the processing loop and bulk update, and wait for them to finish.

This lets us get rid of the Eventually that checks for dispatched notifications (Quartz allows you to wait until the tick processing is done, so there is no race in just asserting santa got what we expected).

It also means we can get rid of the Eventually below for the same reason --- we can wait for bulk processing to complete before asserting the interceptor's state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a follow-up item to implement Quartz in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 613e074


// Wait until both success & failure updates have been sent to the store.
require.Eventually(t, func() bool { return interceptor.failed.Load() == 1 && interceptor.sent.Load() == 1 }, testutil.WaitMedium, testutil.IntervalFast)
require.Eventually(t, func() bool { return interceptor.failed.Load() == 1 && interceptor.sent.Load() == 2 }, testutil.WaitMedium, testutil.IntervalFast)
}

func TestBuildPayload(t *testing.T) {
Expand Down
0