chore: Remove RunTarget from options
#5264
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors command execution to a multi-stage preparation pipeline (PrepareConfig → PrepareSource → PrepareGenerate → PrepareInit/PrepareInputsAsEnvVars), moves run invocation behind a global function reference ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Logger
participant Prepare as PrepareConfig
participant Source as PrepareSource
participant Gen as PrepareGenerate
participant Init as PrepareInit/Inputs
participant Runner as runfn.Run
CLI->>Logger: create logger
CLI->>Prepare: PrepareConfig(ctx, logger, opts)
Prepare-->>CLI: PreparedConfig (updated opts, cfg, logger)
CLI->>Source: PrepareSource(ctx, logger, updatedOpts, cfg, report)
Source-->>CLI: updatedOpts (with source downloaded or resolved)
CLI->>Gen: PrepareGenerate(logger, updatedOpts, cfg)
Gen-->>CLI: generation complete
alt download-dir / needs init
CLI->>Init: PrepareInit(ctx, logger, originalOpts, updatedOpts, cfg, report)
else no init
CLI->>Init: PrepareInputsAsEnvVars(logger, updatedOpts, cfg)
end
CLI->>Runner: runfn.Run(ctx, logger, finalOpts, report)
Runner-->>CLI: execution result / outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)cli/commands/exec/exec.go (5)
⏰ 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). (3)
🔇 Additional comments (2)
Comment |
yhakbar
force-pushed
the
chore/undo-all-and-graph-wrap
branch
12 times, most recently
from
c723bdd to
35f3968
Compare
yhakbar
force-pushed
the
chore/remove-run-terragrunt
branch
from
afbd427 to
0a07289
Compare
yhakbar
force-pushed
the
chore/remove-run-terragrunt
branch
from
0a07289 to
cbddaf9
Compare
![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
🤖 Fix all issues with AI agents
In @cli/commands/exec/exec.go:
- Line 22: You create two separate report instances: one at report.NewReport()
used in PrepareSource and PrepareInit and another new report passed into
RunActionWithHooks, which causes prep telemetry to be lost; fix by reusing the
same report instance (r) through the call chain — modify Run so it passes the
existing r into runTargetCommand, update runTargetCommand’s signature to accept
a *report.Report (or the concrete type used) and have it forward that same r
into RunActionWithHooks and any other call sites like PrepareSource/PrepareInit
so all stages share the single report instance.
🧹 Nitpick comments (4)
cli/commands/info/print/print.go (1)
34-59: Graceful degradation pattern looks intentional but has a subtle inconsistency.The approach of printing available info even when preparation fails is reasonable for an info/debug command. However, there's an inconsistency in the second error path (lines 48-57):
- When
PrepareSourcefails, the code uses the original loggerland originalopts- At this point,
prepared.Loggeris available and could provide richer contextConsider using
prepared.Loggerin the second error block for consistency:♻️ Suggested improvement
// Download source updatedOpts, err := run.PrepareSource(ctx, prepared.Logger, prepared.UpdatedOpts, prepared.TerragruntConfig, report.NewReport()) if err != nil { // Even on error, try to print what info we have - l.Debugf("Fetching info with error: %v", err) + prepared.Logger.Debugf("Fetching info with error: %v", err) - if printErr := printTerragruntContext(l, opts); printErr != nil { - l.Errorf("Error printing info: %v", printErr) + if printErr := printTerragruntContext(prepared.Logger, prepared.UpdatedOpts); printErr != nil { + prepared.Logger.Errorf("Error printing info: %v", printErr) } return nil }internal/runner/run/prepare.go (2)
147-161: Documentation and implementation inconsistency.The doc comment states "requires PrepareInputsAsEnvVars to have been called first", but
PrepareInitthen duplicates bothCheckFolderContainsTerraformCodeandSetTerragruntInputsAsEnvVarscalls. Either:
- Remove the duplicate calls if callers are expected to call
PrepareInputsAsEnvVarsfirst, or- Update the doc comment to indicate this is a standalone function that doesn't require prior preparation
♻️ Option 1: Remove duplication (if prior call is expected)
// PrepareInit runs terraform init if needed. This is the final preparation stage. // It requires PrepareInputsAsEnvVars to have been called first. func PrepareInit(ctx context.Context, l log.Logger, originalOpts, opts *options.TerragruntOptions, cfg *config.TerragruntConfig, r *report.Report) error { - // Check for terraform code - if err := CheckFolderContainsTerraformCode(opts); err != nil { - return err - } - - if err := SetTerragruntInputsAsEnvVars(l, opts, cfg); err != nil { - return err - } - // Run terraform init via the non-init command preparation path return prepareNonInitCommand(ctx, l, originalOpts, opts, cfg, r) }
16-21: Minor:UpdatedOptsfield name may be misleading.In
PrepareConfig, theoptsparameter is returned asUpdatedOptswithout any modifications. The logger is updated (viaCheckVersionConstraints), butoptsis passed through unchanged. Consider renaming toOptsfor clarity, or document what updates are expected in future iterations.internal/runner/run/target.go (1)
11-21: Consider reordering type definition before const block.The
targetPointTypetype is defined at line 21, after the const block that uses it (lines 13-18). While valid Go (package-level declarations can reference each other in any order), placing the type definition before its usage improves readability.Suggested reordering
+type targetPointType byte + // targetPointType represents stages in the OpenTofu/Terraform preparation pipeline. // These are used internally by the run() function for flow control. const ( targetPointParseConfig targetPointType = iota + 1 targetPointDownloadSource targetPointGenerateConfig targetPointSetInputsAsEnvVars targetPointInitCommand ) - -type targetPointType byte
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/commands/aws-provider-patch/aws-provider-patch.gocli/commands/commands.gocli/commands/exec/exec.gocli/commands/hcl/validate/validate.gocli/commands/info/print/print.gocli/commands/render/render.goconfig/dependency.gointernal/runner/common/unit_runner.gointernal/runner/run/prepare.gointernal/runner/run/run.gointernal/runner/run/target.gointernal/runner/runfn/runfn.gooptions/options.go
🧰 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:
options/options.gocli/commands/aws-provider-patch/aws-provider-patch.gointernal/runner/common/unit_runner.gocli/commands/info/print/print.goconfig/dependency.gocli/commands/exec/exec.gocli/commands/hcl/validate/validate.gointernal/runner/runfn/runfn.gocli/commands/render/render.gointernal/runner/run/prepare.gocli/commands/commands.gointernal/runner/run/target.gointernal/runner/run/run.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:
options/options.gointernal/runner/common/unit_runner.gocli/commands/info/print/print.goconfig/dependency.gointernal/runner/runfn/runfn.gocli/commands/render/render.gocli/commands/commands.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:
options/options.gocli/commands/render/render.gocli/commands/commands.go
📚 Learning: 2025-09-10T04:41:35.652Z
Learnt from: jorhett
Repo: gruntwork-io/terragrunt PR: 1234
File: errors/multierror.go:35-39
Timestamp: 2025-09-10T04:41:35.652Z
Learning: In Terragrunt's MultiError.Error() method, checking for the exact string "exit status 2" and returning it directly is not a brittle hack but a semantic fix. Terraform's --detailed-exitcode flag uses exit code 2 to mean "plan succeeded with changes" (not an error), so when multiple modules return this status, it should not be wrapped in "Hit multiple errors:" formatting as that misrepresents successful operations as errors.
Applied to files:
internal/runner/common/unit_runner.go
📚 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:
config/dependency.gocli/commands/render/render.gocli/commands/commands.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:
cli/commands/hcl/validate/validate.go
🧬 Code graph analysis (9)
internal/runner/common/unit_runner.go (3)
internal/runner/runfn/runfn.go (1)
Run(17-17)internal/runner/run/run.go (1)
Run(88-94)internal/report/report.go (1)
Run(26-35)
config/dependency.go (2)
internal/runner/runfn/runfn.go (1)
Run(17-17)internal/runner/run/run.go (1)
Run(88-94)
cli/commands/exec/exec.go (3)
internal/runner/run/prepare.go (5)
PrepareConfig(25-48)PrepareSource(52-128)PrepareGenerate(132-134)PrepareInit(149-161)PrepareInputsAsEnvVars(138-145)internal/report/report.go (1)
NewReport(72-79)internal/runner/run/run.go (1)
RunActionWithHooks(430-449)
cli/commands/hcl/validate/validate.go (3)
internal/runner/run/prepare.go (3)
PrepareConfig(25-48)PrepareSource(52-128)PrepareGenerate(132-134)config/config.go (1)
TerragruntConfig(140-166)internal/report/report.go (1)
NewReport(72-79)
internal/runner/runfn/runfn.go (2)
internal/cli/context.go (1)
Context(4-10)options/options.go (1)
TerragruntOptions(101-319)
cli/commands/render/render.go (5)
internal/runner/run/prepare.go (1)
PrepareConfig(25-48)config/config.go (1)
TerragruntConfig(140-166)cli/commands/render/options.go (2)
Options(16-34)FormatJSON(13-13)cli/commands/commands.go (1)
New(62-124)internal/report/report.go (2)
Format(47-47)FormatJSON(51-51)
internal/runner/run/prepare.go (5)
config/config.go (3)
TerragruntConfig(140-166)ReadTerragruntConfig(1139-1147)DefaultParserOptions(98-126)options/options.go (5)
TerragruntOptions(101-319)EngineOptions(595-600)ErrorsConfig(603-606)IAMRoleOptions(339-344)DefaultWorkingAndDownloadDirs(418-427)internal/runner/run/creds/getter.go (1)
NewGetter(17-21)internal/runner/run/version_check.go (1)
CheckVersionConstraints(40-88)internal/runner/run/run.go (4)
CommandNameTerragruntReadConfig(45-45)GenerateConfig(237-264)CheckFolderContainsTerraformCode(506-517)SetTerragruntInputsAsEnvVars(453-471)
cli/commands/commands.go (1)
internal/runner/run/run.go (1)
Run(88-94)
internal/runner/run/target.go (3)
internal/cli/context.go (1)
Context(4-10)options/options.go (1)
TerragruntOptions(101-319)config/config.go (1)
TerragruntConfig(140-166)
⏰ 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). (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (24)
cli/commands/aws-provider-patch/aws-provider-patch.go (2)
100-100: Signature simplification is appropriate.Removing
ctxandcfgparameters fromrunAwsProviderPatchis correct since the function only performs file patching and doesn't need the context or config after preparation is complete.
36-56: LGTM - Clean migration to preparation-based workflow.The refactored flow correctly sequences the preparation steps: config parsing → source download → code generation → terraform init. This aligns well with the new architecture.
The parameter order on line 52 is correct:
PrepareInitexpects both the originaloptsandupdatedOpts, where the original options are passed as the third parameter and the updated options as the fourth. This differs intentionally fromPrepareGenerate, which only usesupdatedOpts.cli/commands/hcl/validate/validate.go (2)
298-319: LGTM - Consistent preparation workflow.The refactored validation flow correctly follows the same pattern as other commands:
PrepareConfigfor config parsingPrepareSourcefor source downloadPrepareGeneratefor code generationrunValidateInputsfor actual validationError handling appropriately continues to the next component on failure, which is the expected behavior for a validate-all operation.
329-329: Signature simplification is correct.Removing the
ctxparameter fromrunValidateInputsis appropriate since the function performs synchronous input validation and doesn't need the context.options/options.go (1)
374-398: LGTM - Clean removal of RunTerragrunt initialization.The
NewTerragruntOptionsWithWritersfunction has been cleaned up with the removal of theRunTerragruntfield initialization. The remaining fields maintain consistent formatting.internal/runner/runfn/runfn.go (1)
1-17: Pattern effectively breaks import cycles and is initialization-safe.This is a valid approach to breaking import cycles. The global
Runvariable is safely initialized before any usage because all commands are wrapped throughWrapWithTelemetry()incli/app.go, which callsinitialSetup()to setrunfn.Runbefore executing any command action. No nil-panic risk exists.The optional defensive implementation suggestion is reasonable for additional code clarity but is not necessary to address a safety concern.
cli/commands/commands.go (1)
40-40: LGTM - Clean migration to function variable pattern.The import of
runfnand the assignmentrunfn.Run = run.Runcorrectly implements the dependency injection pattern, replacing the previousopts.RunTerragruntfield assignment. This allows the run function to be called from other packages without direct coupling to the run package.Also applies to: 406-406
config/dependency.go (1)
23-23: LGTM - Correct migration to runfn.Run.The import and call site update correctly replace the previous
RunTerragruntmethod invocation withrunfn.Run. The function signature matches, and creating a new report viareport.NewReport()is appropriate for this isolated JSON output retrieval operation.Also applies to: 1182-1182
internal/runner/common/unit_runner.go (1)
11-11: LGTM - Correct migration of both call sites.Both usages of
runfn.Runare correctly implemented:
- Line 81: Uses the passed-in report
rfor proper tracking of the main unit execution- Line 154: Correctly uses an ad-hoc report to isolate the JSON output operation from the main report
The import and function signatures are correct.
Also applies to: 81-81, 154-154
cli/commands/render/render.go (4)
35-40: LGTM - Clean migration to PrepareConfig workflow.The Run function now correctly uses
run.PrepareConfigto obtain the prepared configuration and logger, then delegates torunRender. This aligns with the broader refactoring pattern introduced in this PR.
66-72: LGTM - Proper error handling in runAll.The per-unit preparation correctly handles errors by appending to the error slice and continuing to process remaining units, which is the expected behavior for batch operations. The FailFast check only applies to render errors, not preparation errors.
95-108: LGTM - Clean runRender implementation.The new
runRenderfunction provides a clear dispatch based on format with proper nil-check and descriptive error message. The switch statement handles all defined formats with a default error case.
215-215: Minor style alignment.The change from
:=to=reuses the existingerrvariable from line 209, which is a valid pattern and aligns with the codebase style of not shadowing error variables within the same function scope.internal/runner/run/prepare.go (2)
38-38: No issues found. The call toconfig.DefaultParserOptions(l, opts)is correct.DefaultParserOptionsis defined in theconfigpackage (config/config.go) and the import statement inprepare.gocorrectly imports"github.com/gruntwork-io/terragrunt/config". The code will compile and run without errors.
75-79: No action needed - the code is safe.The
GetAfterHooks(),GetBeforeHooks(), andGetErrorHooks()methods inTerraformConfigalready include nil receiver checks. These methods are defined to return an empty slice if the receiver is nil, so calling them on a potentially nilcfg.Terraformis safe and does not require an additional guard clause.Likely an incorrect or invalid review comment.
cli/commands/exec/exec.go (3)
52-70: LGTM!The
runTargetCommandhelper is well-structured: it cleanly extracts command context, conditionally sets the working directory, and properly wraps execution with hooks. The error message includes the directory context for easier debugging.
17-33: LGTM!The staged preparation flow (
PrepareConfig→PrepareSource→PrepareGenerate) is well-structured with consistent error propagation and proper logger handling throughout the pipeline.
40-47: SetAutoInitonupdatedOptsinstead of the originalopts.The modification at line 42 sets
opts.AutoInit = false, butupdatedOpts(which is passed torunTargetCommandat line 49) may be a separate clone created bydownloadTerraformSourceviaCloneWithConfigPath. If the intent is to disable auto-init, the flag should be set onupdatedOptsto ensure consistency. Additionally, note that theexeccommand does not callPrepareInit, so this flag may not have any actual effect in this code path.internal/runner/run/run.go (4)
93-94: LGTM!The
Runfunction now correctly instantiates the internaltargettype, aligning with the refactoring to make the target mechanism unexported.
639-643: LGTM!Passing
nilfor the target parameter during auto-init is correct. Thetargetmethods (isPoint,runCallback,runErrorCallback) safely handle nil receivers, and this ensures no target callbacks are triggered during the init subprocess.
271-279: LGTM!The function signature update to use
*targetand the parameter rename totis consistent with the internal type refactoring. The flow control checkpoints remain at the correct logical points in the preparation pipeline.
96-119: LGTM!The internal
runfunction correctly uses the privatetargettype. Error handling properly invokestarget.runErrorCallbackfor version constraint and config parsing errors before the first checkpoint.internal/runner/run/target.go (2)
27-33: LGTM!The internal
targetstruct is well-designed with clear field names and appropriate unexported visibility, aligning with the PR's goal of making the target mechanism internal.
35-57: LGTM!All three methods implement proper nil receiver handling:
isPointsafely returnsfalsefor nil targetsrunCallbackreturnsnil(no-op) when target or callback is nilrunErrorCallbackpreserves and returns the original error when no error callback is configuredThis design enables safe optional usage patterns throughout the codebase.
Description
Removes the
RunTargetlambda function inoptions.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.