feat(agent/agentcontainers): support apps for dev container agents #18346
Conversation
d06023f to
08f0f9c
Compare
08f0f9c to
6270098
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
c3ba85a to
c800571
Compare
75709b9 to
8ec61b6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for passing app definitions to sub agents based on dev container customizations and updates related tests and API integrations.
- Extends the API to accept new app-related configuration options.
- Adds new tests for sub-agent apps functionality and updates environment variable handling in the devcontainer CLI.
- Updates various components (API, client, tests, and mocks) to support the new apps configuration.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| agent/api.go | Appends options for user and workspace names and devcontainer apps; injects environment variables. |
| agent/agenttest/client.go | Introduces GetSubAgentApps for retrieving app definitions. |
| agent/agentcontainers/subagent_test.go | Adds tests for sub-agent creation with app configurations. |
| agent/agentcontainers/subagent.go | Extends the SubAgent struct and converts app configuration for API calls. |
| agent/agentcontainers/devcontainercli_test.go | Updates devcontainer CLI test to include an explicit env argument. |
| agent/agentcontainers/devcontainercli.go | Adds environment variables to CLI commands. |
| agent/agentcontainers/api_test.go | Updates tests to include env validations for sub-agent app settings. |
| agent/agentcontainers/acmock/acmock.go | Adjusts mock method signatures to include the new env arg. |
Comments suppressed due to low confidence (1)
agent/agentcontainers/api.go
Outdated
| fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", dc.Name), | ||
| fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.userName), | ||
| fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName), | ||
| fmt.Sprintf("CODER_DEPLOYMENT_URL=%s", api.subAgentURL), |
There was a problem hiding this comment.
| fmt.Sprintf("CODER_DEPLOYMENT_URL=%s", api.subAgentURL), | |
| fmt.Sprintf("CODER_AGENT_URL=%s", api.subAgentURL), |
The naming is slightly unfortunate but we currently have both CODER_URL and CODER_AGENT_URL in the product, and a third one seems like too much.
IMO CODER_URL would be more appropriate, however, it would be nice if these environment variables were also set on the parent workspace/agent (with CODER_AGENT_URL we're at 3/4). The reason is that if a user runs devcontainer up manually, these values are then serializable into the docker container label.
There was a problem hiding this comment.
I've replaced it with CODER_URL. If you'd rather CODER_AGENT_URL I'm happy to update it again.
I don't think we can rely on anything serialized at the point of devcontainer up, as the CODER_WORKSPACE_AGENT_NAME will be the parents agent name, not the child. So I'm not sure there is too much benefit there.
There was a problem hiding this comment.
Yeah, that's a good point about the conflicting env. Let's go with CODER_URL for now 👍🏻
agent/api.go
Outdated
| if manifest != nil { | ||
| containerAPIOpts = append(containerAPIOpts, | ||
| agentcontainers.WithUserName(manifest.OwnerName), | ||
| agentcontainers.WithWorkspaceName(manifest.WorkspaceName), |
There was a problem hiding this comment.
It'd be nice to consolidate similar things rather than having too many knobs, perhaps a agentcontainers.WithManifestInfo(url, owner, workspace)? (I'm aware my nam
8000
ing suggestion could've been better 😅.)
| @@ -153,6 +156,15 @@ func WithSubAgentEnv(env ...string) Option { | |||
| } | |||
| } | |||
|
|
|||
| // WithManifestInfo sets the owner name, and workspace name | |||
| // for the sub-agent. | |||
| func WithManifestInfo(owner, workspace string) Option { | |||
There was a problem hiding this comment.
nit: the naming suggests you can pass in the entire manifest?
| fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", dc.Name), | ||
| fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName), | ||
| fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName), | ||
| fmt.Sprintf("CODER_URL=%s", api.subAgentURL), |
There was a problem hiding this comment.
Note for later: we'll need to document these environment variables somewhere.
| if config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, | ||
| []string{ | ||
| fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", dc.Name), | ||
| fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName), |
There was a problem hiding this comment.
Can we modify agent/agent.go to set this on the parent as well?
There was a problem hiding this comment.
| subAgentConfig.DisplayApps = displayApps | ||
| subAgentConfig.Apps = apps |
There was a problem hiding this comment.
We should log app creation errors from the response after agent creation too.
| Hidden: &app.Hidden, | ||
| Order: &app.Order, | ||
| Subdomain: &app.Subdomain, | ||
| } |
There was a problem hiding this comment.
Did we check to see what the "defaults" when creating apps in terraform?
| URL string `json:"url"` | ||
| } | ||
|
|
||
| func (app SubAgentApp) ToProtoApp() (*agentproto.CreateSubAgentRequest_App, error) { |
There was a problem hiding this comment.
Comment: Typically these methods exist in agentsdk/convert.go. Personally I think it makes sense to keep here though.
Add apps to the sub agent based on the dev container customization.
The implementation also provides the following env variables for use in the devcontainer json
CODER_WORKSPACE_AGENT_NAMECODER_WORKSPACE_USER_NAMECODER_WORKSPACE_NAMECODER_URL