fix(agent): start devcontainers through agentcontainers package #18471
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes coder/internal#706 Context for the implementation here coder/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 of being reliant on packages which may or may not exist in the user-provided image.
There was a problem hiding this comment.
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.
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.
Suggested change
| _, _ = 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes coder/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.