fix glob pattern for impacted projects #1989
Conversation
WalkthroughThe changes update the logic in the Changes
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Simplifies and fixes the glob pattern matching logic in project impact detection, particularly affecting how changed files are matched against project directories.
- Modified
libs/digger_config/digger_config.goto use a consistent*glob pattern for both Terragrunt and non-Terragrunt projects, simplifying the path matching logic - Added test in
libs/digger_config/digger_config_test.goto verify correct behavior when project directory is set to '.' to prevent false matches from subdirectories
2 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
libs/digger_config/digger_config.go (2)
865-872: ‘./*’ pattern misses root-level files whenproject.Dir == "."
filepath.Join(project.Dir, "*")evaluates to"./*"for dot-projects.
Changed files that live at the repository root (e.g."main.tf") will not match"./*"because they do not start with"./".
As a result, a project whoseDiris"."will never be marked as impacted by root-level changes – an obvious regression compared to the previous recursive behaviour.- includePatterns = append(includePatterns, filepath.Join(project.Dir, "*")) + if project.Dir == "." || project.Dir == "" { + includePatterns = append(includePatterns, "*") // match root-level files + } else { + includePatterns = append(includePatterns, filepath.Join(project.Dir, "*")) + }Please add a companion test proving that a root-level change (e.g.
"main.tf") does impact a"."project.
861-870: Repeatedly appending the same pattern inside the inner loop inflatesincludePatterns
includePatternsis re-initialised for every changed file, then appended to: over N files the base pattern is added N times, growing the slice quadratically and doing redundant work during pattern matching.Move the construction of
includePatternsoutside thefor _, changedFile := …loop:- for _, changedFile := range changedFiles { - includePatterns := project.IncludePatterns - excludePatterns := project.ExcludePatterns - includePatterns = append(includePatterns, ...) - ... - } + includePatterns := append( + slices.Clone(project.IncludePatterns), // keep original slice intact + filepath.Join(project.Dir, "*"), + ) + excludePatterns := project.ExcludePatterns + + for _, changedFile := range changedFiles { + if MatchIncludeExcludePatternsToFile(changedFile, includePatterns, excludePatterns) { + ... + } + }This removes needless allocations and comparisons while retaining behaviour.
🧹 Nitpick comments (1)
libs/digger_config/digger_config_test.go (1)
1270-1285: Test misses root-level change & contains dead code
The new test proves that sub-directory changes do not impact a dot-project, but it does not prove that root-level changes do.
Please extend the test (or add a second one) withchangedFiles := []string{"main.tf"}and assert one impacted project.Lines 1281-1282 contain a commented-out variable declaration that can be safely removed.
- //expectedImpactingLocations := map[string]ProjectToSourceMapping{}Cleaning this up keeps the suite tidy and focused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/digger_config/digger_config.go(1 hunks)libs/digger_config/digger_config_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
fix for #1988
Summary by CodeRabbit