- Notifications
You must be signed in to change notification settings - Fork928
feat: notify on successful autoupdate#13903
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the other PR, we did this a bit differently. We added this fake helper into thetestutils package.Link to PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I'm aware of it. This is an alternative approach, and I'm happy to leave it or switch to the other form.@dannykopping any preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I prefer names which are explicit and clear, so I prefertestutils TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will wait with merging this PR until Bruno merges#13868, then adjust it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall, the code looks good to me, but I would wait for@dannykopping review since he is leading the work on this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/database/migrations/000227_notifications_workspace_autoupdated.up.sql OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1,37 @@ | |||
| package notiffake | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: I like splitting this out but I think the name is too "cute".
I think something liketestutil or something would be more idiomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alright, I decided to merge it as is, and refactor/unify once#13868 is merged.
fbd1d7f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Related:#13891
This PR implements notifications on workspace autoupdate.
Comment:
I took inspiration fromBruno's PR, and exposed the
FakeNotificationsEnqueueras a separate package.