- Notifications
You must be signed in to change notification settings - Fork1k
feat(agent/agentcontainers): allow auto start for discovered containers#19040
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
| strings.HasPrefix(s,"CODER_AGENT_AUTH=")|| | ||
| strings.HasPrefix(s,"CODER_AGENT_DEVCONTAINERS_ENABLE=") | ||
| strings.HasPrefix(s,"CODER_AGENT_DEVCONTAINERS_ENABLE=")|| | ||
| strings.HasPrefix(s,"CODER_AGENT_DEVCONTAINERS_PROJECT_DISCOVERY_ENABLE=") |
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.
Missed this in a previous PR#19016
ee6b803 todf5255bCompare| } | ||
| api.knownDevcontainers[workspaceFolder]=dc | ||
| api.broadcastUpdatesLocked() |
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'll now broadcast updates as we find the dev containers. This makes it feel a little quicker when there is a lot of files being searched through.
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce support for automatically starting devcontainers during project discovery if their configuration specifies Changes
Sequence Diagram(s)sequenceDiagram participant User participant API participant DevcontainerCLI User->>API: Triggers project discovery API->>DevcontainerCLI: ReadConfig(devcontainer.json) DevcontainerCLI-->>API: Returns config (with AutoStart flag) API->>API: If AutoStart == true, set status to "Starting" API->>API: Broadcast devcontainer update API->>DevcontainerCLI: (async) CreateDevcontainer(workspace, configPath) DevcontainerCLI-->>API: (container started)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn morehere, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
agent/agentcontainers/api.go (2)
146-147:LGTM!Correctly filters out the project discovery environment variable to prevent configuration conflicts between parent and child agents.
553-553:Broadcasting updates during discovery improves UX.This allows the UI to show devcontainers as they're discovered rather than waiting for the entire discovery process to complete.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agent/agentcontainers/api.go(2 hunks)agent/agentcontainers/api_test.go(1 hunks)agent/agentcontainers/devcontainercli.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
agent/agentcontainers/api_test.go (5)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.064Z
Learning: Applies to **/*_test.go : All tests must uset.Parallel() to run concurrently, which improves test suite performance and helps identify race conditions.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.064Z
Learning: Applies to **/*_test.go : All tests should run in parallel usingt.Parallel() to ensure efficient testing and expose potential race conditions.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T00:33:07.777Z
Learning: Applies to **/*_test.go : Use unique identifiers (e.g., fmt.Sprintf("test-client-%s-%d", t.Name(), time.Now().UnixNano())) and never use hardcoded names in concurrent tests to prevent race conditions.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T00:33:07.155Z
Learning: Applies to **/*_test.go : Use unique identifiers (e.g., fmt.Sprintf("test-client-%s-%d", t.Name(), time.Now().UnixNano())) and never use hardcoded names in concurrent tests to prevent race conditions.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T00:33:10.063Z
Learning: Applies to **/*_test.go : Use unique identifiers (e.g., fmt.Sprintf("test-client-%s-%d", t.Name(), time.Now().UnixNano())) and never use hardcoded names in concurrent tests to prevent race conditions.
🔇 Additional comments (3)
agent/agentcontainers/devcontainercli.go (1)
94-94:LGTM!The
AutoStartfield is correctly added to theCoderCustomizationstruct with appropriate JSON serialization tags.agent/agentcontainers/api.go (1)
528-528:Good addition of workspace folder context to logger.This improves traceability during project discovery.
agent/agentcontainers/api_test.go (1)
3572-3816:LGTM! Well-implemented test for auto-start functionality.This new test comprehensively validates the auto-start behavior described in the PR objectives. The implementation follows established patterns and best practices:
- Properly uses
t.Parallel()as required by the coding guidelines- Covers multiple scenarios: single enabled/disabled, mixed configurations, and multiple enabled devcontainers
- Uses proper mock expectations with
gomock.InOrder()to verify thatUp()is called only for devcontainers withAutoStart: true- Follows the existing test structure and cleanup patterns
- Uses appropriate test utilities and context handling
The test effectively validates that discovered devcontainers with the
AutoStartflag set will trigger container creation viaUp(), while those without it will not, which aligns perfectly with the PR's goal of implementing auto-start for discovered containers.
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.
LGTM!
| gofunc() { | ||
| deferapi.asyncWg.Done() | ||
| _=api.CreateDevcontainer(dc.WorkspaceFolder,dc.ConfigPath) |
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: I suppose it returns an error, should we log it somewhere?
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.
CreateDevcontainer already does a reasonable amount of logging. I don't think theerror returned has any additional information to what is already logged.
| // detects an error has been logged. | ||
| }) | ||
| t.Run("AutoStart",func(t*testing.T) { |
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.
Nice tests!
66cf90c intomainUh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#711