- Notifications
You must be signed in to change notification settings - Fork1k
feat(coderd): implement task to app linking#20237
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
base:mafredri/feat-coderd-tasks-data-model-split4_split_split
Are you sure you want to change the base?
Conversation
This change adds workspace build/agent/app linking to tasks and wires itinto `wsbuilder` and `provisionerdserver`.Closescoder/internal#948Closes#20212Closes#19773
mafredri commentedOct 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
8b065ed to293764dCompare5adbc9d to30464daCompare30464da tod3fd9ebCompare293764d to83a00fbCompareacd5dcf to336477cCompare83a00fb to281cf6eCompare| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Review: This was unfortunately the most compatible way I could think of sinceInsertWorkspaceResource 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My worry here is that a future change will usemakeStaticID without covering all the possible code paths withstaticID(). 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you create a follow-up PR for refactoring this?
What's your proposal for how it would be refactored?
I can also name itmakeStaticAgentID if that helps make it clear that it (currently) only has one intended use-case.
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.
Replaced with functional options, see a7e1d10
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
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
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.
I like this much better! Thank you@mafredri !
| } | ||
| } | ||
| iftask,err:=db.GetTaskByWorkspaceID(ctx,workspace.ID);err==nil { |
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.
Review: Previous sidebar app behavior is retained for the time being, we're just improving the task data model in parallel.
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.
I've got no objections (other than what Cian has already raised withmakeStaticID), although I'm not familiar enough with either components to feel confident to give the green light
281cf6e tofff8343Compare336477c tof7e0c43Compare
Uh oh!
There was an error while loading.Please reload this page.
This change adds workspace build/agent/app linking to tasks and wires it
into
wsbuilderandprovisionerdserver.Closescoder/internal#948
Closes#20212
Closes#19773