bug: runner pool external dependencies inclusion fix#5199
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR fixes issue Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner Pool
participant Builder as Builder/Helpers
participant Discovery as Discovery
participant DepDisc as DependencyDiscovery
participant Report as Report
Runner->>Builder: Build with Options
Builder->>Builder: Extract Report from Options
Builder->>Discovery: Create Discovery
Builder->>Discovery: Attach Report via WithReport()
Runner->>Discovery: Discover Components
Discovery->>DepDisc: Create with Report
Discovery->>DepDisc: Attach Report via WithReport()
DepDisc->>DepDisc: Check if External Dependency<br/>(excluded by default)
alt External Dependency Found
DepDisc->>Report: Log Exclusion
DepDisc->>Report: Record in Report<br/>(Excluded result)
end
Discovery-->>Runner: Return Components<br/>(externals excluded)
Report-->>Runner: Exclusion Summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
| EnvVars: tgPrefix.EnvVars(QueueExcludeExternalFlagName), | ||
| Destination: &opts.IgnoreExternalDependencies, | ||
| Usage: "Ignore external dependencies for --all commands.", | ||
| Usage: "DEPRECATED: External dependencies are now excluded by default. Use --queue-include-external to include them.", |
There was a problem hiding this comment.
Let's just add a strict control as the Action for the flag, and hide the flag.
There was a problem hiding this comment.
I did that here, which might help with the language for this:
#5189
There was a problem hiding this comment.
Probably want to get rid of this.
There was a problem hiding this comment.
Probably want to get rid of this.
|
Thanks for addressing this! I’ve checked the description and it seems to solve the issue I raised. |
denis256
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)
cli/flags/shared/shared.go (1)
10-10: Hidden flag + strict control action for--queue-exclude-externallook correct; optional exit-error wrappingThe flag is now hidden and its
Actionevaluates theQueueExcludeExternalstrict control only when the user explicitly sets it, which cleanly enforces deprecation while keeping existing env/flag plumbing.If you want consistency with other strict-control usages (e.g., places that wrap control failures in
cli.NewExitError), you could optionally wrap the returned error fromEvaluateincli.NewExitError(..., cli.ExitCodeGeneralError), but functionally this wiring is already sound.Also applies to: 111-122
internal/discovery/dependencydiscovery.go (1)
288-295: Consider handling potential errors from report operations.The return values from
EnsureRunandEndRunare discarded. While this may be intentional to avoid disrupting the discovery flow, logging or at least documenting why errors are ignored would improve debuggability.Additionally,
util.CleanPath(depPath)normalizes the path but doesn't guarantee an absolute path. IfdepPathis already absolute (which it should be fromextractDependencyPaths), this should work correctly, but the variable nameabsPathmight be misleading.l.Debugf("Excluded external dependency: %s", depComponent.DisplayPath()) // Record in report as excluded external dependency if dd.report != nil { - absPath := util.CleanPath(depPath) - run, _ := dd.report.EnsureRun(absPath) - _ = dd.report.EndRun(run.Path, report.WithResult(report.ResultExcluded), report.WithReason(report.ReasonExcludeExternal)) + cleanPath := util.CleanPath(depPath) + run, err := dd.report.EnsureRun(cleanPath) + if err != nil { + l.Debugf("Failed to ensure run for excluded dependency %s: %v", cleanPath, err) + } else if endErr := dd.report.EndRun(run.Path, report.WithResult(report.ResultExcluded), report.WithReason(report.ReasonExcludeExternal)); endErr != nil { + l.Debugf("Failed to end run for excluded dependency %s: %v", cleanPath, endErr) + } }test/integration_regressions_test.go (1)
653-668: Consider checking or documenting the error discard.Line 659 discards the error from
RunTerragruntCommandWithOutput. While this may be intentional (the test focuses on verifying discovery behavior regardless of execution success), explicitly handling or documenting this would improve clarity.// Run terragrunt run --all plan from the parent directory (live/) _, stderr, err := helpers.RunTerragruntCommandWithOutput( t, "terragrunt run --all plan --non-interactive --working-dir "+rootPath, ) - // The command should succeed in terms of discovery - _ = err + // The command may fail due to missing Terraform configuration, but we only care about discovery + // behavior in this test, so we intentionally ignore the error. + require.NoError(t, err, "run --all plan should succeed in discovering all modules")Alternatively, if the command is expected to fail:
- // The command should succeed in terms of discovery - _ = err + // We only care about discovery behavior, not execution success + // (some modules may not have valid Terraform configuration)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cli/flags/shared/shared.go(2 hunks)docs-starlight/src/content/docs/04-reference/03-strict-controls.mdx(2 hunks)docs-starlight/src/data/flags/queue-exclude-external.mdx(1 hunks)docs-starlight/src/data/flags/queue-include-external.mdx(1 hunks)internal/discovery/dependencydiscovery.go(6 hunks)internal/discovery/discovery.go(4 hunks)internal/runner/common/options.go(1 hunks)internal/runner/runnerpool/builder.go(0 hunks)internal/runner/runnerpool/builder_helpers.go(5 hunks)internal/runner/runnerpool/runner.go(1 hunks)internal/runner/runnerpool/runner_test.go(1 hunks)internal/strict/controls/controls.go(2 hunks)test/fixtures/regressions/5195-scope-escape/bastion/terragrunt.hcl(1 hunks)test/fixtures/regressions/5195-scope-escape/module1/terragrunt.hcl(1 hunks)test/fixtures/regressions/5195-scope-escape/module2/terragrunt.hcl(1 hunks)test/integration_dag_test.go(0 hunks)test/integration_regressions_test.go(4 hunks)test/integration_report_test.go(2 hunks)
💤 Files with no reviewable changes (2)
- test/integration_dag_test.go
- internal/runner/runnerpool/builder.go
🧰 Additional context used
📓 Path-based instructions (2)
docs-starlight/**/*.md*
⚙️ CodeRabbit configuration file
Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation in
docsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
Files:
docs-starlight/src/content/docs/04-reference/03-strict-controls.mdxdocs-starlight/src/data/flags/queue-exclude-external.mdxdocs-starlight/src/data/flags/queue-include-external.mdx
**/*.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:
test/integration_report_test.gointernal/strict/controls/controls.gointernal/runner/common/options.gointernal/runner/runnerpool/runner_test.gointernal/discovery/dependencydiscovery.gointernal/runner/runnerpool/runner.gointernal/discovery/discovery.gointernal/runner/runnerpool/builder_helpers.gocli/flags/shared/shared.gotest/integration_regressions_test.go
🧠 Learnings (5)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
test/integration_report_test.gointernal/discovery/dependencydiscovery.gointernal/discovery/discovery.gointernal/runner/runnerpool/builder_helpers.gocli/flags/shared/shared.gotest/integration_regressions_test.go
📚 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/regressions/5195-scope-escape/bastion/terragrunt.hcltest/fixtures/regressions/5195-scope-escape/module2/terragrunt.hcl
📚 Learning: 2025-02-10T23:20:04.295Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 3868
File: docs-starlight/patches/@astrojs%2Fstarlight@0.31.1.patch: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/regressions/5195-scope-escape/module2/terragrunt.hcl
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
test/fixtures/regressions/5195-scope-escape/module1/terragrunt.hclinternal/discovery/dependencydiscovery.gointernal/discovery/discovery.gointernal/runner/runnerpool/builder_helpers.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
cli/flags/shared/shared.go
🧬 Code graph analysis (7)
internal/strict/controls/controls.go (2)
internal/strict/controls/control.go (1)
Control(16-47)internal/strict/control.go (1)
Control(22-55)
internal/runner/common/options.go (2)
internal/experiment/experiment.go (1)
Report(27-27)internal/runner/common/runner.go (1)
StackRunner(15-28)
internal/runner/runnerpool/runner_test.go (1)
internal/runner/runnerpool/runner.go (1)
NewRunnerPoolStack(211-329)
internal/discovery/discovery.go (2)
internal/experiment/experiment.go (1)
Report(27-27)internal/runner/common/options.go (1)
WithReport(69-71)
internal/runner/runnerpool/builder_helpers.go (1)
internal/runner/common/options.go (3)
Option(10-12)ReportProvider(51-53)WithReport(69-71)
cli/flags/shared/shared.go (2)
internal/strict/controls/controls.go (1)
QueueExcludeExternal(60-60)internal/filter/evaluator.go (1)
Evaluate(40-61)
test/integration_regressions_test.go (3)
test/helpers/package.go (3)
CleanupTerraformFolder(882-889)CopyEnvironment(89-105)RunTerragruntCommandWithOutput(1007-1011)util/file.go (1)
JoinPath(626-628)test/helpers/test_helpers.go (1)
CreateGitRepo(65-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration_tests / Test (Experiment mode)
- GitHub Check: integration_tests / Test (Experiment mode)
- GitHub Check: integration_tests / Test (Experiment mode)
- GitHub Check: integration_tests / Test (Experiment mode)
- GitHub Check: integration_tests / Test (Experiment mode)
- GitHub Check: integration_tests / Test (Experiment mode)
🔇 Additional comments (21)
test/fixtures/regressions/5195-scope-escape/module1/terragrunt.hcl (1)
1-4: Fixture correctly models “reverse” external dependencyThe dependency on
../bastionplus the comment about not discovering this unit from bastion matches the regression scenario and should give good coverage for scope-escape behavior.test/fixtures/regressions/5195-scope-escape/module2/terragrunt.hcl (1)
1-1: Clear description of module2’s role in the regression fixtureThe comment succinctly documents that this is a simple unit that must not be discovered from bastion, which matches the intended test scenario.
test/fixtures/regressions/5195-scope-escape/bastion/terragrunt.hcl (1)
1-9: Dependency and mock_outputs setup look correct for the regression testPointing
sharedat../module2and constrainingmock_outputs_allowed_terraform_commandsto["plan", "destroy"]keeps the fixture focused on queue/discovery behavior without requiring real Terraform state.docs-starlight/src/data/flags/queue-include-external.mdx (1)
9-13: Docs clearly explain default exclusion and when to use--queue-include-externalThe text accurately states that external dependencies are excluded from the queue by default and explains when enabling this flag is useful, in clear and concise language.
internal/runner/runnerpool/runner.go (1)
900-907: Nil-guard inWithOptionscorrectly prevents panics from nil optionsSkipping
nilentries in the options slice before callingApplyensures calls likeNewRunnerPoolStack(..., nil)won’t panic, while still applying all non-nil options. This is a safe and idiomatic defensive check.internal/strict/controls/controls.go (1)
59-61: Strict control forqueue-exclude-externalis wired consistentlyThe
QueueExcludeExternalconstant and correspondingControlentry follow the existing strict-control pattern: category is set, messaging clearly explains that the flag is deprecated/no longer supported, and the name matches what the CLI wiring uses.Also applies to: 215-221
internal/runner/runnerpool/runner_test.go (1)
38-38: Test change exercises nil-option path inNewRunnerPoolStackPassing
nilas the final variadic option ensures the new nil-handling inRunner.WithOptionsis covered in tests and won’t regress into a panic.internal/discovery/dependencydiscovery.go (1)
82-87: LGTM!The
WithReportmethod follows the established builder pattern used throughout this file and correctly attaches the report for recording excluded external dependencies.docs-starlight/src/content/docs/04-reference/03-strict-controls.mdx (2)
196-208: LGTM!The new
queue-exclude-externalstrict control documentation is clear, well-structured, and follows the established pattern of other strict controls in this file. The deprecation reason, example output, and migration guidance to--queue-include-externalare all helpful.
233-233: LGTM!The addition of
queue-exclude-externalto thedeprecated-flagscontrol category is consistent with the new strict control documentation above.test/integration_report_test.go (1)
512-531: LGTM!The test correctly verifies the new external dependency exclusion behavior by:
- Using
--log-level debugto capture exclusion messages- Asserting that "Excluded external dependency" appears in stderr
- Maintaining existing verifications for report file content
This provides good coverage for the regression fix.
docs-starlight/src/data/flags/queue-exclude-external.mdx (1)
9-19: LGTM!The deprecation documentation is clear and provides appropriate guidance:
- The
<Aside type="caution">component makes the deprecation notice prominent- Links to
--queue-include-externalfor users who need the old behavior- Explains what constitutes an external dependency
This aligns well with the strict controls documentation and the code changes.
test/integration_regressions_test.go (2)
601-637: LGTM! Comprehensive regression test for issue #5195.The test thoroughly validates the fix by:
- Setting up a proper git repository (important for scope detection)
- Running from a subdirectory to trigger the scope escape scenario
- Verifying external dependencies are logged as excluded
- Checking the report shows correct unit counts (2 units, 1 succeeded, 1 excluded)
The detailed docstring explaining the test structure and expected behavior is excellent for maintainability.
321-336: LGTM!The comment updates correctly document that
--queue-exclude-externalis now deprecated and acts as a no-op, while the test continues to verify backward compatibility.internal/runner/runnerpool/builder_helpers.go (3)
117-127: LGTM! Critical fix for issue #5195.The gating logic correctly restores pre-v0.94.0 behavior by making external dependencies discovery opt-in. The comment clearly explains:
- Why external dependencies are now excluded by default
- The reference to the issue being fixed (#5195)
- That external dependencies are still tracked for reporting purposes
This is the core fix that prevents the scope escape regression.
80-89: LGTM!The
extractReporthelper follows the established pattern ofextractWorktreesand correctly extracts the report from options using theReportProviderinterface.
178-181: LGTM!Report is correctly wired into discovery to enable recording of excluded external dependencies during the discovery phase.
internal/discovery/discovery.go (3)
114-116: LGTM!The new
reportfield is properly documented and follows the struct field pattern used throughout this file.
260-265: LGTM!The
WithReportmethod follows the established builder pattern and correctly sets the report field for propagation to dependency discovery.
1172-1175: LGTM!The report is correctly propagated to
DependencyDiscoveryviaWithReport, enabling the recording of excluded external dependencies during dependency discovery. This completes the report wiring from the runner pool through discovery to dependency discovery.internal/runner/common/options.go (1)
50-71: ReportProvider/reportOption design looks solid and preserves existing behaviorThe new
ReportProviderinterface andreportOptionimplementation integrate cleanly with the existingOptionpattern (similar toParseOptionsProvider).reportOption.Applysimply delegates tostack.SetReport, soWithReportpreserves prior behavior while now allowing downstream consumers to type‑assert options toReportProviderwhen they need access to the shared*report.Report. I don't see correctness or API‑stability concerns here.
thisguycodes
left a comment
thisguycodes
left a comment
There was a problem hiding this comment.
looks fine, but I really don't understand (or like) the nil option thing. It appears to do nothing, but I trust it does something or we wouldn't have added it. Seems like a footgun waiting to go off.
| if opt == nil { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
this feels so weird, we shouldn't be passing in nil options
| l := thlogger.CreateLogger() | ||
|
|
||
| runner, err := runnerpool.NewRunnerPoolStack(context.Background(), l, opts, discovered) | ||
| runner, err := runnerpool.NewRunnerPoolStack(context.Background(), l, opts, discovered, nil) |
There was a problem hiding this comment.
since the nil just executes continue, I don't understand why we need this here
4 participants
Description
Logic v0.93.x : discover all -> then filter out based on flags
Implemented logic in PR: only discover what you actually need
Fixes #5195.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Release Notes
Bug Fixes
--queue-include-externalflag to include external dependencies when necessary.Documentation
--queue-exclude-external.✏️ Tip: You can customize this high-level summary in your review settings.