fix(agent/agentcontainers): respect ignore files #19016
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements respect for .gitignore files when discovering devcontainer configurations. The change ensures that files and directories specified in .gitignore patterns are excluded from devcontainer discovery, preventing ignored directories from being processed.
- Adds gitignore pattern parsing and matching using the go-git library
- Creates a new ignore package to handle gitignore pattern processing with afero filesystem compatibility
- Updates devcontainer discovery logic to skip ignored files and directories
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds go-git dependency and updates gliderlabs/ssh version |
| agent/agentcontainers/ignore/dir.go | New module for parsing gitignore patterns and converting file paths to components |
| agent/agentcontainers/ignore/dir_test.go | Unit tests for the FilePathToParts function |
| agent/agentcontainers/api.go | Integrates gitignore pattern matching into devcontainer discovery |
| agent/agentcontainers/api_test.go | Test cases verifying gitignore functionality |
the initial call duplicates work done in Walk
DanielleMaywood
force-pushed
the
danielle/respect-ignore-files
branch
from
a913cf8 to
f5d16ea
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API (discoverDevcontainersInProject)
participant IgnorePkg
participant Filesystem
User->>API (discoverDevcontainersInProject): Request devcontainer discovery
API->>IgnorePkg: Read and parse .gitignore patterns
IgnorePkg->>Filesystem: Read .gitignore files recursively
IgnorePkg-->>API: Return ignore patterns
loop Walk project directory
API->>Filesystem: Read directory entry
API->>IgnorePkg: Check if path matches ignore pattern
alt Path is ignored
API-->>Filesystem: Skip directory or file
else Path is not ignored
API->>Filesystem: Check for devcontainer
alt Devcontainer found
API-->>User: Report devcontainer
end
end
end
API-->>User: Return discovered devcontainers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
agent/agentcontainers/ignore/dir.go (2)
21-21: Usestrings.Splitfor better Go version compatibility.The
strings.SplitSeqfunction was introduced in Go 1.23 and may not be available in older Go versions. Consider using the standardstrings.Splitfor better compatibility.- for segment := range strings.SplitSeq(filepath.Clean(path), string(filepath.Separator)) { + for _, segment := range strings.Split(filepath.Clean(path), string(filepath.Separator)) {
38-38: Usestrings.Splitfor better Go version compatibility.The
strings.SplitSeqfunction was introduced in Go 1.23 and may not be available in older Go versions. Consider using the standardstrings.Splitfor better compatibility.- for s := range strings.SplitSeq(string(data), "\n") { + for _, s := range strings.Split(string(data), "\n") {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
agent/agentcontainers/api.go(2 hunks)agent/agentcontainers/api_test.go(1 hunks)agent/agentcontainers/ignore/dir.go(1 hunks)agent/agentcontainers/ignore/dir_test.go(1 hunks)go.mod(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
agent/agentcontainers/api.go (4)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : Coder emphasizes clear error handling, with specific patterns required: Concise error messages that avoid phrases like "failed to"; Wrapping errors with %w to maintain error chains; Using sentinel errors with the "err" prefix (e.g., errNotFound).
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : Follow Uber Go Style Guide
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : The codebase is rigorously linted with golangci-lint to maintain consistent code quality.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
agent/agentcontainers/ignore/dir_test.go (5)
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests should run in parallel using t.Parallel() to ensure efficient testing and expose potential race conditions.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests must use t.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.035Z
Learning: Applies to coderd/coderdtest/**/* : The coderdtest package in coderd/coderdtest/ provides utilities for creating test instances of the Coder server, setting up test users and workspaces, and mocking external components.
agent/agentcontainers/api_test.go (2)
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
go.mod (4)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : The codebase is rigorously linted with golangci-lint to maintain consistent code quality.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : Coder emphasizes clear error handling, with specific patterns required: Concise error messages that avoid phrases like "failed to"; Wrapping errors with %w to maintain error chains; Using sentinel errors with the "err" prefix (e.g., errNotFound).
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : Follow Uber Go Style Guide
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : OAuth2-compliant error responses must use writeOAuth2Error in Go code
agent/agentcontainers/ignore/dir.go (2)
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : Follow Uber Go Style Guide
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : Coder emphasizes clear error handling, with specific patterns required: Concise error messages that avoid phrases like "failed to"; Wrapping errors with %w to maintain error chains; Using sentinel errors with the "err" prefix (e.g., errNotFound).
🔇 Additional comments (15)
go.mod (1)
487-487: ✅ Approve go-git v5.16.2 – latest stable release with no known vulnerabilities
- go.mod (line 487): added
github.com/go-git/go-git/v5 v5.16.2, which is the most recent tag (2025-06-09).- GitHub vulnerability alerts for go-git/go-git returned zero advisories.
This change cleanly supports the gitignore parsing functionality as intended.
agent/agentcontainers/api.go (6)
24-24: LGTM: Appropriate gitignore import for pattern matching.The import of
github.com/go-git/go-git/v5/plumbing/format/gitignorecorrectly provides the gitignore pattern matching functionality described in the PR objectives.
30-30: LGTM: Internal ignore package import.The import of the new internal
ignorepackage is correctly added to support the gitignore functionality.
474-479: LGTM: Well-structured gitignore pattern reading and matcher setup.The implementation correctly separates pattern reading from matching, with proper error handling using
xerrors.Errorfwith%wfor error chain preservation.
487-487: LGTM: Proper path conversion for gitignore matching.Converting file paths to parts using
ignore.FilePathToPartsis the correct approach for gitignore pattern matching, ensuring proper path normalization.
489-498: LGTM: Efficient directory skipping with gitignore support.The implementation correctly uses
fs.SkipDirto skip ignored directories entirely, which is both performant and follows Go file walking conventions. The logic properly handles directory traversal while respecting gitignore patterns.
500-502: LGTM: Complete file filtering with gitignore support.The file filtering logic correctly uses
matcher.Match(pathParts, false)to identify ignored files and skip them, completing the gitignore integration for both directories and files.agent/agentcontainers/ignore/dir_test.go (3)
1-10: LGTM: Proper test package structure and imports.Using the external test package
ignore_testis good practice for testing public APIs. The imports are minimal and appropriate, usingtestify/requirewhich is consistent with the codebase patterns.
12-13: LGTM: Proper test structure with parallel execution.The test function correctly uses
t.Parallel()as required by the coding guidelines, which improves test suite performance and helps identify race conditions.
15-47: LGTM: Comprehensive table-driven test with excellent coverage.The test implementation excellently covers various path scenarios including:
- Absolute and relative paths
- Edge cases (empty, root paths)
- Path normalization (removing
.,.., multiple slashes)- Trailing slashes
The table-driven approach with parallel subtests follows Go best practices, and the test naming using
fmt.Sprintfcreates unique identifiers as recommended for concurrent tests.agent/agentcontainers/api_test.go (3)
3365-3387: LGTM! Excellent test coverage for nested gitignore functionality.This test case properly validates that nested
.gitignorefiles are respected during devcontainer discovery, ensuring that ignored paths are excluded while non-ignored paths are still discovered.
3348-3387: Well-implemented test cases for gitignore functionality.These two test cases provide excellent coverage for the new gitignore-aware devcontainer discovery feature. They test both root-level and nested
.gitignorefiles, ensuring that ignored paths are properly excluded during filesystem traversal.The test structure follows the existing pattern and integrates well with the
TestDevcontainerDiscoverytest suite.
3348-3364: No change needed for devcontainer config file path format. The code intentionally supports both “.devcontainer/devcontainer.json” and “.devcontainer.json” (seedevcontainerConfigPathsin agent/agentcontainers/api.go), so the test’s use of “.devcontainer.json” is valid and exercises that alternate lookup.agent/agentcontainers/ignore/dir.go (2)
47-68: LGTM! Well-structured recursive pattern collection.The function correctly walks the filesystem to collect gitignore patterns from all directories. The error handling properly propagates errors, the logic is sound, and it integrates well with the afero filesystem abstraction.
1-12: LGTM! Clean package structure and appropriate dependencies.The package is well-structured with appropriate imports. The integration of
go-gitfor gitignore pattern parsing withaferofilesystem abstraction aligns perfectly with the PR objectives.
| @@ -122,7 +122,7 @@ require ( | |||
| github.com/fergusstrange/embedded-postgres v1.31.0 | |||
| github.com/fullsailor/pkcs7 v0.0.0-20190404230743-d7302db945fa | |||
| github.com/gen2brain/beeep v0.11.1 | |||
| github.com/gliderlabs/ssh v0.3.4 | |||
| github.com/gliderlabs/ssh v0.3.8 | |||
There was a problem hiding this comment.
I'm assuming this is a result of adding go-git as a dependency.
There was a problem hiding this comment.
It happened when I added go-git, yeah, unfortunate as it feels unrelated.
Closes #19011
We now use go-git's
gitignoreplumbing implementation to parse the.gitignorefiles and match against the patterns generated. We use this to ignore any ignored files in the git repository.Unfortunately I've had to slightly re-implement some of the interface exposed by
go-gitbecause they usebilly.Filesysteminstead ofafero.Fs.