feat: notify on successful autoupdate #13903
Conversation
There was a problem hiding this comment.
In the other PR, we did this a bit differently. We added this fake helper into the testutils package. Link to PR
There was a problem hiding this comment.
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.
I prefer names which are explicit and clear, so I prefer testutils TBH.
There was a problem hiding this comment.
I will wait with merging this PR until Bruno merges #13868, then adjust it.
There was a problem hiding this comment.
Overall, the code looks good to me, but I would wait for @dannykopping review since he is leading the work on this feature.
coderd/database/migrations/000227_notifications_workspace_autoupdated.up.sql
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,37 @@ | |||
| package notiffake | |||
There was a problem hiding this comment.
Nit: I like splitting this out but I think the name is too "cute".
I think something like testutil or something would be more idiomatic.
There was a problem hiding this comment.
Alright, I decided to merge it as is, and refactor/unify once #13868 is merged.
Related: #13891
This PR implements notifications on workspace autoupdate.
Comment:
I took inspiration from Bruno's PR, and exposed the
FakeNotificationsEnqueueras a separate package.