feat: notify on workspace update #15979
Conversation
.../notifications/testdata/rendered-templates/smtp/TemplateWorkspaceManuallyUpdated.html.golden
Outdated
Show resolved
Hide resolved
coderd/workspacebuilds.go
Outdated
| @@ -327,6 +328,16 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| previousWorkspaceBuild, err := api.Database.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) | |||
There was a problem hiding this comment.
This is racy since we're not in a transaction. There could be an intermediate build between this one and the actual build (unlikely).
This and builder.Build could be moved closer to each other and wrapped inside a tx to eliminate the edge case, though (the same is done in createWorkspace so should be fine). Or since the builder already fetches the previous build, perhaps the deduction of "template changed" can be moved in there and exposed by some means.
(Not looking to complicate this so feel free to ignore, but yet another alternative is to move the notification logic to wsbuilder, although that may need to rely on initiator vs the signal of using this endpoint. The initiator could be you, or it could be an admin, etc.)
There was a problem hiding this comment.
I think the likelihood of a race is very minimal, but even if it should happen the notifier would handle de-duplication anyways.
I also think moving the notification logic into wsbuilder is at the wrong abstraction level.
There was a problem hiding this comment.
I'm fine with not moving the notification logic, but why would we not remove the race, even if there's only a minimal chance? From my POV there's two factors I want to protect against:
- Potentially missing a notification because of the race (yes, minimal and unlikely, but still).
- Now that we're introduced
previousWorkspaceBuilddata into this endpoint, future changes may rely upon it in ways we haven't thought of here, this consideration may be important there as well and less likely to be spotted.
coderd/workspacebuilds.go
Outdated
| @@ -428,6 +445,79 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | |||
| httpapi.Write(ctx, rw, http.StatusCreated, apiBuild) | |||
| } | |||
|
|
|||
| func (api *API) notifyWorkspaceUpdated(ctx context.Context, workspaceID uuid.UUID) { | |||
There was a problem hiding this comment.
It seems a bit unfortunate that this method needs to redo a bunch of queries just to get the message across, AFAICT we already have most of the data available in the endpoint calling out to this method.
Not going to enforce a change here, but this could be made more specific for the use-case (for the time-being).
(Additionally, if moved inside wsbuilder I believe we would have pretty much all of the data available.)
There was a problem hiding this comment.
Yeah I totally agree, I've tried to reduce the amount of queries it makes.
coderd/database/migrations/000280_workspace_update_notification.up.sql
Outdated
Show resolved
Hide resolved
coderd/database/migrations/000280_workspace_update_notification.up.sql
Outdated
Show resolved
Hide resolved
coderd/workspacebuilds.go
Outdated
| @@ -327,6 +328,16 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| previousWorkspaceBuild, err := api.Database.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) | |||
There was a problem hiding this comment.
I'm not convinced if this is the right place to send the notification. We should send it after the successful update, so I would target the provisioner server?
There was a problem hiding this comment.
Good point. I think we can tweak this via wording.
For instance, this would be pretty flexible and let us keep the notification here:
A new workspace build to update your workspace to template version 'boo' has been manually created/initiated.
Personally I think it's more valuable here as you know immediately this has been triggered and since the update happens here whether or not the build is successful.
There was a problem hiding this comment.
Personally I think it's more valuable here as you know immediately this has been triggered and since the update happens here whether or not the build is successful.
Fair 👍 A user could cancel the request.
Relates to #15845
When the
/workspace/<name>/buildsendpoint is hit, we check if the requested template version is different to the previously used template version. If these values differ, we can assume that the workspace has been manually updated and send the appropriate notification. Automatic updates happen in the lifecycle executor and bypasses this endpoint entirely.