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 andbuilder.Build could be moved closer to each other and wrapped inside a tx to eliminate the edge case, though (the same is done increateWorkspace 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.)

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 intowsbuilder is at the wrong abstraction level.

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:

1. Potentially missing a notification because of the race (yes, minimal and unlikely, but still).
2. Now that we're introducedpreviousWorkspaceBuild data 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.

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 insidewsbuilder I believe we would have pretty much all of the data available.)

Yeah I totally agree, I've tried to reduce the amount of queries it makes.

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?

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.

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.