- Notifications
You must be signed in to change notification settings - Fork914
feat: support devcontainer agents in ui and unify backend#18332
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:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c9f4ca4
to18e1593
Comparefc1a236
to414b6f4
Compare414b6f4
toecfe483
Compare8f12d26
to79e1844
CompareThere 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.
haven't managed to read all the Go yet but will read it by Monday
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
016b6b0
tod3bda05
Compareabc9999
tod4f208b
CompareUh oh!
There was an error while loading.Please reload this page.
for_,id:=rangesubAgentIDs { | ||
err:=api.subAgentClient.Delete(deleteCtx,id) | ||
iferr!=nil { | ||
api.logger.Error(api.ctx,"delete subagent record during shutdown failed", | ||
slog.Error(err), | ||
slog.F("agent_id",id), | ||
) | ||
} | ||
} |
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.
Suggestion: should we do this in parallel instead?
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.
What's the argument for doing it in parallel? My gut feeling is I want to avoid spamming coderd. If this is something we want, we should allow deleting multiple agents in one request instead. // cc@DanielleMaywood
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.
Main argument is to reduce time to shutdown. Currently it is worst case maximumlen(subAgentIDs) * deleteCtx timeout
. Not a blocker though.
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.
Gotcha 👍🏻. It's btw just1 * deleteCtx
timeout, we give up on deleting the rest if it takes too long.
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.
Gotcha. That may be another argument for doing multiple at once - I'd pefer it to be all or nothing.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
} | ||
// From codersdk/workspaceagents.go | ||
export interface WorkspaceAgentDevcontainerAgent { |
BrunoQuaresmaJun 16, 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.
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.
This name sounds confusing to me 🤔. From my understanding, we have two agents, WorkspaceAgent, and Devcontainer Agent, is that correct? If yes, why not call DevcontainerAgent to simplify the naming?
I think prefixing the resource names, when not necessary, a bad practice, but I see we do that in a lot of places in coderd, so it is not a blocker, but I would like to bring attention to it.
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 do see your point. Technically I suppose "Workspace" in "WorkspaceAgent" is also somehwhat useless. The main idea with this is to follow existing naming conventions but also to symbolize the dependency tree. A devcontainer agent (a workspace agents subagent) cannot exist without a (parent) workspace agent. Hence it is nested in the naming. Another existing type likeWorkspaceAgentContainer
also already exists so I don't think it's worth diverging right now without a plan for how to tackle existing names.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
consthandleRebuildDevcontainer=async()=>{ | ||
setIsRebuilding(true); | ||
setSubAgentRemoved(true); | ||
letrebuildSucceeded=false; | ||
try{ | ||
constresponse=awaitfetch( |
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.
In the FE, we handle requests in two ways: queries or mutations. In this case, you should wrap this request into a mutation.Here is the docs.
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.
When using a mutation, you can invalidate other queries or update their data to reflect the most recent status, instead of doing it manually, so you wouldn't need the isRebuilding or subAgentRemoved statuses.Here is how you can do it. If you need more examples, you can search in the codebase for "invalidateQueries".
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.
Left a few comments related to the FE codebase.
I don't have any blocking comments, but deferring final approval to@DanielleMaywood |
Uh oh!
There was an error while loading.Please reload this page.
This PR adds consolidates two container endpoints on the backend and improves the frontend devcontainer support by showing names and displaying apps as appropriate.
With this change, the frontend now has knowledge of the subagent and we can also display things like port forwards.
The frontend was updated to show dev container labels on the border as well as subagent connection status. The recreation flow was also adjusted a bit to show placeholder app icons when relevant.
Support for
apps
was also added, although these are still WIP on the backend. And the port forwarding utility was added in since the sub agents now provide the necessary info.Fixescoder/internal#666