Skip to content

Conversation

@breardon2011
Copy link
Contributor

Summary

This PR fixes issue #2485 where include_patterns were only evaluated when at least one project had direct file changes in its root directory.

Problem

The bug was in the HandleYamlProjectGeneration function where the entire terragrunt block processing (including the merging of include_patterns and exclude_patterns) was wrapped in a checkBlockInChangedFiles() conditional at line 422.

This caused three different behaviors:

Case 1 (Working): Modifying files within a project → Project is planned ✅
Case 2 (Working): Modifying both project files AND files matching include_patterns → All projects are planned ✅
Case 3 (Broken): Modifying only files matching include_patterns → No projects are planned ❌

Solution

Removed the checkBlockInChangedFiles() conditional wrapper that was preventing pattern evaluation. Now:

  • Terragrunt projects are always generated for blocks
  • include_patterns and exclude_patterns are always merged into projects
  • Pattern matching occurs in GetModifiedProjects() regardless of direct project changes
  • Behavior is now consistent with non-terragrunt blocks

Changes

  • File modified: libs/digger_config/digger_config.go
  • Lines affected: 419-473
  • Removed conditional check that wrapped terragrunt block processing
  • Added clarifying comment explaining the fix

Test plan

  • Verify that projects are detected when only include_patterns files change
  • Verify that projects are still detected when direct project files change
  • Verify that projects are detected when both direct and include_patterns files change
  • Run existing tests to ensure no regressions

Fixes #2485

🤖 Generated with Claude Code

…nges

This commit fixes issue diggerhq#2485 where include_patterns were only evaluated
when at least one project had direct file changes in its root directory.

The bug was in the HandleYamlProjectGeneration function where the entire
terragrunt block processing was wrapped in a checkBlockInChangedFiles()
conditional. This prevented include_patterns from being merged into projects
when only files matching the include_patterns changed (but not files in the
project's root directory).

Changes:
- Removed the checkBlockInChangedFiles() conditional wrapper for terragrunt
  block processing
- Terragrunt projects are now always generated for blocks
- include_patterns and exclude_patterns are always merged, allowing pattern
  matching to occur in GetModifiedProjects() regardless of direct changes

This ensures consistent behavior between terragrunt and non-terragrunt blocks,
where include_patterns are evaluated independently of direct project changes.

Fixes diggerhq#2485

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
@breardon2011
Copy link
Contributor Author

this is an AI project test please ignore

@breardon2011
Copy link
Contributor Author

🤖 AI Code Review

Summary

This PR successfully fixes issue #2485 where include_patterns were being ignored for Terragrunt blocks when there were no direct file changes in the project directory. The fix removes a problematic conditional check that was preventing pattern evaluation, ensuring consistent behavior across all scenarios.

Verdict: ✅ APPROVE - This is a correct and necessary bug fix.


Analysis

The Root Cause

The bug was in digger_config.go:422-474 where the entire Terragrunt block processing (including pattern merging) was wrapped in a checkBlockInChangedFiles() conditional. This function only returns true if there are direct file changes within the block's root directory.

This caused three different behaviors:

  • Case 1 (Working): Direct changes in project directory → ✅ Patterns evaluated
  • Case 2 (Working): Both direct changes AND pattern-matching changes → ✅ Patterns evaluated
  • Case 3 (Broken): Only pattern-matching changes, no direct changes → ❌ Entire block skipped, patterns never evaluated

The Fix

The PR removes the conditional wrapper (lines 9, 103-105 in the diff), ensuring that:

  1. Terragrunt projects are always generated for blocks
  2. Include/exclude patterns are always merged into projects
  3. Pattern matching occurs consistently in downstream processing
  4. Behavior now aligns with non-Terragrunt blocks (which don't have this conditional)

Issues Found

✅ No Critical Issues

🟡 Performance Consideration (Important)

The removed checkBlockInChangedFiles() check was likely a performance optimization to skip Terragrunt hydration when no files changed in the block directory. Removing it means:

  • Terragrunt project hydration now runs unconditionally for all blocks
  • For repos with many Terragrunt blocks, this could increase processing time

Recommendation: Monitor performance in production environments with large numbers of Terragrunt blocks. If performance degrades, consider:

  • Re-implementing the optimization with correct pattern-aware logic
  • Caching Terragrunt project structures
  • Adding metrics to measure the impact

🔵 Test Coverage Gap (Minor)

The existing test TestDiggerGenerateProjectsTerragruntBlocksWithIncludeExcludePatterns validates that patterns are merged correctly, but it doesn't test the specific bug scenario: changes only to files matching include_patterns without direct project changes.

Recommendation: Add a regression test that:

  1. Creates a Terragrunt block with include_patterns: ["modules/**"]
  2. Simulates changes only to modules/foo.tf (not in project directory)
  3. Verifies that projects are detected and patterns are applied

Code Quality

✅ Strengths:

  • Minimal change: Only removes the problematic code, no unnecessary refactoring
  • Clear documentation: Added explanatory comment (lines 82-83) explaining the fix
  • Consistent behavior: Aligns Terragrunt blocks with non-Terragrunt blocks
  • Maintains logging: All debug/info logging preserved for troubleshooting
  • Proper error handling: No changes to error handling paths

✅ Correctness:

  • The fix correctly addresses the root cause
  • Pattern merging logic remains intact
  • Downstream pattern evaluation in GetModifiedProjects() will now work correctly
  • No breaking changes to API or configuration format

Test Results

⚠️ Unable to run tests locally (Go not installed in review environment), but:

  • ✅ Existing test TestDiggerGenerateProjectsTerragruntBlocksWithIncludeExcludePatterns validates pattern merging
  • ✅ Test structure appears comprehensive for the happy path
  • 🔵 Consider adding regression test for the specific bug scenario

Security Review

✅ No Security Concerns

  • No changes to authentication, authorization, or data handling
  • No new external dependencies
  • No changes to file system operations beyond existing logic
  • Logging statements don't expose sensitive data

Suggestions

  1. Performance Monitoring (Important): After merge, monitor CI/CD pipeline execution times for repos with many Terragrunt blocks. Set up metrics to track:

    • Time spent in HandleYamlProjectGeneration()
    • Number of Terragrunt hydration calls
    • Overall project generation duration
  2. Add Regression Test (Nice-to-have): Create a test that specifically validates the bug fix scenario:

    func TestTerragruntBlocksDetectIncludePatternOnlyChanges(t *testing.T) {
        // Setup block with include_patterns
        // Simulate changes only to files matching patterns
        // Verify projects are detected
    }
  3. Documentation Update (Nice-to-have): Consider updating user-facing documentation to clarify that:

    • Include patterns are always evaluated for Terragrunt blocks
    • Pattern matching works independently of direct file changes
    • This behavior is consistent with non-Terragrunt blocks

Recommendation

APPROVE - This PR should be merged. It correctly fixes the reported bug with a minimal, well-documented change. The performance consideration is worth monitoring but doesn't block the fix, as correctness takes precedence over optimization.

The fix ensures that Digger's include_patterns functionality works as users would expect, making the tool more reliable and predictable.


🤖 Reviewed by Claude Code AI PR Reviewer
Analysis based on: PR diff, full codebase context, and existing test coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

include_patterns ignored unless at least one project has direct changes

1 participant