TBD: Which is the desired order here? 1 -> 2, 2-> 1, or 1 XOR 2?

Wouldn't it be a potential security issue if a user could override the template tags? The user could then swap to a provisioner they shouldn't be able to access. Just a thought.

As long as a user can't modifyowner orscope, I think being able to override tags is a feature. It's how I would get a workspace to be provisioned using a provisioner in my specific region if the template defaults to a US region provisioner for example. And MutateTags protects the owner and scope tags afaik.

Although the template may well have a tag sourced from a variable for this.

Wouldn't it be a potential security issue if a user could override the template tags? The user could then swap to a provisioner they shouldn't be able to access. Just a thought.

Should this be an error? Silently replacing seems like a recipe for confusion.

I was debating that, but you've sold me. 👍

I might make this a follow-up, as the existing behaviour ofprovisionersdk.MutateTags is to overwriteprovisionersdk.TagOwner

https://github.com/coder/coder/pull/15518/files#diff-01b662bbd7ba1a97489f36a381633c882cc2126dab8fb9ede81d3cb9239dd035R36-R40

review: I considered adding as a field on theProvisionerJob but it did not seem 100% correct.

However, as it does not make sense to return this from e.g. getting a previous template version, it may make sense instead to make this nullable (e.g.*MatchedProvisioners). Thoughts?

From what I could see last week, the only way to see tags for a workspace build is to get that workspace's template version. Therefore, I think we need this even for previous template versions.

Looked at where else we use this payload. You're right, it isn't necessary elsewhere. That means we could make it nullable. Whether that would be better for API clients, I'm not sure. Would be happy if you made it nullable. Would be happy if you left it as is.

I'll keep it as-is for now so you can build on top of it. It should be a fairly simple refactor to make it a nullable field later.

review: this is duplicated from / in line withcoderd/healthcheck:

if opts.StaleInterval == 0 {opts.StaleInterval = provisionerdserver.DefaultHeartbeatInterval * 3}

Do we need the above as a code comment or perhaps a test that checks that they remain in sync?

I'm not sure of the value of adding a test for this, but we can possibly refactor where it's defined in a follow-up so that it's consistent. For now, I'll add a comment 👍

On the surface it kinda makes sense to use the same logic (3*poll), but at the same time I feel multiplying by 3 is pretty extreme, I'm more in favor of a fixed time like poll+30s or something like that. In this instance poll+30s would surface the issue in half the time. If poll is ever raised, to say 2min, the multiplication would go from 1.5min to 3.5min longer to surface the issue, etc.

On the surface it kinda makes sense to use the same logic (3*poll), but at the same time I feel multiplying by 3 is pretty extreme, I'm more in favor of a fixed time like poll+30s or something like that. In this instance poll+30s would surface the issue in half the time. If poll is ever raised, to say 2min, the multiplication would go from 1.5min to 3.5min longer to surface the issue, etc.

❤️