- Notifications
You must be signed in to change notification settings - Fork1k
feat: notify on workspace creation#15934
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
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, some suggestions below but nothing blocking:
- Regarding your
coderdtests, it's good practice to "drop privileges" by creating a separate user with the minimum permission set for the behaviour you want to test, and testing with that user. This will catch RBAC-related bugs. - As it's also possible to create workspaces on behalf of other users, we should also test the behaviour in this workflow.
Uh oh!
There was an error while loading.Please reload this page.
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.
Obligatory prod to double-check migration number before merging :-)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| funcWithTemplateID(id uuid.UUID)func(*FakeNotification)bool { | ||
| returnfunc(n*FakeNotification)bool { | ||
| returnn.TemplateID==id | ||
| } |
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.
Nice :-)
f0e81ab intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#15845
This PR introduces notifications for when workspaces are created.
Each notification contains some metadata about the workspace that has been created. I've intentionally limited this information to just IDs and names, as we can always add more data if requested/desired.
Adding the new notification caused other unrelated tests to fail due to how they were checking for the right notification. I've updated these tests to use a new helper function
SentWithTemplateIDto hopefully reduce the likelihood of further notifications to cause a cascade of test failures.