- Notifications
You must be signed in to change notification settings - Fork921
fix(agent): start devcontainers through agentcontainers package#18471
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?
Conversation
Fixescoder/internal#706Context for the implementation herecoder/internal#706 (comment)Synchronously starts dev containers defined in terraform with our`DevcontainerCLI` abstraction, instead of piggybacking off of our`agentscripts` package. This gives us more control over logs, instead ofbeing reliant on packages which may or may not exist in theuser-provided image.
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.
Pull Request Overview
This PR refactors how devcontainers are started by splitting out the devcontainer CLI functionality from the agentscripts package. Key changes include removing post start scripts from agentscripts, updating tests to reference new expected outputs and behavior, and updating the API to synchronously trigger devcontainer creation via a new CreateDevcontainer method.
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/agentscripts/agentscripts_test.go | Removed tests for post-start scripts and updated expectations. |
| agent/agentscripts/agentscripts.go | Refactored to remove runnerScript and post start script logic. |
| agent/agentcontainers/testdata/devcontainercli/parse/up.expected | Updated expected log output for devcontainer CLI. |
| agent/agentcontainers/devcontainercli_test.go | Updated test file references to expected log output file. |
| agent/agentcontainers/devcontainercli.go | Updated option defaults and logging writer behavior. |
| agent/agentcontainers/devcontainer_test.go | Entire file removed, reflecting changes in devcontainer handling. |
| agent/agentcontainers/devcontainer.go | Removed devcontainer script extraction helper functions. |
| agent/agentcontainers/api.go | Modified recreateDevcontainer to return errors and added CreateDevcontainer. |
| agent/agent.go | Removed legacy handling of experimental devcontainers and added calls to containerAPI.CreateDevcontainer. |
Comments suppressed due to low confidence (1)
| @@ -401,6 +408,7 @@ func (l *devcontainerCLILogWriter) Write(p []byte) (n int, err error) { | |||
| } | |||
| if logLine.Level >= 3 { | |||
| l.logger.Info(l.ctx, "@devcontainer/cli", slog.F("line", string(line))) | |||
| _, _ = l.writer.Write([]byte(logLine.Text + "\n")) | |||
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 might be beneficial to check the error returned by l.writer.Write rather than ignoring it. Handling any potential write failure could improve the reliability of the logging.
| _,_=l.writer.Write([]byte(logLine.Text+"\n")) | |
| if_,err:=l.writer.Write([]byte(logLine.Text+"\n"));err!=nil { | |
| l.logger.Error(l.ctx,"failed to write to log writer",slog.Error(err),slog.F("text",logLine.Text)) | |
| } |
Copilot uses AI. Check for mistakes.
Fixescoder/internal#706
Context for the implementation here
coder/internal#706 (comment)
Synchronously starts dev containers defined in terraform with our
DevcontainerCLIabstraction, instead of piggybacking off of ouragentscriptspackage. This gives us more control over logs, instead of being reliant on packages which may or may not exist in the user-provided image.