feat(coderd): implement task to app linking #20237
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8b065ed to
293764d
Compare
5adbc9d to
30464da
Compare
30464da to
d3fd9eb
Compare
293764d to
83a00fb
Compare
acd5dcf to
336477c
Compare
83a00fb to
281cf6e
Compare
| // the agent to a task. As per previous behavior, this is OK | ||
| // because we always overwrite the given protoAgent.Id. | ||
| agentID := uuid.New() | ||
| protoAgent.Id = makeStaticID(agentID) |
There was a problem hiding this comment.
Review: This was unfortunately the most compatible way I could think of since InsertWorkspaceResource is called from all over the place, and tests across the code-base. So I wanted to avoid churn by changing the function call. We also needed a way to opt-out of using the provided ID here since many tests start erroring out on duplicate IDs.
Functional options would've been one choice, but for this tiny change it seemed overkill.
Frankly it's a bit weird we have this field but discard it nonetheless.
There was a problem hiding this comment.
My worry here is that a future change will use makeStaticID without covering all the possible code paths with staticID(). It should be fairly apparent in tests since a failure to remove the prefix will cause an invalid UUID error. The only other solution I could imagine would involve changing types which would involve more churn. Can you create a follow-up PR for refactoring this?
There was a problem hiding this comment.
Can you create a follow-up PR for refactoring this?
What's your proposal for how it would be refactored?
I can also name it makeStaticAgentID if that helps make it clear that it (currently) only has one intended use-case.
There was a problem hiding this comment.
Replaced with functional options, see a7e1d10
| } | ||
| } | ||
|
|
||
| if task, err := db.GetTaskByWorkspaceID(ctx, workspace.ID); err == nil { |
There was a problem hiding this comment.
Review: Previous sidebar app behavior is retained for the time being, we're just improving the task data model in parallel.
DanielleMaywood
left a comment
There was a problem hiding this comment.
I've got no objections (other than what Cian has already raised with makeStaticID), although I'm not familiar enough with either components to feel confident to give the green light
281cf6e to
fff8343
Compare
336477c to
f7e0c43
Compare
a7e1d10 to
e8773e7
Compare
360fc06 to
c790418
Compare
e8773e7 to
77ba7ea
Compare
ed3b6f4 to
df42f86
Compare
This change adds workspace build/agent/app linking to tasks and wires it into `wsbuilder` and `provisionerdserver`. Closes coder/internal#948 Closes #20212 Closes #19773
77ba7ea to
64c8d86
Compare
df42f86 to
5dc57da
Compare
This change adds workspace build/agent/app linking to tasks and wires it
into
wsbuilderandprovisionerdserver.Closes coder/internal#948
Closes #20212
Closes #19773