feat(coderd/provisionerdserver): insert sub agent resource#21699
feat(coderd/provisionerdserver): insert sub agent resource#21699DanielleMaywood merged 13 commits intomainfrom
Conversation
DanielleMaywood
force-pushed
the
danielle/devcontainer-resources/provisionerdserver
branch
2 times, most recently
from
8ed9c0d to
36171bd
Compare
DanielleMaywood
force-pushed
the
danielle/devcontainer-resources/provisionerdserver
branch
from
36171bd to
dbb9ad8
Compare
DanielleMaywood
requested review from
johnstcn and
mafredri
and removed request for
mafredri
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Quick review to flag a few things. I'll do a more thorough one later.
coderd/database/migrations/000413_add_subagent_id_to_dev_containers.up.sql
Outdated
Show resolved
Hide resolved
| dc.Id = uuid.New().String() | ||
|
|
||
| if dc.GetSubagentId() != "" { | ||
| subAgentID := uuid.New() |
There was a problem hiding this comment.
How does this work with the agent id from proto option used elsewhere in the code?
There was a problem hiding this comment.
I'm not sure I fully understand the question
There was a problem hiding this comment.
Actually I think this is probably fine, just a very confusing pattern since we're overwriting IDs supplied by the provider. Perhaps a comment is warranted on why we do it?
There was a problem hiding this comment.
I've added a small comment in f8ad555, although not sure how useful it is.
I've tried to fully understand the surrounding context and why it was needed (without it the added test would fail) but I'll be honest it is slightly going over my head 😅 .
| dc.Id = uuid.New().String() | ||
|
|
||
| if dc.GetSubagentId() != "" { | ||
| subAgentID := uuid.New() |
There was a problem hiding this comment.
Actually I think this is probably fine, just a very confusing pattern since we're overwriting IDs supplied by the provider. Perhaps a comment is warranted on why we do it?
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
A few final things, but other than that, looks good. I don't need to re-review.
|
|
||
| // Subagents in devcontainers can also have apps that need | ||
| // tracking for task linking, just like the parent agent's | ||
| // apps above. |
There was a problem hiding this comment.
This doesn't really explain why we replace the subagent ID. But considering it's a common theme throughout the provisioner implementation, I won't push for it here. We should look at improving the reasoning here or refactoring though, because it's almost like an arcane art. 😄
| scriptRunOnStart = append(scriptRunOnStart, true) | ||
| scriptRunOnStop = append(scriptRunOnStop, false) | ||
| scriptsParams.ScriptRunOnStart = append(scriptsParams.ScriptRunOnStart, true) | ||
| scriptsParams.ScriptRunOnStop = append(scriptsParams.ScriptRunOnStop, false) |
There was a problem hiding this comment.
I spent some time scratching my head about this because I had forgotten why I added it, and of course didn't read the comments haha. Not required now, but this should really be broken out into it's own function with a very explicit name.
DanielleMaywood
deleted the
danielle/devcontainer-resources/provisionerdserver
branch
3 participants
Update provisionerdserver to handle the changes introduced to provisionerd in #21602
We create a new relationship between
workspace_agent_devcontainersandworkspace_agentswith the newly createdsubagent_id.