- Notifications
You must be signed in to change notification settings - Fork918
feat(agent/agentcontainers): support apps for dev container agents#18346
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d06023f to08f0f9cCompare08f0f9c to6270098Compare This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
c3ba85a toc800571Compare75709b9 to8ec61b6CompareThere 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.
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)
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.
I still need to take a closer look at the tests, but I identified some things that may require some refactoring.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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 bothCODER_URL andCODER_AGENT_URL in the product, and a third one seems like too much.
IMOCODER_URL would be more appropriate, however, it would be nice if these environment variables werealso set on the parent workspace/agent (withCODER_AGENT_URL we're at 3/4). The reason is that if a user runsdevcontainer up manually, these values are then serializable into the docker container label.
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 replaced it withCODER_URL. If you'd ratherCODER_AGENT_URL I'm happy to update it again.
I don't think we can rely on anything serialized at the point ofdevcontainer up, as theCODER_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, that's a good point about the conflicting env. Let's go withCODER_URL for now 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It'd be nice to consolidate similar things rather than having too many knobs, perhaps aagentcontainers.WithManifestInfo(url, owner, workspace)? (I'm aware my naming 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note for later: we'll need to document these environment variables somewhere.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
529fb50 intomainUh 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.
A few more things but other than that, looks good, nice work!
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we modifyagent/agent.go to set this on the parent as well?
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.
Just to ensure I know what you're referring to:
In this stanza here?
Lines 1293 to 1309 in529fb50
| // Define environment variables that should be set for all commands, | |
| // and then merge them with the current environment. | |
| envs:=map[string]string{ | |
| // Set env vars indicating we're inside a Coder workspace. | |
| "CODER":"true", | |
| "CODER_WORKSPACE_NAME":manifest.WorkspaceName, | |
| "CODER_WORKSPACE_AGENT_NAME":manifest.AgentName, | |
| // Specific Coder subcommands require the agent token exposed! | |
| "CODER_AGENT_TOKEN":*a.sessionToken.Load(), | |
| // Git on Windows resolves with UNIX-style paths. | |
| // If using backslashes, it's unable to find the executable. | |
| "GIT_SSH_COMMAND":fmt.Sprintf("%s gitssh --",unixExecutablePath), | |
| // Hide Coder message on code-server's "Getting Started" page | |
| "CS_DISABLE_GETTING_STARTED_OVERRIDE":"true", | |
| } |
| subAgentConfig.DisplayApps = displayApps | ||
| subAgentConfig.Apps = apps |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Comment: Typically these methods exist inagentsdk/convert.go. Personally I think it makes sense to keep here though.
Uh oh!
There was an error while loading.Please reload this page.
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