fix: Do not parse excluded dirs during discovery phase #5151
Conversation
atmask
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@atmask is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughSkips parsing of discovered components whose paths match configured exclude-directory patterns while still discovering and reporting them; wires ExcludeDirs into discovery; adds fixtures and an integration test validating excluded template modules are discovered but not parsed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Discovery
participant ExcludeChecker
participant Parser
Client->>Discovery: Start discovery (with ExcludeDirs)
Discovery->>ExcludeChecker: Evaluate discovered component path
alt matches exclude pattern
ExcludeChecker-->>Discovery: mark as excluded (skip parsing)
Note right of Discovery `#dfefff`: Component is discovered and reported\nbut not parsed
else not excluded
ExcludeChecker-->>Discovery: allow parsing
Discovery->>Parser: enqueue parse task
Parser->>Parser: parse component config
end
Discovery->>Client: return discovery results (includes excluded entries)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-03T04:40:01.000ZApplied to files:
🔇 Additional comments (2)
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. Comment |
atmask
changed the title
atmask
changed the title
![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
🧹 Nitpick comments (3)
test/fixtures/stacks/exclude-template-module/live/project-A/module/terragrunt.hcl (1)
1-20: Tidy minor comment typos for clarityThe behavior here (cross‑project dependency plus
values.envinput) matches the intended scenario. Two tiny nits you might consider cleaning up:
- Line 1:
dpeendency→dependency.- Lines 16–17: the comment talks about
${values}but the code actually usesvalues.env; aligning the wording would reduce potential confusion for future readers.internal/runner/runnerpool/builder.go (1)
68-72: ExcludeDirs wiring into discovery matches intended behaviorPassing
terragruntOptions.ExcludeDirsintoNewDiscoveryviaWithExcludeDirsis the right place to ensure components matched by--queue-exclude-dirare still discovered but skipped during parsing.One small polish point: the later comment on Lines 86–87 (“Exclude patterns are handled by the unit resolver…”) now overlaps with the new comment here. Consider rephrasing those two comments together (e.g., noting that discovery uses
ExcludeDirsto skip parsing while the resolver uses them for reporting/queuing) so future readers see a single, consistent story about how excludes flow through the system.test/integration_stacks_test.go (1)
130-150: New integration test correctly guards the regression; tighten comments/expectationsThe test setup and command look solid:
- Uses the new fixture path and
CopyEnvironment/CleanupTerraformFolderin the same way as other stack tests.- Runs
terragrunt stack run planwith--queue-exclude-dir '**/module'from theliveroot, matching the scenario from the linked issue.- Asserts that all four generated units for project A/B, staging/production, appear in the execution queue and that the command succeeds, which would have failed before the fix.
A couple of small refinements you might consider:
- Line 145’s comment says “only the app unit is in the execution queue (not the module-based one)”, but we actually assert four units here. Rewording this to something like “only the generated stack units are queued (template modules are excluded)” would be clearer.
- Optionally, to make the regression more explicit, you could also assert that
stderrdoes not contain the original failure message (e.g.,There is no variable named "values"), or that nomodule/paths appear as units, which would more directly encode the previous bug behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/discovery/discovery.go(2 hunks)internal/runner/runnerpool/builder.go(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/module/main.tf(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/module/terragrunt.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/terragrunt.stack.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/module/main.tf(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/terragrunt.stack.hcl(1 hunks)test/integration_stacks_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/runner/runnerpool/builder.gointernal/discovery/discovery.gotest/integration_stacks_test.go
🧠 Learnings (2)
📚 Learning: 2025-11-03T04:40:01.000Z
Learnt from: ThisGuyCodes
Repo: gruntwork-io/terragrunt PR: 5041
File: test/fixtures/hclvalidate/valid/duplicate-attributes-in-required-providers/main.tf:2-7
Timestamp: 2025-11-03T04:40:01.000Z
Learning: In the terragrunt repository, test fixtures under test/fixtures/hclvalidate/valid/ are intentionally testing that INPUT validation succeeds even when Terraform code contains syntax errors or other issues unrelated to input validation (e.g., duplicate attributes, circular references, invalid locals). The "valid" designation means "valid for input validation purposes" not "syntactically valid Terraform/OpenTofu code".
Applied to files:
test/fixtures/stacks/exclude-template-module/live/project-A/terragrunt.stack.hcltest/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcltest/integration_stacks_test.go
📚 Learning: 2025-02-10T23:20:04.295Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
Applied to files:
test/fixtures/stacks/exclude-template-module/live/project-B/terragrunt.stack.hcltest/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl
🧬 Code graph analysis (2)
internal/discovery/discovery.go (3)
internal/component/component.go (1)
Component(23-46)util/file.go (1)
CanonicalPath(88-99)pkg/log/log.go (1)
Debugf(72-74)
test/integration_stacks_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(882-889)CopyEnvironment(89-105)RunTerragruntCommandWithOutput(1007-1011)util/file.go (1)
JoinPath(626-628)
🔇 Additional comments (8)
test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl (1)
1-9: Fixture correctly exercises stack-onlyvaluesusageThis module config is minimal and correctly uses
values.envto trigger the previous parsing issue whenmodule/isn’t excluded. Looks good as a focused regression fixture.test/fixtures/stacks/exclude-template-module/live/project-A/module/main.tf (1)
1-7: Straightforward variable/output wiringDefining
environmentas a string variable and re‑exposing it via an output is idiomatic and sufficient for the fixture’s needs.test/fixtures/stacks/exclude-template-module/live/project-B/terragrunt.stack.hcl (1)
1-18: Stack fixture matches intended exclusion scenarioThe two units (
staging,production) sourcing./modulewithvalues.envset provide a clear template‑module stack for exercising the exclude‑dir behavior. Comments accurately describe whymodule/must be excluded.test/fixtures/stacks/exclude-template-module/live/project-B/module/main.tf (1)
1-7: Symmetric variable/output definition is fineMirroring Project A’s
environmentvariable/output keeps the fixtures consistent and is sufficient for the dependency/mocking use case.test/fixtures/stacks/exclude-template-module/live/project-A/terragrunt.stack.hcl (1)
1-18: Project A stack fixture aligns with Project B and test expectationsDefining
stagingandproductionunits that source./moduleand setvalues.envmatches the pattern used for Project B and lines up with the integration test’s expectation of four generated units.test/integration_stacks_test.go (1)
52-58: Fixture constant addition is consistentAdding
testFixtureStackExcludeTemplateModulefollows the existing naming and layout pattern for stack fixtures and is correctly referenced by the new test.internal/discovery/discovery.go (2)
905-924: LGTM! Clean implementation of parse-skipping logic.The
shouldSkipParsingmethod correctly implements the exclusion check with appropriate safeguards:
- Early return when no patterns exist
- Safe default (don't skip) when path canonicalization fails
- Uses precompiled patterns for efficient matching
The approach aligns with the PR objective to skip parsing excluded directories while still discovering them for reporting.
943-948: LGTM! Correct integration of exclusion check.The integration properly prevents parsing of excluded components while preserving discovery. The debug log message provides good visibility for troubleshooting, and the inline comment clearly explains the intended behavior.
This correctly implements the fix for issue #5124 where excluded directories were being parsed during discovery despite being excluded from execution.
…contents while in the discovery phase
atmask
force-pushed
the
issue-5124-do-not-parse-excluded
branch
from
7f08f13 to
c39f4a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/runner/runnerpool/builder.go (1)
68-72: ExcludeDirs wiring into discovery looks correct; consider updating comment to reflect new behaviorPassing
terragruntOptions.ExcludeDirsviaWithExcludeDirsis exactly what’s needed so discovery can decide not to parse excluded components while still discovering them. Functionally this aligns with the PR’s intent.The later comment (“Exclude patterns are handled by the unit resolver to ensure proper reporting”) is now slightly misleading, since excludes are also honored earlier in discovery for parsing. Consider rewording it to clarify that:
- discovery uses
ExcludeDirsto skip parsing, and- the resolver still accounts for exclude patterns when building/reporting the final unit set.
Also applies to: 86-88
test/integration_stacks_test.go (1)
52-52: New exclude-template-module fixture and test are consistent with existing patternsThe new fixture constant and
TestStacksGenerateExcludeModuleWithStackVariablesfollow the existing testing conventions (cleanup,CopyEnvironment,JoinPath,RunTerragruntCommandWithOutput, and stderr assertions) and clearly exercise the--queue-exclude-dir '**/module'behavior. The test validates both that:
- the command no longer fails when template modules reference stack-only
values, and- the expected stack units for project A and B appear in the execution queue.
Two small, optional tighten-ups:
- The comment “Verify only the app unit is in the execution queue (not the module-based one)” doesn’t currently have a corresponding negative assertion. You could add an
assert.NotContainson whatever module path would appear in stderr to fully lock in the behavior.- If there’s a single “app unit” conceptually, consider rephrasing the comment to match the four asserted units, or make it explicit that you’re checking for “only stack-generated units (no module/* units)”.
Also applies to: 130-150
test/fixtures/stacks/exclude-template-module/live/project-A/module/terragrunt.hcl (1)
1-20: Dependency wiring looks good; minor comment typoThe Terragrunt dependency on Project B’s stack and the use of
values.envin bothconfig_pathandinputscorrectly model a template module that must not be parsed outside the stack context, which is exactly what this PR is testing.Only nit: the header comment has a typo (
dpeendency→dependency). If you touch this file again, it’s worth fixing for clarity:# Project A dependency on Project B using values
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/discovery/discovery.go(2 hunks)internal/runner/runnerpool/builder.go(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/module/main.tf(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/module/terragrunt.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-A/terragrunt.stack.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/module/main.tf(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl(1 hunks)test/fixtures/stacks/exclude-template-module/live/project-B/terragrunt.stack.hcl(1 hunks)test/integration_stacks_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/fixtures/stacks/exclude-template-module/live/project-B/terragrunt.stack.hcl
- test/fixtures/stacks/exclude-template-module/live/project-A/terragrunt.stack.hcl
- internal/discovery/discovery.go
- test/fixtures/stacks/exclude-template-module/live/project-B/module/main.tf
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/runner/runnerpool/builder.gotest/integration_stacks_test.go
🧠 Learnings (2)
📚 Learning: 2025-02-10T23:20:04.295Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
Applied to files:
test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl
📚 Learning: 2025-11-03T04:40:01.000Z
Learnt from: ThisGuyCodes
Repo: gruntwork-io/terragrunt PR: 5041
File: test/fixtures/hclvalidate/valid/duplicate-attributes-in-required-providers/main.tf:2-7
Timestamp: 2025-11-03T04:40:01.000Z
Learning: In the terragrunt repository, test fixtures under test/fixtures/hclvalidate/valid/ are intentionally testing that INPUT validation succeeds even when Terraform code contains syntax errors or other issues unrelated to input validation (e.g., duplicate attributes, circular references, invalid locals). The "valid" designation means "valid for input validation purposes" not "syntactically valid Terraform/OpenTofu code".
Applied to files:
test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcltest/integration_stacks_test.go
🧬 Code graph analysis (1)
test/integration_stacks_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(882-889)CopyEnvironment(89-105)RunTerragruntCommandWithOutput(1007-1011)util/file.go (1)
JoinPath(626-628)
🔇 Additional comments (2)
test/fixtures/stacks/exclude-template-module/live/project-B/module/terragrunt.hcl (1)
1-9: Fixture correctly exercises values-only-in-stack contextThis module intentionally references
values.envso that standalone parsing would fail, which is exactly what the exclude-dir behavior is meant to protect against. The comments clearly document this; no changes needed.test/fixtures/stacks/exclude-template-module/live/project-A/module/main.tf (1)
1-7: Terraform wiring forenvironmentis straightforward and consistentThe variable and output definitions are minimal and correct for passing
environmentthrough this module in the stack fixtures.
|
Failed on codespell |
atmask
force-pushed
the
issue-5124-do-not-parse-excluded
branch
from
c39f4a7 to
49aaa62
Compare
Thanks, fixed that typo and a misleading comment in the latest push. |
|
@denis256 any chance I get another look at this when you're able? |
2 participants
Description
Fixes #5124 .
The
--queue-exclude-dirflag was not preventing the parsing of excluded directories during dependency discovery. This caused parsing errors when using Terragrunt stacks with template modules whoseterragrunt.hclfiles reference stack-specific values (e.g.,${values.env},${values.project}).Why this was an issue:
Solution
Commit
f3f7dd7: Core ImplementationshouldSkipParsing()method to check if a component matches exclude patternsparseConcurrently()to skip parsing for components that match--queue-exclude-dirpatternsCommit
7f08f13: Test CoverageTestStacksGenerateExcludeModuleWithStackVariablesintegration test${values.env})--queue-exclude-dir '**/module'in tandem with the fix prevents parsing errors while still processing generated stack unitsTesting
Run the new integration test:
The test verifies that:
--queue-exclude-dirflag works as expected in stack workflowsTODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Discovery phase no longer parses directories matched by
--queue-include-dirglob mitigating failures from the use of values referenced in generated stacksSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.