feat: exec, run, info strict commands implementation, flags renaming.
#3723
Conversation
WalkthroughThe changes across the codebase reflect a significant refactoring of Terragrunt's command handling, particularly around how Terraform-related commands are processed. The most notable shifts include:
Changes
Assessment against linked issue #3757
Sequence DiagramsequenceDiagram
participant Terragrunt
participant TerraformCLI
Terragrunt->>TerraformCLI: Execute providers lock command
TerraformCLI-->>Terragrunt: Process command arguments
Terragrunt->>TerraformCLI: Pass platform arguments
Poem
Additional InsightsWhile the changes look promising, I recommend additional testing specifically around the The team has clearly put significant effort into improving command execution and reducing potential sources of confusion. The move from Would you like me to elaborate on any specific aspect of these changes? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
cli/commands/render-json/action.go (1)
108-111: Fix error handling in marshalCtyValueJSONWithoutType.The function always wraps the error in
errors.New()even when there is no error (when err is nil). This could lead to unexpected behavior.Here's the fix:
- return jsonBytes, errors.New(err) + if err != nil { + return nil, errors.New(err) + } + return jsonBytes, nil
🧹 Nitpick comments (126)
internal/cache/context.go (2)
7-11: Add documentation for the constants.While the naming is good, adding documentation comments would help other developers understand:
- The purpose of
RunCmdCacheContextKey- Why we need a separate cache name constant
- Any specific requirements or constraints
Add documentation like this:
const ( + // RunCmdCacheContextKey is the context key used to store and retrieve the run command cache RunCmdCacheContextKey ctxKey = iota + // runCmdCacheName is the identifier for the run command cache instance runCmdCacheName = "runCmdCache" )
13-13: Document the ctxKey type and consider type safety.The byte type is efficient, but let's make it more maintainable:
Consider these improvements:
+// ctxKey is a type-safe context key type to prevent key collisions type ctxKey byteYou might also want to consider using an unexported struct{} as the key type for even better type safety:
type ctxKey struct{}internal/strict/view/plaintext/render.go (3)
21-31: Consider adding more context to errorsThe methods look clean and focused! One small suggestion: consider wrapping the errors with some context about what failed (list vs detail rendering).
Here's how you might enhance the error handling:
func (render *Render) List(controls strict.Controls) (string, error) { - return render.executeTemplate(listTemplate, map[string]any{ + result, err := render.executeTemplate(listTemplate, map[string]any{ "controls": controls, }, nil) + if err != nil { + return "", errors.New(err).WithMessage("failed to render controls list") + } + return result, nil }
56-58: Consider making tabwriter config values constantsThe magic numbers for tabwriter configuration would be more maintainable as named constants at the package level.
Here's a suggested improvement:
+const ( + tabMinWidth = 1 + tabWidth = 8 + tabPadding = 2 +) + - var minwidth, tabwidth, padding = 1, 8, 2 - tabOut := tabwriter.NewWriter(out, minwidth, tabwidth, padding, ' ', 0) + tabOut := tabwriter.NewWriter(out, tabMinWidth, tabWidth, tabPadding, ' ', 0)
33-69: Consider breaking down the template execution logicThe
executeTemplatemethod handles quite a few responsibilities. Consider splitting it into smaller, more focused functions for better testability and maintainability.Here's a suggested refactor:
+func (render *Render) buildTemplate(templ string, customFuncs map[string]any) (*template.Template, error) { + funcMap := template.FuncMap{} + for key, value := range customFuncs { + funcMap[key] = value + } + + t := template.Must(template.New("template").Funcs(funcMap).Parse(templ)) + templates := map[string]string{ + "subcontrolTemplate": subcontrolTemplate, + "controlTemplate": controlTemplate, + "rangeSubcontrolsTemplate": rangeSubcontrolsTemplate, + "rangeControlsTemplate": rangeControlsTemplate, + } + + for name, value := range templates { + if _, err := t.New(name).Parse(value); err != nil { + return nil, errors.New(err).WithMessage("failed to parse template %s", name) + } + } + return t, nil +} + +func (render *Render) formatOutput(t *template.Template, data any) (string, error) { + out := new(bytes.Buffer) + tabOut := tabwriter.NewWriter(out, tabMinWidth, tabWidth, tabPadding, ' ', 0) + + if err := t.Execute(tabOut, data); err != nil { + return "", errors.New(err).WithMessage("failed to execute template") + } + + if err := tabOut.Flush(); err != nil { + return "", errors.New(err).WithMessage("failed to flush output") + } + + return out.String(), nil +} + func (render *Render) executeTemplate(templ string, data any, customFuncs map[string]any) (string, error) { - funcMap := template.FuncMap{} - // ... existing code ... + t, err := render.buildTemplate(templ, customFuncs) + if err != nil { + return "", err + } + return render.formatOutput(t, data) }docs/_docs/04_reference/05-strict-mode.md (2)
62-64: Clarity Improvement for Strict-Control Explanation
The sentence “flag to a value to a particular strict control” could be made a bit clearer. Consider rephrasing it—for example: “...by setting the strict-control flag to enable a specific control.”
108-109: Introduction to Control Categories
The new text directing users to the control categories is a welcome addition. As a minor style note (in line with the static analysis hint), consider varying the start of successive sentences to improve readability.internal/strict/strict.go (1)
2-2: Add a direct link to relevant docs for enabling strict mode
It’s great to note that strict mode starts disabled. If there’s a handy doc page with examples on turning it on, a reference here could really help folks find it quickly.cli/flags/flag.go (6)
1-2: Consider adding usage examples in the package comment.
This package-level comment nicely introduces the purpose of these flags, but quick examples could help future developers adopt this faster.
13-18: Potential naming conflict with embedded Flag.
Defining a type named “Flag” while embedding thecli.Flaginterface may lead to confusion. It might help to rename your new struct to something more descriptive (e.g., “FlagWrapper”) to reduce ambiguity.
19-30: Renaming local variable to avoid overshadowing.
The local variable "flag := &Flag{…}" might be overshadowing the Go built-in "flag" package. Consider using a different name (e.g., “customFlag” or “wrappedFlag”) for clarity.
36-46: Clarify logic in TakesValue.
The check for a boolean type to return false is clever, but a quick comment explaining the rationale (i.e., “Booleans don’t accept extra values”) may reduce confusion for new contributors.
47-57: Revisit ignoring the error in Set.
Line 52 ignores the error returned by Set. If a malformed value is provided by the deprecated flags, skipping the error could obscure root causes. Consider logging or returning this error to the caller.- new.Flag.Value().Set(newValue) //nolint:errcheck + if err := new.Flag.Value().Set(newValue); err != nil { + // Log or return the error + // e.g., return err + }
82-107: Watch out for concurrency in RunAction.
Although the current approach calls Evaluate once per deprecated flag, be mindful if multiple goroutines might set or run actions on the same instance of Flag. If that scenario arises, you may need explicit locking. For now, this is well-structured.internal/strict/controls/depreacted_env_var.go (1)
1-1: Typo in filename.
The file is named “depreacted_env_var.go,” which appears to be misspelled. It might be more readable as “deprecated_env_var.go.”cli/commands/graph/command.go (1)
28-41: Prefix usage is flexible but could be documented better.
Providing a prefix for these flags is a neat approach. Adding a quick doc comment in NewFlags explaining how prefix interacts (or is ignored) might help future maintainers.cli/commands/deprecated.go (2)
39-39: Consider naming consistency for type.While the type name nicely describes its purpose, “replaceDeprecatedCommandFuncType” feels a bit unwieldy. A shorter alias might improve readability.
53-54: Centralized evaluation logic.Directly returning an error here simplifies flow control. Consider adding a bit of context in the error message, so users immediately know which command triggered the deprecation check.
internal/strict/controls/deprecated_flag_name.go (1)
75-116: Evaluate method thoroughly checks usage.It carefully decides when to warn vs. error. A small heads-up: if multiple new flags exist, only the first name is used. You may consider clarifying or supporting multiple synonyms in the future.
cli/commands/run-all/command.go (2)
22-28: Clarify naming for "out-dir" flags.Defining both new and deprecated constants for "out-dir" and "json-out-dir" might cause confusion. Make sure devs know which is the currently recommended flag versus deprecated ones. Possibly add a short inline comment indicating their recommended usage.
37-37: Potential readability improvement in flag assembly.Chaining flags in one line with append(...).Sort() is concise but can be a little dense. Consider splitting it or adding a comment for readability, though it’s not mandatory.
internal/experiment/experiment.go (4)
50-61: Nicely done: retrieving experiment names.The sorting step is a nice user-friendly touch. No major issues, though keep an eye on potential performance overhead if the list grows large. Probably negligible in typical usage.
87-92: Turning on experiment mode.Enabling only ongoing experiments is consistent with the approach. Keep in mind a scenario if devs want to forcibly enable completed experiments. Probably not relevant but worth clarifying in docs.
105-120: Completed experiments get a log warning.This is a nice heads-up for devs. Possibly consider logging at
InfoforWarnfconditionally if the feature is brand new. Some folks might see it as noise. No major issues though.
131-149: Good struct refactor.Storing experiment flags and statuses is more future-proof now. The Evaluate() method is straightforward. Just keep an eye on whether more advanced logic might be needed in the future if statuses become multi-state or if we track partial enablement.
shell/run_cmd.go (1)
21-28: Signal forwarding approach is thoughtful.Giving
terraformortofutime to exit gracefully is wise. Make sure the 15-second delay is widely accepted by the team or configurable, in case some folks consider it too long or too short.internal/cli/slice_flag.go (3)
34-41: Great struct extension for environment-based configuration.
Allowing multiple environment variables (EnvVars) plus an Action, Setter, and Destination significantly improves flexibility. Just ensure you document how multiple EnvVars are handled if they’re all set at once—especially which takes precedence.
133-133: Confirm usage of this variable for consistency.
You’re assigning a reference to a sliceValue, probably for reflection or type registration. That’s typically fine, but confirm it’s not just leftover code.
152-154: Reset method is helpful.
This is a nice approach if you need to re-parse flags, though do confirm that resetting the slice in a running environment won’t cause confusion.internal/cli/generic_flag.go (3)
30-35: Support for multiple EnvVars and a flexible Setter is intuitive.
This extension helps unify environment-based config. Make sure your docs clarify how multiple env variables are resolved, especially if more than one is present.
143-143: FlagVariable type registration.
Same pattern as in slice_flag. Typically harmless for reflection.
158-165: Robust value parsing logic.
Covers multiple data types with custom error messages. Great! Keep an eye on localization if you ever want to handle other languages.Also applies to: 206-216
cli/flags/deprecated_flag.go (2)
33-36: allowedSubcommandScope accessor.
If you plan to permit use in subcommands, maybe also mention the reason behind the design. Looks okay for now.
100-109: StrictControlsByCommand is a well-labeled function.
Using commandName to categorize controls is straightforward. Make sure controlNames covers all relevant variants.internal/cli/map_flag.go (3)
42-47: Add usage example or doc note for the 'Action' and 'Setter' fields.
This is a cool addition, but might feel a bit magical without examples. A short code snippet in the doc would help folks see how to define and pass in a custom setter or action.
49-50: Minor naming note on DisableEnvVar.
We have "EnvVars" as a slice and "DisableEnvVar" in singular. They still make sense, but consider naming it "DisableEnvVars" to emphasize it affects all environment variables.
197-198: Ensure errors from Setter are fully contextualized.
Right now, errors from the custom Setter are just returned directly. Consider wrapping them to clearly indicate which flag environment triggered them, especially if multiple flags share logic.internal/cloner/cloner.go (1)
36-44: Potential performance caution with reflection.
Deep cloning is powerful but can be slow if used frequently. Monitor usage in performance-critical loops. Optionally, mention these trade-offs in doc comments so users are aware.internal/cli/command.go (1)
155-202: Multi-pass parse logic
Your approach splits out undefined flags as you parse a second time with subcommand scope. This is clever, but it may be tricky for new contributors to follow. A short comment explaining the rationale can keep them from scratching their heads later.internal/cli/flag.go (2)
59-60: Correct the doc comment to match the function name.Here, the doc says “IsSet returns true if the flag was by CLI arg,” but the method is named “IsArgSet.” The mismatch could confuse readers.
Apply this diff to align the doc with the method:
- // IsSet returns true if the flag was by CLI arg. + // IsArgSet returns true if the flag was set by CLI arg. IsArgSet() bool
91-117: Clarify naming vs. type usage.The struct is named “flagValueGetter,” while the interface field is also named “flagValue,” and there’s a local variable named “flag.” This triple usage might cause confusion. Renaming one could improve readability.
internal/strict/control.go (1)
85-105: Consider using a map to optimize duplicate removal.The nested loop in RemoveDuplicates() leads to O(n²) complexity, which may degrade performance for large sets of controls.
Below is an example showing a map-based approach:
func (ctrls Controls) RemoveDuplicates() Controls { - var unique Controls - for _, ctrl := range ctrls { - skip := false - for _, uniqueCtrl := range unique { - if uniqueCtrl.GetName() == ctrl.GetName() && uniqueCtrl.GetCategory().Name == ctrl.GetCategory().Name { - skip = true - break - } - } - if !skip { - unique = append(unique, ctrl) - } - } - return unique + seen := make(map[string]bool) + var results Controls + for _, ctrl := range ctrls { + key := ctrl.GetName() + ":" + ctrl.GetCategory().Name + if !seen[key] { + seen[key] = true + results = append(results, ctrl) + } + } + return results }cli/app.go (1)
145-177: Minor naming fix for “globaFlags.”The variable name “globaFlags” (missing “l”) might cause confusion. Use “globalFlags” for clarity.
- globaFlags := global.NewFlags(opts, nil) + globalFlags := global.NewFlags(opts, nil)cli/provider_cache.go (1)
345-345: Redundant code path.
Consider refactoring if “cloneOpts” creation is repeated often across the codebase to simplify maintenance.cli/app_test.go (1)
81-82: Deprecated flag usage.
Consider removing or clearly labeling the old flags soon if they are truly deprecated.cli/commands/run/flags.go (3)
15-34: Consider organizing related flags into smaller, well-documented groups.Right now, there's a large block of constants representing various flags, many for niche scenarios. Grouping logically related flags together (e.g., states/config, concurrency/queue, provider cache, or IAM) and adding short doc comments at the group level could help maintain clarity and discoverability.
139-147: Ensure user clarity on under-development flags.The “--all” flag is marked “under development.” Returning a generic error message is fine for early prototypes, but consider offering a short explanation or instructions for next steps (e.g., how to get help or typical usage). This reduces user frustration and bug reports.
159-166: Include a quick note explaining environment variable resolution when handling config paths.It might be useful to mention environment variable resolution in the Usage comment for the “--config” flag—especially if Terragrunt supports specifying the config path via environment variables. Simple clarifications help new users avoid path resolution pitfalls.
options/options.go (1)
57-57: Make log level a configurable option at initialization.“DefaultLogLevel = log.InfoLevel” is a sensible default. Look out for opportunities to override it more conveniently when building new TerragruntOptions. This helps folks quickly switch to debug or error logging in custom integration scenarios.
test/integration_test.go (4)
119-161: Welcome addition of TestExecCommand.
- The table-driven approach is a neat way to test multiple argument combos.
- You might consider adding negative tests for missing or invalid arguments to confirm robust error handling.
1066-1069: Test case for invalid command 'paln'.This scenario is clear. Perhaps add a quick comment clarifying that 'paln' is an intentional typo for negative testing.
1077-1093: Looping through CLI args for test.This block neatly checks expected errors vs. actual errors. If time permits, consider adding a direct check ensuring user’s combined flags are valid.
1175-1192: New TestRunCommand method.
- Good demonstration of running Terragrunt with “run” vs. direct usage.
- The test especially verifies final outputs with “validateInputs,” ensuring correctness.
- Might consider including an error-handling test scenario for unexpected output or missing resources.
cli/commands/exec/options.go (1)
3-5: Add field documentation to clarify usage 📝The
InDownloadDirfield would benefit from a comment explaining when it should be true/false and its impact on command execution.type Options struct { + // InDownloadDir determines whether the command should execute in the download directory + // rather than the working directory. This affects environment variable handling. InDownloadDir bool }internal/cli/exit_code.go (1)
4-7: Consider adding more standard exit codes 🎯While the basic success/error codes are good, Unix tools typically use some standard exit codes. Consider adding more common ones:
const ( ExitCodeSuccess ExitCode = iota ExitCodeGeneralError + ExitCodeUsageError // Invalid command usage (2) + ExitCodeDataError // Input data error (65) + ExitCodeConfigError // Configuration error (78) )pkg/log/context.go (1)
15-23: Consider providing a default logger 🔍When no logger is found in context, returning nil might lead to nil pointer panics. Consider returning a default no-op logger instead:
+var defaultLogger = &noopLogger{} + +type noopLogger struct{} + +func (l *noopLogger) Debug(args ...interface{}) {} +// ... implement other Logger methods ... + func LoggerFromContext(ctx context.Context) Logger { if val := ctx.Value(loggerContextKey); val != nil { if val, ok := val.(Logger); ok { return val } } - return nil + return defaultLogger }internal/strict/errors.go (1)
14-16: Let's make this error message more user-friendly! 🎯The current error message is a bit terse. Consider making it more descriptive and helpful by including what the invalid control name was.
Here's a more informative version:
func (err InvalidControlNameError) Error() string { - return "allowed control(s): " + err.allowedNames.String() + return "Invalid control name provided. Allowed control(s) are: " + err.allowedNames.String() }internal/cli/funcs.go (1)
9-10: Good addition of the HelpFunc type! 🎯The new type follows the established pattern and makes help handling more organized. Consider adding a brief example in the comment to make it even more developer-friendly.
-// HelpFunc is the action to execute when help needs to be displayed. +// HelpFunc is the action to execute when help needs to be displayed. +// Example: +// func showHelp(ctx *Context) error { +// fmt.Println("Usage: ...") +// return nil +// }internal/strict/controls/deprecated_command.go (1)
14-22: Fix typos in error messagesThere's a small typo in both the error and warning messages - "commands is" should be "command is".
- Error: errors.Errorf("The `%s` commands is no longer supported. Use `%s` instead.", command, newCommand), - Warning: fmt.Sprintf("The `%s` commands is deprecated and will be removed in a future version. Use `%s` instead.", command, newCommand), + Error: errors.Errorf("The `%s` command is no longer supported. Use `%s` instead.", command, newCommand), + Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `%s` instead.", command, newCommand),tf/detailed_exitcode.go (1)
25-37: Consider enhancing the documentation for better clarityThe Set method's logic is clear, but the documentation could better explain the meaning of different exit codes and when they're used.
-// Set sets the newCode if the previous value is not 1 and the new value is greater than the previous one. +// Set updates the exit code following OpenTofu's exit code convention: +// - 0 = Success +// - 1 = Error +// - 2 = Success with changes pending +// The method only updates if: +// - The current code is not 1 (error state) +// - The new code is greater than current OR equals 1internal/strict/view/writer.go (2)
17-23: Consider adding more detailed documentationWhile the constructor is documented, it would be helpful to describe what
renderis and how it's used.Add something like:
-// NewWriter returns a new `Writer` instance. +// NewWriter returns a new Writer instance that uses the provided io.Writer for output +// and the Render interface for formatting controls.
45-51: Consider adding error contextThe error wrapping could be more descriptive to help with debugging.
- return errors.New(err) + return errors.New(fmt.Errorf("failed to write output: %w", err))cli/commands/run/command_test.go (1)
16-37: Consider adding more test casesThe test table covers error cases well, but could benefit from adding positive test cases where commands succeed.
Consider adding test cases like:
{ name: "valid terraform command", opts: &options.TerragruntOptions{ TerraformCommand: "plan", TerraformPath: "terraform", }, expectedErr: nil, },internal/experiment/errors.go (1)
7-20: Consider implementing error comparison methodsFor better error handling, consider implementing
Ismethod for error comparison.func (err InvalidExperimentNameError) Is(target error) bool { _, ok := target.(*InvalidExperimentNameError) return ok }configstack/log_test.go (1)
43-52: Consider extracting log parsing logicThe log parsing logic could be moved to a helper function for better readability and reuse.
func findLogEntry(output string, contains string) string { for _, line := range strings.Split(output, "\n") { if strings.Contains(line, contains) { return line } } return "" }cli/commands/exec/action.go (1)
38-42: Add error context for better debuggingWhen the command fails, it might be helpful to include the working directory in the error message to help users understand where the command was executed.
return run.RunActionWithHooks(ctx, command, opts, cfg, func(ctx context.Context) error { - _, err := shell.RunCommandWithOutput(ctx, opts, dir, false, false, command, args...) + _, err := shell.RunCommandWithOutput(ctx, opts, dir, false, false, command, args...) + if err != nil { + return fmt.Errorf("failed to run command in directory %s: %w", dir, err) + } return err })internal/cli/context.go (2)
40-45: Consider optimizing context value updatesThe current implementation creates a copy of the entire context when updating a value. For large contexts, this could be inefficient.
Consider using a pointer-based approach or implementing a more selective copy mechanism.
56-62: Add documentation for Flag methodThe new
Flagmethod could benefit from some documentation explaining its purpose and return value semantics.+// Flag retrieves a command flag by name. Returns nil if the command is not set +// or if the flag doesn't exist. func (ctx *Context) Flag(name string) Flag {internal/cloner/clone.go (1)
7-16: Nice use of generics and functional options pattern! 🎯The implementation is clean and follows modern Go practices. Just a small note - the nolint directive could be more specific:
-func Clone[T any](src T, opts ...Option) T { //nolint:ireturn +func Clone[T any](src T, opts ...Option) T { //nolint:ireturn // Generic functions naturally return interfaceinternal/strict/status.go (2)
29-30: Fix typo in commentThere's a small typo in the comment:
-// Status represetns the status of the Control. +// Status represents the status of the Control.
41-53: Consider using constants for ANSI color codesThe raw ANSI codes could be made more maintainable by using constants:
+const ( + greenColor = "\033[0;32m" + yellowColor = "\033[0;33m" + resetColor = "\033[0m" +) func (status Status) StringWithANSIColor() string { str := status.String() switch status { case ActiveStatus: - return "\033[0;32m" + str + "\033[0m" + return greenColor + str + resetColor case CompletedStatus, SuspendedStatus: - return "\033[0;33m" + str + "\033[0m" + return yellowColor + str + resetColor } return str }internal/strict/view/plaintext/template.go (2)
3-3: Consider extracting common template patterns into named sub-templatesThe
controlTemplateandsubcontrolTemplateshare similar structure. Consider extracting the common pattern into a named sub-template to improve maintainability.+const baseTemplate = `{{ .Name }}{{ "\t" }}{{ if .Description }}{{ .Description }}{{ else }}{{ .Warning }}{{ end }}` -const controlTemplate = `{{ .Name }}{{ "\t" }}{{ .Status.StringWithANSIColor }}{{ "\t" }}{{ if .Description }}{{ .Description }}{{ else }}{{ .Warning }}{{ end }}` +const controlTemplate = `{{ template "baseTemplate" . }}{{ "\t" }}{{ .Status.StringWithANSIColor }}` -const subcontrolTemplate = `{{ .Name }}{{ "\t" }}{{ if .Description }}{{ .Description }}{{ else }}{{ .Warning }}{{ end }}` +const subcontrolTemplate = `{{ template "baseTemplate" . }}`Also applies to: 8-8
13-20: Consider breaking down complex template logicThe
listTemplatecontains nested conditionals and multiple template calls. Consider breaking it down into smaller, more focused templates for better readability.+const categoryHeaderTemplate = `{{ $category.Name }}:` +const categoryControlsTemplate = `{{ $categoryControls := $controls.FilterByCategories $category }}{{ template "rangeControlsTemplate" $categoryControls }}` const listTemplate = ` {{ $controls := .controls }}{{ $categories := $controls.GetCategories.FilterNotHidden.Sort }}{{ range $index, $category := $categories }}{{ if $index }} {{ end }}{{ template "categoryHeaderTemplate" . }} {{ template "categoryControlsTemplate" . }} {{ end }}{{ $noCategoryControls := $controls.FilterByCategories }}{{ if $noCategoryControls }} {{ template "rangeControlsTemplate" $noCategoryControls }} {{ end }} `pkg/log/formatter.go (1)
50-58: Consider adding error handling for type conversionThe
Formatmethod assumes the type conversion will always succeed. Consider adding error handling for the case where fields can't be converted.func (f *fromLogrusFormatter) Format(parent *logrus.Entry) ([]byte, error) { + if parent == nil { + return nil, fmt.Errorf("nil entry provided") + } entry := &Entry{ Entry: parent, Level: FromLogrusLevel(parent.Level), Fields: Fields(parent.Data), } return f.Formatter.Format(entry) }cli/commands/run/file_copy_getter.go (1)
53-55: Consider adding more context to error messagesWhen wrapping the error from
GetFile, consider adding more context about what failed.- return errors.New(err) + return errors.Errorf("failed to copy file to %s: %v", dst, err)main.go (1)
32-34: Consider extracting context setup into a helper function.The context setup logic could be more reusable if extracted into a helper function.
+func setupContext(opts *options.TerragruntOptions, exitCode *tf.DetailedExitCode) context.Context { + ctx := context.Background() + ctx = tf.ContextWithDetailedExitCode(ctx, exitCode) + return log.ContextWithLogger(ctx, opts.Logger) +} func main() { // ... - ctx := context.Background() - ctx = tf.ContextWithDetailedExitCode(ctx, &exitCode) - ctx = log.ContextWithLogger(ctx, opts.Logger) + ctx := setupContext(opts, &exitCode) // ... }cli/commands/shortcuts.go (1)
33-71: Consider adding documentation for the experiment gate.The implementation looks solid! Just one suggestion: since the
CLIRedesignexperiment gates some functionality, it would be helpful to add a comment explaining when this experiment might be enabled/disabled.Add a comment like this:
func NewShortcutsCommands(opts *options.TerragruntOptions) cli.Commands { + // Note: Some functionality is gated behind the CLIRedesign experiment. + // This experiment controls the deprecation warnings for non-shortcut commands. var ( runCmd = run.NewCommand(opts)internal/strict/category.go (1)
54-63: Consider simplifying the Less implementation.The current implementation can be more concise:
-func (categories Categories) Less(i, j int) bool { - if len((categories)[j].Name) == 0 { - return false - } else if len((categories)[i].Name) == 0 { - return true - } - - return (categories)[i].Name < (categories)[j].Name +func (categories Categories) Less(i, j int) bool { + if categories[i].Name == "" || categories[j].Name == "" { + return categories[j].Name == "" + } + return categories[i].Name < categories[j].Name +}cli/commands/info/strict/command.go (2)
1-31: LGTM! Consider enhancing the package documentation.The command structure and flags are well-organized. The environment variables support is a nice touch! 🎯
Consider adding an example usage in the package documentation:
// Package strict represents CLI command that displays Terragrunt's strict control settings. +// Example usage: +// terragrunt info strict list # List active strict controls +// terragrunt info strict list --all # List all strict controls package strict
54-79: Solid implementation with room for a small enhancement.The filtering and display logic is well-organized. Consider making the error message more helpful when an invalid control name is provided.
- return strict.NewInvalidControlNameError(controls.Names()) + return strict.NewInvalidControlNameError(name, controls.Names())This would allow you to show both the invalid name and the list of valid names in the error message.
internal/cli/errors.go (1)
110-114: Consider enhancing the error message format.While the implementation is clean, the error message could be more helpful to users.
func (err UndefinedFlagError) Error() string { - return string(err) + return fmt.Sprintf("undefined flag: %s", string(err)) }cli/commands/exec/command.go (1)
47-53: Update command examples for clarity.The usage and examples look good, but could be more descriptive.
- "# Utilize the AWS CLI.\nterragrunt exec -- aws s3 ls", + "# List S3 buckets using AWS CLI\nterragrunt exec -- aws s3 ls", - "# Inspect `main.tf` file of module for Unit\nterragrunt exec --in-download-dir -- cat main.tf", + "# View the main.tf file in the module's download directory\nterragrunt exec --in-download-dir -- cat main.tf",cli/flags/prefix.go (2)
15-35: Great documentation and examples!The documentation is clear and the examples are helpful. Consider adding a note about the deprecated
TerragruntPrefixin the examples to guide users toward usingTgPrefix.Add a deprecation note in the examples:
// Can be passed to subcommands and contain the names of parent commands, // thus creating env vars as a chain of "TG prefix, parent command names, command name, flag name". For example: // `TG_HLC_FMT_FILE`, where `hcl` is the parent command, `fmt` is the command and `file` is a flag. Example of use: +// Note: Use TgPrefix instead of the deprecated TerragruntPrefix.
47-64: Consider adding input validation.The string manipulation looks good, but we might want to add some input validation.
func (prefix Prefix) EnvVar(name string) string { + if name == "" { + return "" + } name = strings.Join(append(prefix, name), "_") return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) }cli/commands/render-json/command.go (1)
60-61: Consider documenting the nil prefix usage.While the code is correct, it might be helpful to add a comment explaining why
nilis passed as the prefix torun.NewFlags. This would help future maintainers understand the intention.internal/cli/args_test.go (1)
91-123: Consider using descriptive test case names.The test cases are well-structured, but using descriptive names instead of indices would make the test output more meaningful. For example:
-t.Run(fmt.Sprintf("testCase-%d", i), func(t *testing.T) { +t.Run(fmt.Sprintf("remove_%s", testCase.removeName), func(t *testing.T) {internal/cli/flags.go (2)
13-32: Consider wrapping errors for better context.While the error handling is good, wrapping errors would provide more context about where they originated:
- return err + return fmt.Errorf("failed to apply flag: %w", err)
115-135: Consider pre-allocating slices for better performance.For the
Namesmethod, pre-allocating the slice capacity could improve performance slightly:- var names []string + names := make([]string, 0, len(flags))internal/strict/controls/control.go (3)
13-43: Consider adding more detailed field documentation.The struct documentation could be more comprehensive. Consider adding:
- Examples of when to use each field
- Explanation of the relationship between Status and Error/Warning fields
- Thread-safety guarantees for OnceWarn
95-120: Evaluate method could be more robust.A few suggestions to improve the Evaluate method:
- Consider adding context timeout handling
- The nil check at the start could be moved to a constructor to prevent nil Control instances
- The error from subcontrols.Evaluate could be wrapped with more context
Here's a suggested improvement:
func (ctrl *Control) Evaluate(ctx context.Context) error { - if ctrl == nil { - return nil - } + if err := ctx.Err(); err != nil { + return fmt.Errorf("context error during evaluation: %w", err) + } if ctrl.Enabled { if ctrl.Status != strict.ActiveStatus || ctrl.Error == nil { return nil } - return ctrl.Error + return fmt.Errorf("control %q failed: %w", ctrl.Name, ctrl.Error) } if logger := log.LoggerFromContext(ctx); logger != nil && ctrl.Warning != "" { ctrl.OnceWarn.Do(func() { logger.Warn(ctrl.Warning) }) } if ctrl.Subcontrols == nil { return nil } - return ctrl.Subcontrols.Evaluate(ctx) + if err := ctrl.Subcontrols.Evaluate(ctx); err != nil { + return fmt.Errorf("subcontrol evaluation failed: %w", err) + } + return nil }
90-93: Consider capacity pre-allocation for AddSubcontrols.When appending to Subcontrols, consider pre-allocating the slice capacity to avoid potential reallocations:
func (ctrl *Control) AddSubcontrols(newCtrls ...strict.Control) { + if ctrl.Subcontrols == nil { + ctrl.Subcontrols = make([]strict.Control, 0, len(newCtrls)) + } ctrl.Subcontrols = append(ctrl.Subcontrols, newCtrls...) }cli/commands/run/target.go (1)
24-42: Update documentation to reflect new package name.The code examples in the comments still reference the old
terraformpackage, but the package has been renamed torun. Let's update them to maintain consistency.- package terragruntinfo + package info ... code omitted - target := terraform.NewTarget(terraform.TargetPointDownloadSource, runTerragruntInfo) - terraform.RunWithTarget(opts, target) + target := run.NewTarget(run.TargetPointDownloadSource, runTerragruntInfo) + run.RunWithTarget(opts, target)cli/commands/hclfmt/command.go (1)
26-30: Consider documenting the prefix handling.The prefix handling logic is a bit complex with multiple prefix types (
tgPrefix,terragruntPrefix). A brief comment explaining the purpose of each prefix would help future maintainers.pkg/log/format/formatter.go (2)
121-124: Fix typo in method comment.There's an extra space in the comment for
SetDisabledOutput.-// SetDisabledOutput enables/disables log output. +// SetDisabledOutput enables/disables log output.
34-36: Consider adding early return optimization.The condition check could be simplified for better readability.
- if formatter.placeholders == nil || formatter.disabledOutput { - return nil, nil - } + if formatter.placeholders == nil || formatter.disabledOutput { + return nil, nil + }cli/commands/run/command.go (1)
85-92: Consider enhancing error messagesThe error handling could be more user-friendly. Instead of just saying it's a wrong command, we could provide examples of valid commands or suggest the closest matching command.
Here's a suggested improvement:
- if strings.HasSuffix(opts.TerraformPath, options.TerraformDefaultPath) { - return cli.NewExitError(errors.New(WrongTerraformCommand(opts.TerraformCommand)), cli.ExitCodeGeneralError) - } else { - // We default to tofu if the terraform path does not end in Terraform - return cli.NewExitError(errors.New(WrongTofuCommand(opts.TerraformCommand)), cli.ExitCodeGeneralError) - } + var errMsg string + if strings.HasSuffix(opts.TerraformPath, options.TerraformDefaultPath) { + errMsg = fmt.Sprintf("%s\nValid commands: %s", + WrongTerraformCommand(opts.TerraformCommand), + strings.Join(tf.CommandNames, ", ")) + } else { + errMsg = fmt.Sprintf("%s\nValid commands: %s", + WrongTofuCommand(opts.TerraformCommand), + strings.Join(tf.CommandNames, ", ")) + } + return cli.NewExitError(errors.New(errMsg), cli.ExitCodeGeneralError)shell/run_cmd_output_test.go (2)
67-67: Consider making test data path configurableThe hardcoded path to "testdata/test_outputs.sh" could cause issues if the test directory structure changes.
Consider using a test helper or constant:
+const testOutputsScript = "testdata/test_outputs.sh" + func testCommandOutput(t *testing.T, withOptions func(*options.TerragruntOptions), assertResults func(string, *util.CmdOutput), allocateStdout bool) { // ... - out, err := shell.RunCommandWithOutput(context.Background(), terragruntOptions, "", !allocateStdout, false, "testdata/test_outputs.sh", "same") + out, err := shell.RunCommandWithOutput(context.Background(), terragruntOptions, "", !allocateStdout, false, testOutputsScript, "same")
99-119: Consider moving BufferWithLocking to a separate fileThe thread-safe buffer implementation is well done, but it might be better suited in a separate test utilities file for reuse across other tests.
Consider moving this to something like
internal/testutil/buffer.gofor better organization and potential reuse.cli/help_test.go (2)
74-106: Consider using a more maintainable approach for expected output.The hardcoded expected output string makes the test brittle and hard to maintain. Consider:
- Loading the expected output from a test fixture file
- Using golden files that can be updated with a flag
Here's an example approach using golden files:
func TestCommandHelpTemplate(t *testing.T) { // ... existing setup code ... var out bytes.Buffer app.Writer = &out require.NoError(t, cli.ShowCommandHelp(ctx)) golden := filepath.Join("testdata", "run_help.golden") if *update { require.NoError(t, os.WriteFile(golden, out.Bytes(), 0644)) } expected, err := os.ReadFile(golden) require.NoError(t, err) assert.Equal(t, string(expected), out.String()) }
14-109: Consider adding more test cases.The test only covers the 'run' command. Consider adding test cases for other commands to ensure consistent help output across the CLI.
internal/experiment/experiment_test.go (1)
33-110: Consider adding edge cases to test cases.The test cases cover the basic scenarios well. Consider adding edge cases like:
- Empty experiment names
- Very long experiment names
- Special characters in experiment names
internal/cli/bool_flag_test.go (1)
129-138: Consider adding tests for multiple environment variables.Since
EnvVarwas replaced withEnvVarsslice, we should add test cases for:
- Multiple valid environment variables
- Mixed valid/invalid environment variables
- Precedence when multiple variables are set
internal/cli/args.go (1)
84-94: Consider optimizing Remove method.The current implementation creates multiple slices. Consider using a more efficient approach:
func (args Args) Remove(name string) Args { - for i := range args { - if args[i] == name { - args := Args(args.Slice()) - return append(args[:i], args[i+1:]...) - } - } + result := make([]string, 0, len(args)) + for _, arg := range args { + if arg != name { + result = append(result, arg) + } + } - return args + return result }test/integration_strict_test.go (1)
46-46: Consider making error message more specificThe error message "is no longer supported" seems too generic. Users might benefit from more context about what exactly is no longer supported.
- expectedError: errors.New("is no longer supported"), + expectedError: errors.New("`plan-all` command is no longer supported"),cli/flags/flag_opts.go (2)
83-83: Unused function retained for future useBased on past review comments,
WithDeprecatedPrefixis currently unused but retained for future use. Consider adding a TODO comment to document this intention.// The deprecated flag will have "terragrunt-no-color","terragrunt-disabe-color" names and "TERRAGRUNT_NO_COLOR","TERRAGRUNT_DISABLE_COLOR" env vars. +// TODO: This function is currently unused but retained for future flag deprecation needs.
82-82: Fix typo in example commentThere's a small typo in the example comment: "disabe-color" should be "disable-color".
-// The deprecated flag will have "terragrunt-no-color","terragrunt-disabe-color" names and "TERRAGRUNT_NO_COLOR","TERRAGRUNT_DISABLE_COLOR" env vars. +// The deprecated flag will have "terragrunt-no-color","terragrunt-disable-color" names and "TERRAGRUNT_NO_COLOR","TERRAGRUNT_DISABLE_COLOR" env vars.cli/commands/stack/action.go (1)
41-43: Consider standardizing error handling.For consistency with the
RunGeneratefunction above, consider usingcli.NewExitErrorhere as well.- return errors.New("stacks experiment is not enabled use --experiment stacks to enable it") + return cli.NewExitError(errors.New("stacks experiment is not enabled use --experiment stacks to enable it"), cli.ExitCodeGeneralError)internal/strict/controls/controls.go (2)
57-58: TODO needs attention.There's a TODO comment about error and warning messages not being displayed. Let's track this to ensure it doesn't get lost.
Would you like me to create an issue to track this TODO? I can help draft the issue with the necessary context.
56-63: Consider enhancing the skip dependencies control description.The
skipDependenciesInputsControldescription could be more specific about the performance impact and migration path. Consider adding:
- Expected performance improvement
- Step-by-step migration guide
- Examples of forwarding inputs as outputs
cli/commands/hclfmt/action.go (1)
144-144: Consider adding more context to the error.While wrapping the error is good, we could make it more helpful by adding context about what operation failed.
-return errors.New(err) +return errors.Errorf("failed to get file info for %s: %v", tgHclFile, err)test/integration_windows_test.go (1)
30-67: Well-structured temporary directory management.The implementation properly:
- Saves and restores environment variables
- Creates the temp directory if it doesn't exist
- Handles errors appropriately
- Uses defer for cleanup
One suggestion though - consider adding a check for directory permissions.
if _, err := os.Stat(tempDir); os.IsNotExist(err) { if err := os.Mkdir(tempDir, os.ModePerm); err != nil { fmt.Printf("Failed to create temp dir due to error: %v", err) os.Exit(1) } +} else { + // Verify write permissions + testFile := filepath.Join(tempDir, ".write_test") + if err := os.WriteFile(testFile, []byte(""), 0666); err != nil { + fmt.Printf("Temp dir %s is not writable: %v", tempDir, err) + os.Exit(1) + } + os.Remove(testFile) }cli/commands/run/version_check.go (1)
31-37: Consider refactoring to avoid side effects.The TODO comment correctly identifies a concern: this function has side effects by modifying the
terragruntOptionsparameter. This could lead to unexpected behavior and makes the function harder to test.Consider splitting this into two functions:
- A pure function that returns the necessary values
- A separate function that handles the side effects
This would make the code more maintainable and easier to test.
internal/strict/control_test.go (1)
3-6: Add more detailed test documentation.While the comment explains the basic purpose, it would be helpful to document:
- The test strategy
- The different scenarios being tested
- The expected outcomes for each case
test/integration_engine_test.go (1)
198-198: Consider using a constant for the environment variable name.Using the string literal
"TERRAGRUNT_ENGINE_SKIP_CHECK"directly makes the code more brittle. If this value needs to change in the future, we'd need to find and update all occurrences manually.Consider defining a constant for this environment variable name:
+const TerragruntEngineSkipCheckEnv = "TERRAGRUNT_ENGINE_SKIP_CHECK" -t.Setenv("TERRAGRUNT_ENGINE_SKIP_CHECK", "1") +t.Setenv(TerragruntEngineSkipCheckEnv, "1")cli/flags/global/flags.go (1)
293-298: Improve error handling for environment variable setting.The error from
os.Setenvis wrapped without context about what failed.Add more context to the error:
- return errors.New(err) + return errors.New(fmt.Errorf("failed to set CLI_TEMPLATE_ERROR_DEBUG environment variable: %w", err))config/config.go (1)
793-793: Consider using fmt.Errorf for error wrapping.While
errors.New(err)works, usingfmt.Errorf("failed to get file info: %w", err)would provide more context about where and why the error occurred.- return errors.New(err) + return fmt.Errorf("failed to get file info: %w", err)test/fixtures/exec-cmd/app/terragrunt.hcl (1)
1-8: Consider adding comments to document the configuration.While the configuration is straightforward, it would be helpful to add comments explaining the purpose of these inputs and why the source is set to the current directory.
+# This configuration manages local terraform resources terraform { source = "." } +# Input variables passed to the terraform configuration inputs = { foo = "FOO" bar = "BAR" }test/fixtures/exec-cmd/script.sh (2)
3-3: Thedirvariable is defined but never used.Either use the
dirvariable or remove it if it's not needed. Also, the space after the equals sign in the assignment could cause issues in some shells.-dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +# If you need the directory, use: +dir="$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)" +# If you don't need it, remove the line entirely🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 3-3: dir appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
7-11: Document required environment variables.Add comments to document the required environment variables at the beginning of the script. Also, consider adding more descriptive error messages.
#!/usr/bin/env bash +# Required environment variables: +# TF_VAR_foo - Should be set to "FOO" +# TF_VAR_bar - Should be set to "BAR" if [ "$TF_VAR_foo" != "FOO" ] then - echo "error: TF_VAR_foo is not defined" + echo "error: TF_VAR_foo must be set to 'FOO' (current value: ${TF_VAR_foo:-not set})" exit 1 fi if [ "$TF_VAR_bar" != "BAR" ] then - echo "error: TF_VAR_bar is not defined" + echo "error: TF_VAR_bar must be set to 'BAR' (current value: ${TF_VAR_bar:-not set})" exit 1 fiAlso applies to: 13-17
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: TF_VAR_foo is referenced but not assigned.
(SC2154)
docs/_docs/04_reference/01-configuration.md (1)
122-131: Refine HCL Formatting Commands Documentation:
The updates to thehclfmtcommand—changing flags to--diff,--check,--exclude-dir,--file, and--stdin—significantly simplify the user experience. One minor note: there’s a bit of repetitive phrasing around the usage instructions. You might consider rephrasing some parts for variety to keep the text engaging.🧰 Tools
🪛 LanguageTool
[style] ~130-~130: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file qa/services/services.hcl`. If you want to format HCL from stdin and print the res...(REP_WANT_TO_VB)
docs/_docs/04_reference/06-experiments.md (1)
60-63: TODO for Additional Examples
The “TODO” note at line 62 is a good prompt for future enhancements. It might be useful to add an example once more experiments are officially supported. Let me know if you’d like assistance drafting this section.🧰 Tools
🪛 LanguageTool
[misspelling] ~60-~60: This word is normally spelled with a hyphen.
Context: ...ble multiple experiments at once with a comma delimited list. TODO: Will add an example he...(EN_COMPOUNDS_COMMA_DELIMITED)
docs/_docs/04_reference/10-log-formatting.md (1)
16-16: Comma Suggestion in Introduction
It appears that a comma after the flag phrase could improve readability. For instance:-Using the `--log-custom-format <format>` flag you can customize the way Terragrunt logs with total control over the logging format. +Using the `--log-custom-format <format>` flag, you can customize the way Terragrunt logs with total control over the logging format.🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...sing the--log-custom-format <format>flag you can customize the way Terragrunt lo...(AI_HYDRA_LEO_MISSING_COMMA)
docs/_docs/02_features/12-provider-cache-server.md (2)
29-29: Missing Comma Suggestion
There seems to be a spot where a comma could improve the sentence flow. Consider updating the sentence as follows:-... so it is disabled by default. To enable it you need to use the flag [`provider-cache`](... +... so it is disabled by default. To enable it, you need to use the flag [`provider-cache`](...🧰 Tools
🪛 LanguageTool
[typographical] ~29-~29: It seems that a comma is missing.
Context: ...so it is disabled by default. To enable it you need to use the flag [`provider-cac...(IN_ORDER_TO_VB_COMMA)
123-125: Repetitive Sentence Beginnings in Configuration Options
The bullet list for "Server Hostname," "Server Port," and "Server Token" all begin similarly. Consider varying the phrasing slightly to avoid redundancy and enhance readability.🧰 Tools
🪛 LanguageTool
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lly every time you launch Terragrunt. - Server Token [provider-cache-token](https://...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/_docs/02_features/01-units.md (2)
166-167: Wordiness in Descriptive Text
The phrase “exact same” could be replaced with “identical” or simply “the same” to streamline the text. This minor change might enhance readability without altering the meaning.🧰 Tools
🪛 LanguageTool
[style] ~167-~167: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... so thesourceparameter supports the exact same syntax as the [module source](https://o...(EN_WORDINESS_PREMIUM_EXACT_SAME)
212-213: Punctuation for Compound Sentences
A brief comma before “and” in the sentence explaining the--source-updateflag could help with readability. A small punctuation tweak here would be beneficial.🧰 Tools
🪛 LanguageTool
[uncategorized] ~212-~212: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rragrunt with the--source-updateflag and it’ll delete the tmp folder, download t...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tf/getproviders/testdata/filesystem-mirror/registry.terraform.io/hashicorp/null/terraform-provider-null_2.1.0_linux_amd64.zipis excluded by!**/*.ziptf/getproviders/testdata/filesystem-mirror/registry.terraform.io/hashicorp/null/terraform-provider-null_invalid.zipis excluded by!**/*.zip
📒 Files selected for processing (207)
.circleci/config.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)cli/app.go(9 hunks)cli/app_test.go(13 hunks)cli/commands/aws-provider-patch/action.go(2 hunks)cli/commands/aws-provider-patch/command.go(1 hunks)cli/commands/catalog/action.go(1 hunks)cli/commands/catalog/command.go(2 hunks)cli/commands/catalog/module/repo.go(2 hunks)cli/commands/commands.go(1 hunks)cli/commands/deprecated.go(3 hunks)cli/commands/exec/action.go(1 hunks)cli/commands/exec/command.go(1 hunks)cli/commands/exec/options.go(1 hunks)cli/commands/flags.go(0 hunks)cli/commands/graph-dependencies/command.go(2 hunks)cli/commands/graph/action.go(3 hunks)cli/commands/graph/command.go(3 hunks)cli/commands/hclfmt/action.go(3 hunks)cli/commands/hclfmt/action_test.go(1 hunks)cli/commands/hclfmt/command.go(1 hunks)cli/commands/hclvalidate/action.go(1 hunks)cli/commands/hclvalidate/command.go(1 hunks)cli/commands/help/command.go(1 hunks)cli/commands/info/command.go(1 hunks)cli/commands/info/strict/command.go(1 hunks)cli/commands/output-module-groups/command.go(2 hunks)cli/commands/render-json/action.go(1 hunks)cli/commands/render-json/command.go(1 hunks)cli/commands/run-all/action.go(2 hunks)cli/commands/run-all/command.go(4 hunks)cli/commands/run/action.go(19 hunks)cli/commands/run/action_test.go(15 hunks)cli/commands/run/command.go(1 hunks)cli/commands/run/command_test.go(3 hunks)cli/commands/run/creds/getter.go(1 hunks)cli/commands/run/creds/providers/amazonsts/provider.go(1 hunks)cli/commands/run/creds/providers/externalcmd/provider.go(2 hunks)cli/commands/run/debug.go(4 hunks)cli/commands/run/download_source.go(11 hunks)cli/commands/run/download_source_test.go(11 hunks)cli/commands/run/errors.go(3 hunks)cli/commands/run/file_copy_getter.go(1 hunks)cli/commands/run/flags.go(1 hunks)cli/commands/run/hook.go(4 hunks)cli/commands/run/target.go(3 hunks)cli/commands/run/version_check.go(5 hunks)cli/commands/run/version_check_test.go(5 hunks)cli/commands/scaffold/action.go(8 hunks)cli/commands/scaffold/command.go(2 hunks)cli/commands/shortcuts.go(1 hunks)cli/commands/stack/action.go(3 hunks)cli/commands/stack/command.go(3 hunks)cli/commands/terraform/command.go(0 hunks)cli/commands/terragrunt-info/action.go(1 hunks)cli/commands/terragrunt-info/command.go(2 hunks)cli/commands/validate-inputs/action.go(2 hunks)cli/commands/validate-inputs/command.go(1 hunks)cli/commands/version/command.go(1 hunks)cli/deprecated_flags.go(0 hunks)cli/flags/deprecated_flag.go(1 hunks)cli/flags/flag.go(1 hunks)cli/flags/flag_opts.go(1 hunks)cli/flags/global/flags.go(1 hunks)cli/flags/prefix.go(1 hunks)cli/help.go(2 hunks)cli/help_test.go(1 hunks)cli/provider_cache.go(9 hunks)cli/provider_cache_test.go(1 hunks)config/catalog.go(1 hunks)config/config.go(6 hunks)config/config_helpers.go(5 hunks)config/config_partial.go(3 hunks)config/config_test.go(4 hunks)config/cty_helpers_test.go(1 hunks)config/dependency.go(6 hunks)config/parsing_context.go(2 hunks)config/util.go(2 hunks)config/variable.go(1 hunks)config/variable_test.go(1 hunks)configstack/log_test.go(1 hunks)configstack/module.go(2 hunks)configstack/module_test.go(3 hunks)configstack/running_module.go(3 hunks)configstack/running_module_test.go(1 hunks)configstack/stack.go(7 hunks)configstack/stack_test.go(9 hunks)docs/_docs/01_getting-started/04-terminology.md(3 hunks)docs/_docs/02_features/01-units.md(3 hunks)docs/_docs/02_features/03-includes.md(2 hunks)docs/_docs/02_features/10-auto-init.md(1 hunks)docs/_docs/02_features/12-provider-cache-server.md(3 hunks)docs/_docs/04_reference/01-configuration.md(2 hunks)docs/_docs/04_reference/03-built-in-functions.md(1 hunks)docs/_docs/04_reference/04-config-blocks-and-attributes.md(6 hunks)docs/_docs/04_reference/05-strict-mode.md(6 hunks)docs/_docs/04_reference/06-experiments.md(2 hunks)docs/_docs/04_reference/10-log-formatting.md(9 hunks)docs/_docs/04_reference/11-terragrunt-cache.md(1 hunks)docs/_docs/06_migration_guides/03-cli-redesign.md(1 hunks)engine/engine.go(1 hunks)internal/cache/context.go(1 hunks)internal/cli/app.go(5 hunks)internal/cli/args.go(4 hunks)internal/cli/args_test.go(3 hunks)internal/cli/autocomplete.go(2 hunks)internal/cli/bool_flag.go(1 hunks)internal/cli/bool_flag_test.go(6 hunks)internal/cli/command.go(9 hunks)internal/cli/command_test.go(3 hunks)internal/cli/commands.go(2 hunks)internal/cli/context.go(2 hunks)internal/cli/errors.go(3 hunks)internal/cli/exit_code.go(1 hunks)internal/cli/flag.go(1 hunks)internal/cli/flags.go(4 hunks)internal/cli/flags_test.go(4 hunks)internal/cli/funcs.go(1 hunks)internal/cli/generic_flag.go(5 hunks)internal/cli/generic_flag_test.go(7 hunks)internal/cli/help.go(3 hunks)internal/cli/map_flag.go(7 hunks)internal/cli/map_flag_test.go(5 hunks)internal/cli/slice_flag.go(5 hunks)internal/cli/slice_flag_test.go(6 hunks)internal/cli/sort_test.go(1 hunks)internal/cloner/clone.go(1 hunks)internal/cloner/cloner.go(1 hunks)internal/experiment/errors.go(1 hunks)internal/experiment/experiment.go(1 hunks)internal/experiment/experiment_test.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/control_test.go(1 hunks)internal/strict/controls/control.go(1 hunks)internal/strict/controls/controls.go(1 hunks)internal/strict/controls/depreacted_env_var.go(1 hunks)internal/strict/controls/deprecated_command.go(1 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)internal/strict/errors.go(1 hunks)internal/strict/status.go(1 hunks)internal/strict/strict.go(1 hunks)internal/strict/strict_test.go(0 hunks)internal/strict/view/plaintext/plaintext.go(1 hunks)internal/strict/view/plaintext/render.go(1 hunks)internal/strict/view/plaintext/template.go(1 hunks)internal/strict/view/render.go(1 hunks)internal/strict/view/view.go(1 hunks)internal/strict/view/writer.go(1 hunks)main.go(2 hunks)options/options.go(12 hunks)pkg/cli/bool_flag.go(0 hunks)pkg/cli/flag.go(0 hunks)pkg/log/context.go(1 hunks)pkg/log/format/format.go(5 hunks)pkg/log/format/formatter.go(5 hunks)pkg/log/format/options/color.go(1 hunks)pkg/log/format/options/option.go(1 hunks)pkg/log/formatter.go(1 hunks)pkg/log/helper.go(0 hunks)pkg/log/level.go(1 hunks)pkg/log/logger.go(5 hunks)pkg/log/options.go(1 hunks)remote/remote_state.go(1 hunks)shell/git.go(3 hunks)shell/run_cmd.go(1 hunks)shell/run_cmd_output_test.go(1 hunks)shell/run_cmd_test.go(4 hunks)shell/run_cmd_unix_test.go(2 hunks)shell/run_cmd_windows_test.go(2 hunks)shell/run_shell_cmd.go(0 hunks)terraform/terraform.go(0 hunks)test/fixtures/exec-cmd/app/main.tf(1 hunks)test/fixtures/exec-cmd/app/terragrunt.hcl(1 hunks)test/fixtures/exec-cmd/script.sh(1 hunks)test/helpers/package.go(9 hunks)test/integration_common_test.go(3 hunks)test/integration_debug_test.go(4 hunks)test/integration_destroy_test.go(5 hunks)test/integration_download_test.go(5 hunks)test/integration_engine_test.go(1 hunks)test/integration_functions_test.go(2 hunks)test/integration_hooks_test.go(1 hunks)test/integration_scaffold_test.go(11 hunks)test/integration_serial_test.go(4 hunks)test/integration_sops_test.go(2 hunks)test/integration_strict_test.go(5 hunks)test/integration_test.go(27 hunks)test/integration_windows_test.go(1 hunks)tf/cache/config.go(1 hunks)tf/cache/controllers/discovery.go(1 hunks)tf/cache/controllers/downloader.go(1 hunks)tf/cache/controllers/provider.go(1 hunks)tf/cache/handlers/provider.go(1 hunks)tf/cache/handlers/provider_direct.go(1 hunks)tf/cache/handlers/provider_filesystem_mirror.go(1 hunks)tf/cache/handlers/provider_network_mirror.go(1 hunks)tf/cache/handlers/provider_test.go(1 hunks)tf/cache/handlers/reverse_proxy.go(1 hunks)tf/cache/models/provider_test.go(1 hunks)tf/cache/server.go(1 hunks)tf/cache/services/provider_cache.go(1 hunks)tf/cliconfig/config_test.go(3 hunks)tf/context.go(2 hunks)tf/detailed_exitcode.go(1 hunks)tf/doc.go(1 hunks)
⛔ Files not processed due to max files limit (19)
- tf/errors.go
- tf/getproviders/hash_test.go
- tf/getproviders/lock.go
- tf/getproviders/lock_test.go
- tf/getproviders/package_authentication_test.go
- tf/getter.go
- tf/getter_test.go
- tf/log.go
- tf/run_cmd.go
- tf/run_cmd_test.go
- tf/source.go
- tf/source_test.go
- tf/testdata/test_outputs.sh
- tf/tf.go
- tflint/tflint.go
- util/collections.go
- util/file.go
- util/file_test.go
- util/shell.go
💤 Files with no reviewable changes (9)
- pkg/log/helper.go
- internal/strict/strict_test.go
- cli/commands/terraform/command.go
- terraform/terraform.go
- cli/commands/flags.go
- pkg/cli/flag.go
- cli/deprecated_flags.go
- shell/run_shell_cmd.go
- pkg/cli/bool_flag.go
✅ Files skipped from review due to trivial changes (27)
- cli/commands/commands.go
- internal/strict/view/view.go
- tf/doc.go
- internal/strict/view/plaintext/plaintext.go
- tf/cache/services/provider_cache.go
- config/catalog.go
- cli/commands/run/creds/providers/amazonsts/provider.go
- tf/cache/handlers/reverse_proxy.go
- pkg/log/format/options/option.go
- cli/commands/catalog/module/repo.go
- tf/cache/config.go
- tf/cache/controllers/downloader.go
- docs/_docs/02_features/03-includes.md
- tf/cache/controllers/discovery.go
- cli/commands/run/creds/getter.go
- configstack/module_test.go
- config/util.go
- tf/cache/handlers/provider_test.go
- tf/cache/handlers/provider_direct.go
- tf/cache/models/provider_test.go
- internal/cli/sort_test.go
- tf/cache/controllers/provider.go
- tf/cache/server.go
- tf/cache/handlers/provider.go
- tf/cache/handlers/provider_network_mirror.go
- tf/cache/handlers/provider_filesystem_mirror.go
- cli/provider_cache_test.go
🧰 Additional context used
🪛 LanguageTool
docs/_docs/04_reference/10-log-formatting.md
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...sing the --log-custom-format <format> flag you can customize the way Terragrunt lo...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/_docs/02_features/01-units.md
[style] ~167-~167: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... so the source parameter supports the exact same syntax as the [module source](https://o...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[uncategorized] ~212-~212: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rragrunt with the --source-update flag and it’ll delete the tmp folder, download t...
(COMMA_COMPOUND_SENTENCE)
docs/_docs/04_reference/01-configuration.md
[style] ~130-~130: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file qa/services/services.hcl`. If you want to format HCL from stdin and print the res...
(REP_WANT_TO_VB)
docs/_docs/02_features/12-provider-cache-server.md
[typographical] ~29-~29: It seems that a comma is missing.
Context: ...so it is disabled by default. To enable it you need to use the flag [`provider-cac...
(IN_ORDER_TO_VB_COMMA)
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lly every time you launch Terragrunt. - Server Token [provider-cache-token](https://...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/_docs/04_reference/05-strict-mode.md
[style] ~107-~107: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...agrunt will exit with error code 1 ``` You can also use [control categories](#cont...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/_docs/06_migration_guides/03-cli-redesign.md
[style] ~29-~29: Consider removing “of” to be more concise
Context: ...sier to find them. ## Migration guide All of the changes you need to make to adopt to th...
(ALL_OF_THE)
[misspelling] ~30-~30: Did you mean “adapt to”?
Context: ... All of the changes you need to make to adopt to this new CLI design involve changing ho...
(ADOPT_TO)
[style] ~34-~34: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact flag with terragrunt- removed from th...
(SAME_EXACT)
[style] ~36-~36: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ragrunt-non-interactive flag, you will need to switch to the [--non-interactive`](/do...
(REP_NEED_TO_VB)
[style] ~52-~52: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...the --terragrunt-debug flag, you will need to switch to the [--inputs-debug](/docs/...
(REP_NEED_TO_VB)
[style] ~70-~70: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact environment variable with TERRAGRUNT_...
(SAME_EXACT)
🪛 Shellcheck (0.10.0)
test/fixtures/exec-cmd/script.sh
[warning] 3-3: dir appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 7-7: TF_VAR_foo is referenced but not assigned.
(SC2154)
[warning] 13-13: TF_VAR_bar is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (378)
pkg/log/format/options/color.go (1)
163-163: LGTM! Variable rename improves clarity.The rename from
DisableColorstoDisabledColorsis a nice improvement - it's more grammatically correct and consistent with typical boolean naming conventions.Let's make sure this rename was applied consistently across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of the old name rg "DisableColors" --type go # Search for all instances of the new name to verify consistent usage rg "DisabledColors" --type goLength of output: 1751
Variable Rename Verified – All Consistent!
The shell script results confirm that all instances across the codebase now use "DisabledColors", with no lingering "DisableColors" in sight. The one use of "DisableColor" in the conditional is the correct constant check and doesn't conflict with the rename.
internal/cache/context.go (1)
1-5: Clean and focused package structure!The package name and imports are well-chosen and minimal.
internal/strict/view/plaintext/render.go (2)
1-11: Clean and well-organized imports! 👍The imports are logically grouped (standard library followed by project imports) and each import serves a clear purpose.
13-19: Great use of Go idioms! 🎯Love the compile-time interface check - it's a neat trick to catch missing method implementations early. The empty struct is perfect since we don't need any state.
docs/_docs/04_reference/05-strict-mode.md (11)
32-37: Clear Example for Deprecated Commands Strict Control
This code snippet clearly demonstrates how the strict-control flag (using “deprecated-commands”) forces Terragrunt to error out when a deprecated command is used. The error message is specific and informative—nice work ensuring the example aligns well with the intended behavior.
58-60: Updated Environment Variable Usage for Strict Mode
The example now correctly shows the new environment variable TG_STRICT_MODE. It’s great to see the documentation reflecting the renaming change and providing a concrete error output example.
68-70: Good Contrast Between Warning and Error Messages
This code block shows the default behavior—displaying a warning instead of an error—when strict controls are not enforced. It effectively contrasts with the strict-mode examples later.
73-75: Enforcing Strict Control via CLI Flag
The example using “--strict-control plan-all” clearly demonstrates how enabling a specific strict control converts the warning into an error. The error message is consistent and aligns with the desired behavior.
80-82: Using TG_STRICT_CONTROL Environment Variable
This snippet effectively shows how to use the environment variable TG_STRICT_CONTROL to achieve the same strict-control behavior. It’s a clear addition to help users in CI/CD contexts.
101-106: Enabling Multiple Strict Controls via Environment Variable
The example using a comma-separated list (with TG_STRICT_CONTROL) and executing multiple commands via bash -c is very illustrative. It might be worth double-checking that the use of bash -c fits all deployment contexts, but overall it clearly demonstrates how multiple controls can be enabled simultaneously.
110-113: Example for Control Categories Usage
The code snippet using “--strict-control deprecated-commands” is helpful and concise. It shows how control categories can aggregate multiple strict controls efficiently.
127-130: Updated List of Strict Mode Controls
The additions of the new items—[root-terragrunt-hcl], [terragrunt-prefix-flags], [terragrunt-prefix-env-vars], and [default-command]—make the strict controls section comprehensive and aligned with current deprecation policies.
188-194: Clear Documentation for Deprecated Flags
The “terragrunt-prefix-flags” section clearly explains that the old “--terragrunt-” flag prefix is no longer supported. The inclusion of an example (replacing --terragrunt-non-interactive with --non-interactive) is very useful for users.
195-201: Updated Environment Variables Prefix Explanation
The “terragrunt-prefix-env-vars” section does a good job explaining the renaming from TERRAGRUNT_ to TG_. The reason and example provided are informative and align well with the overall updates.
202-208: Deprecation of the Default Command
The “default-command” section explains the change to support the new “run” command—this is clear and concise. The example provided reinforces the advice, ensuring users know to switch from using the deprecated default command.cli/commands/catalog/command.go (4)
6-8: All good with the new imports
The refactor to use scaffold- and flags-related packages is aligned with the new approach. Looks smooth to me.
16-20: Double-check which flags remain after filtering
Filtering out certain flags is neat, but please confirm we’re not unintentionally dropping any must-have flags.
25-28: Be mindful of breaking changes with ErrorOnUndefinedFlag
Forbidding undefined flags improves safety but might surprise users if they rely on extra flags. A quick heads-up in release notes would be nice.
37-37: Fallback to the default root file name
Auto-assigning a default if none is set helps keep consistency. Nicely done!internal/cli/help.go (5)
6-6: Ensure Go version compatibility for slices
The “exp/slices” import is handy. Just confirm our minimum Go version is fresh enough to support it, typically 1.21 or higher.
37-37: Return success for ShowAppHelp
Returning an explicit success code is consistent with other CLI operations. Good call!
40-45: Flexible command help flow
Nice switch to using ctx.Command.CustomHelp and flexible templates. Streamlines help usage for custom commands!Also applies to: 46-49, 51-53, 55-57, 59-61, 63-63
78-79: Same success code for version
Returning success from the version help is consistent with the rest. No concerns here.
81-96: Parent commands logic
Reversing the chain of parent commands is a neat improvement. This helps the user see their command path in a more intuitive order.cli/flags/flag.go (2)
32-34: Verify nil usage in NewMovedFlag.
Calling NewFlag(nil, …) could be risky if any part attempts to access the underlying cli.Flag methods or fields. Double-check that there’s no path leading to a nil pointer dereference.
59-80: Good approach for integrating deprecated flags.
Applying both the new flag and its deprecated counterparts in one pass is straightforward. The logic looks consistent and should help keep code DRY. Nice work!internal/strict/controls/depreacted_env_var.go (2)
21-29: Structure for DeprecatedEnvVar looks solid.
Embedding Control and storing formatting strings is straightforward. This design effectively centralizes logic for deprecated environment variables.
57-95: Nice use of sync.Once and context-aware logging.
Using ctrl.OnceWarn.Do ensures that the warning is logged only once, which is user-friendly and avoids spamming. The Evaluate method is also cleanly laid out.cli/commands/graph/command.go (4)
12-16: Confirm old imports replaced by run and flags.
Replacing Terraform-based calls with “run” is sensible for your new strategy. Just verify that all references to the old import are removed to avoid confusion or leftover dependencies.
22-25: Deprecated and non-deprecated flags share the same name.
Setting both GraphRootFlagName and DeprecatedGraphRootFlagName to “graph-root” may cause confusion. If they differ only internally, that’s fine, but ensure users and docs reflect one consistent approach.
47-51: ErrorOnUndefinedFlag may surprise users.
With ErrorOnUndefinedFlag = true, Terragrunt will fail if users pass unknown flags. This is often desired but can lead to user frustration if reusing older flags. Double-check you clearly communicate this in docs.
81-81: cmds.Add(run.NewCommand(opts)) is consistent with the new architecture.
Incorporating the run command in subCommands aligns with the rest of the refactor and seems coherent.cli/commands/deprecated.go (6)
2-2: Good reorganization of package structure.Switching to "package commands" helps keep things clear and domain-focused.
7-7: Imports look well organized.These new imports provide the necessary references to the updated modules. Just be sure all are used (no dead imports).
Also applies to: 9-10, 12-12
30-36: Map of deprecated commands is straightforward.The mapping from old commands to the new run-all commands is tidy. Make sure the help docs also mention these replacements.
43-47: Structured approach to track deprecated commands.Using “NewDeprecatedCommand” and adding subcontrols clarifies usage for these legacy commands. Great approach for ensuring deprecated features are tracked.
64-64: New factory function aligns with the overall design.The “NewDeprecatedCommands” function is an intuitive way to generate the hidden commands. This keeps them neatly separated from active commands.
71-72: Flags for deprecated commands.Associating the default run flags with these deprecated commands is practical. Just confirm none of these flags conflict with the replaced commands’ usage.
cli/commands/scaffold/command.go (7)
5-5: New imports add clarity.The additional imports for context, flags, and strict controls neatly reflect the new scaffolding logic.
Also applies to: 7-11
18-21: Great use of descriptive flag constants.Explicit naming for “root-file-name,” “no-include-root,” “var,” and “var-file” helps maintain clarity.
24-48: Flag definitions with Action callbacks are solid.Enforcing a non-empty root-file-name and hooking into strict controls is user-friendly. The error message for empty input is also straightforward.
50-55: Clear boolean flag usage.“NoIncludeRootFlagName” neatly toggles root scaffolding. Straightforward and easy to test.
57-69: Good approach for collecting multiple vars.Using slice flags for “var” and “var-file” is convenient for capturing unknown numbers of inputs.
75-78: Strict enforcement of undefined flags.Enabling “ErrorOnUndefinedFlag” ensures users don’t pass random flags by mistake, which is a nice tough-love approach.
99-105: Fit-for-purpose default root file selection.Reverting to “RecommendedParentConfigName” if strict checks fail is a safe fallback. Should reliably handle user defaults.
internal/strict/controls/deprecated_flag_name.go (7)
1-2: Neat package structure.“controls” is a logical home for these strict checks on flags.
3-12: Imports are consistent with the rest of the changes.Everything lines up with your newly added CLI and logging logic.
14-18: Constant approach helps categorize flags.Defining the format strings upfront makes the code more maintainable and consistent.
20-20: Explicit conformance to “strict.Control.”This highlights that “DeprecatedFlagName” fully implements the interface. A nice self-documenting detail.
22-30: Struct is well-structured for capturing deprecation details.It’s clear how error and warning formats align with the control’s data fields.
32-48: Helper for flags with new command location.“NewDeprecatedMovedFlagName” guides users toward the correct command. This is succinct and practical.
50-73: Straightforward definition for typical deprecation.Allowing an optional newValue is a nice touch for flags that must pass a parameter.
cli/commands/run-all/command.go (3)
12-16: Nice import updates, watch for potential duplication.Bringing in "run" and "flags" packages is logical. Quick check though: if import collisions occur or if we don’t actually need all these references, consider pruning them to maintain clarity. Otherwise, looks good!
41-62: Check for misalignment between usage and environment variables.The new flags method is well-structured. However, confirm that the env vars (tgPrefix.EnvVars) align with usage help text. That’ll reduce confusion for users setting environment variables. Also, watch out for duplication in the “deprecated” arrays so as not to create collisions unexpectedly.
93-93: Great addition for subcommand consistency.Adding run.NewCommand as a subcommand ensures consistent user experience across commands. Looks properly integrated, no further concerns here.
internal/experiment/experiment.go (9)
11-13: Imports look good—just keep an eye on usage.The addition of "golang.org/x/exp/slices" is neat for sorting. Double-check that we’re not inflating our dependency footprint unnecessarily. Otherwise, looks fine.
15-22: Clear experiment constants.Neat approach to enumerating experiments. Just ensure the doc references are up-to-date so folks know what each experiment does (e.g., synergy with docs).
24-28: Straightforward status constants.The iota usage is a nice pattern. Confirm that these statuses map well to rest of the code so we don’t accidentally treat “completed” as “ongoing.”
31-31: Switching from map to slice.Converting Experiments from map to slice simplifies iteration. Check if you have code anywhere else that relies on map-based lookups to avoid performance hits.
38-47: Default initialization seems correct.Having a default set of experiments is helpful. Just be sure that enabling quietly is deliberate (nobody might call
EnableExperimentfor them, and they’ll remain disabled). All good from a read standpoint.
63-74: Filter method is well-structured.This is straightforward. Just be sure the usage fits your concurrency model if multiple goroutines might call FilterByStatus simultaneously—though a slice is generally safe if read-only.
76-85: Simple, effective Find method.No big concerns. Be sure to handle the possibility that an experiment name might be spelled incorrectly. Looks good though.
94-103: EnableExperiment approach is solid.Returning an error for invalid experiment names is user-friendly. Just ensure the error text clarifies how they can see valid options.
122-129: Evaluate experiment by name.Neat convenience method. If you anticipate cross-checking multiple statuses than “ongoing,” might rename or adapt. But for now, looks good.
shell/run_cmd.go (5)
1-2: Intro docstring is clear and direct.Great for explaining the package purpose. Helps new contributors quickly see what this package is all about.
4-20: Clean import separation.Context, logging, exec, etc., are all relevant for shell commands. Keep an eye on the overall dependency list to ensure minimal overhead. Looking good though.
30-35: RunCommand helper function.Nicely wraps RunCommandWithOutput. This helps keep code DRY. No immediate worries here.
37-49: Clear doc for RunCommandWithOutput.Explaining the usage of workingDir and why we might suppress stdout is great. The doc is thorough and addresses real user scenarios.
51-149: Robust command execution logic.• Good usage of context and telemetry.
• Watch out for overshadowing references toerrin nested scopes, though it seems fine here.
• The usage of the engine for Terraform is especially nice.
• The graceful shutdown approach is good—just be mindful of heavier resource usage might need more direct management.
Overall, this is well-structured error handling and logging.internal/cli/slice_flag.go (9)
5-5: Good use of the standard library.
Importing "os" is straightforward and doesn't appear to introduce any issues.
66-73: Ensure consistent environment variable splitting behavior.
Setting a custom LookupEnvFunc is handy, but consider explaining in docs how flagged environment variables are handled if multiple EnvVars are set or if no environment variable is found. Also confirm you don’t accidentally ignore partial matches.
76-78: Initialization logic is clear, but watch for potential collisions.
valueType and value creation seems fine, but if multiple flags share the same Destination, you might get unexpected merges. In most cases, it won’t happen, but consider clarifying.Also applies to: 79-84
87-87: Neat usage of ApplyFlag.
This cleanly delegates final application. No issues spotted.
100-102: EnvVars retrieval is straightforward.
No concerns here, but watch for empty slices and their effect on parent code.
108-108: Returning the initial text value is consistent.
This ensures help text remains aligned with your default state.
137-141: sliceValue struct is well designed.
The included fields provide enough context to parse multiple values. Just be sure the concurrency model is safe if flags are processed in parallel.
143-150: Constructor pattern is consistent.
newSliceValue neatly encapsulates logic, but watch for potential overshadowing if used widely. Otherwise, looks good!
164-165: Conditional setter call is a handy extension point.
This is great if you want immediate side effects on set. Just ensure the setter never panics or changes the slice size mid-parse.internal/cli/generic_flag.go (9)
45-55: Streamlined Apply logic but watch for overshadowing.
Setting FlagValue only if nil is good defensive coding. Keep an eye on user-provided FlagValue to avoid overshadowing custom logic.
68-71: EnvVars array retrieval is consistent.
Implementation is parallel to slice_flag.go, giving a unified approach. Looks good.
76-77: Default text approach synergy.
Returning the initial text for help displays is a friendly user experience.
94-97: Flexible action handling with Destination.
Graceful fallback to a new pointer is fine if Destination is nil. Double-check that your final T has a valid default state.Also applies to: 99-100
106-106: Helpful reference for typed usage.
You’re using var _ = Value(...) for internal type registration or checks, which seems standard.
110-111: Adding a setter in genericValue is great.
This is a neat place to intercept changes. Just watch for error chaining so you keep context if setter fails.
114-119: newGenericValue constructor is straightforward.
This keeps value initialization consistent. No major concerns.
123-130: Setting the flag calls the underlying setter.
Returning setter errors upfront keeps usage clear. Good approach.
146-155: genericVar struct is minimal but effective.
Just keep track that you might unavoidably have to handle new(T) default states.cli/flags/deprecated_flag.go (15)
1-2: New package for deprecated flags is well-named.
This centralizes all logic in one place, making it simpler to track.
3-11: Dependence on internal strict controls is interesting.
You’re mixing package references for controlling deprecations. Check if that layering might complicate maintainability, e.g., cyclical imports.
12-16: Aggregating DeprecatedFlags is a nice approach.
No immediate issues. Just ensure iteration logic is consistent when applying them.
17-26: DeprecatedFlag struct is straightforward.
Storing original Flag plus custom fields is a clean approach. Document carefully how newValueFn is used.
28-31: Hiding deprecated flags from help.
Smart strategy to avoid cluttering the CLI. Implementation looks correct.
38-45: GetEnvVars fallback is consistent.
Allowing fallback to the underlying Flag’s environment list is good. Just watch for collisions or overshadowed env vars.
47-54: Name fallback logic parallels env var fallback.
This will keep usage consistent. Looks good.
56-59: Evaluate for warnings or errors.
This concept is neat, especially if you plan to unify logs vs. exceptions. No immediate issues.
61-79: SetStrictControls ensures an easy path for future removal.
Nice approach—once the control is triggered, you wrap errors/warnings. Just confirm end-users get a clear message.
81-88: NewValueFunc design is flexible.
This is a good pattern to transition old flags to new ones with possibly different types.
90-95: NewValue convenience function.
Simple approach that works. No concerns.
97-99: RegisterStrictControlsFunc callback is well-defined.
You keep strict controls at a single integration point, which helps maintain clarity.
111-117: StrictControlsByGlobalFlags for easy grouping.
Neat approach to keep global flags under one category.
119-126: MovedGlobalFlags approach is interesting.
This specifically addresses flags that shifted from global to command scope. Solid concept.
128-147: registerStrictControls is methodical.
Filtering then adding subcontrols is readable. Just confirm the string formatting meets all edge cases if controlNames is empty.internal/cli/map_flag.go (2)
66-68: Initialization in Apply.
Good call on making sure the map is only allocated once if Destination is nil. This avoids a panic down the line.
83-91: Centralized environment variable lookup function is handy.
This is a clean approach: if folks ever need custom logic (e.g., reading from an encrypted store), they can override LookupEnvFunc. Everything else flows neatly.internal/cloner/cloner.go (1)
216-250: Struct cloning logic.
This approach is thorough, but watch out for fields that might contain channels, functions, or other typed references that remain unhandled (in your switch). If users expect those to be copied or zeroed, they might be surprised.internal/cli/command.go (4)
25-25: Nicely done including Examples.
Having real usage examples ups the friendliness factor and clarifies what folks should type.
28-29: Rename for clarity: ErrorOnUndefinedFlag.
This rename is more explicit—great improvement on meaning vs “DisallowUndefinedFlags.” New name helps communicate the expected behavior instantly.
104-107: Graceful handling of parseFlags errors.
Returning a cleaned-up error with NewExitError is more user-friendly. This clarifies the exit reason nicely.
262-271: WrapAction is a neat hooking mechanism.
Cloning command structure then wrapping the Action is a flexible approach. However, if we keep chaining multiple WrapActions, watch out for recursion or confusion about which “action” is the final.internal/cli/flag.go (1)
244-263: Looks straightforward.The logic in “ApplyFlag” accurately binds environment variables and command-line arguments to flags, ensuring any conflicts trigger errors. Good work!
internal/strict/control.go (1)
186-199: Logging strategy looks great.The logging of enabled and completed controls ensures clear visibility in strict mode usage.
cli/app.go (7)
78-79: Neat approach for global flags and commands.Setting up global flags separately and then wrapping commands with telemetry is tidy and modular.
136-136: Good idea to handle “no-color” duplicates early.Centralizing the duplicate-flag handling in one place helps reduce confusion with color flags.
206-206: Command definitions look solid.Defining Terragrunt commands in a single block keeps things organized and easy to maintain.
321-334: Command alias detection is on point.Mapping “terraform apply -destroy” to “terraform destroy” avoids confusion and ensures consistent run order.
337-339: Nice color-propagation detail.Appending the “-no-color” flag to Terraform if Terragrunt color is disabled is a good user-experience choice.
463-463: Tidy final assignment.Assigning “opts.RunTerragrunt = runCmd.Run” is a clear way to unify Terragrunt’s runtime entry point with the new
runcommand logic.
183-201:⚠️ Potential issueAvoid double-allocating the slice in removeNoColorFlagDuplicates.
Currently, you create “filteredArgs” with length = len(args), then call “append” inside the loop. You’ll end up with empty slots or an overly large slice.
Here’s a fix:
-func removeNoColorFlagDuplicates(args []string) []string { - var ( - foundNoColor bool - filteredArgs = make([]string, len(args)) - ) +func removeNoColorFlagDuplicates(args []string) []string { + var ( + foundNoColor bool + filteredArgs = make([]string, 0, len(args)) + )Likely invalid or redundant comment.
cli/provider_cache.go (12)
15-23: Smooth import refactoring.
It's great to see the migration to the new package layout for clarity and maintainability.
160-160: Ensure context continuity.
Switching to tf.ContextWithTerraformCommandHook looks fine, but double-check that passing nil won’t break downstream usage.
180-180: Looks consistent with command detection.
Changing to tf.CommandNameInit is in line with the new naming. Great job!
182-182: Providers lock condition.
Using tf.CommandNameProviders with args.SubCommandName() == tf.CommandNameLock is explicit and clear. Thumbs up!
188-188: Switch to tf.RunCommandWithOutput.
Replacing shell calls with tf is consistent. Make sure you’ve removed any leftover shell references.
252-252: Clone opts usage.
CloneWithConfigPath here is straightforward. Just confirm we properly handle nested or absolute config paths.
260-260: Refactored run command.
Switching to tf.RunCommandWithOutput is appropriate. Good improvement for consistent usage.
340-342: Automatic no-color flag injection.
This snippet properly respects user input. Nice approach to unify color settings.
356-356: Consistent command execution.
All command runs align well under tf.RunCommandWithOutput. Smooth change!
373-373: Adaptive environment variable naming.
Using tf.EnvNameTFTokenFmt with a sanitized registry name is a neat approach.
389-389: Explicitly setting TF_CLI_CONFIG_FILE.
Overriding ensures Terraform picks up your custom config. This looks good.
392-392: Disable plugin cache.
Setting TF_PLUGIN_CACHE_DIR to an empty string is likely intentional, but confirm any side effects on local dev flows.cli/commands/run/download_source_test.go (13)
1-1: New package name.
Renaming to run_test clarifies the test ownership and scope. Thumbs up.
18-19: Refreshed imports.
Switching to the run and tf packages is consistent with your refactor. Looks good.
258-260: Handling source download errors.
Great use of run.DownloadingTerraformSourceErr. It makes error handling more explicit.
278-280: ValidateWorkingDir with run.
The typed error (WorkingDirNotFound) is a better approach for clear error reporting. Good job.
298-300: WorkingDir sanity checks.
run.ValidateWorkingDir and run.WorkingDirNotDir give clear user feedback for invalid paths. Nice detail.
383-383: Consistent source download.
run.DownloadTerraformSourceIfNecessary keeps logic symmetrical. Established patterns are easier to reason about.
397-397: ModuleInitRequiredFile check.
Neat feature to confirm module initialization. This prevents silent misconfigurations.
402-407: createConfig refactor.
The function and new tf.Source usage reflect the new approach nicely.
428-428: Populate Terraform version.
run.PopulateTerraformVersion is a logical addition. Good place to keep version info up to date.
438-438: Constructing tf.Source.
Yes, referencing your new Source struct is consistent with the broader refactor.
449-449: AlreadyHaveLatestCode check.
run.AlreadyHaveLatestCode is a nice helper. Good separation of concerns.
457-458: Using t.TempDir.
This modern approach cleans up temp directories automatically—big improvement over manual logic.
544-545: UpdateGetters usage.
The new run.UpdateGetters function with the FileCopyGetter is a clean design for customization. Well done!Also applies to: 549-549
cli/app_test.go (16)
5-5: Refined imports and prefix usage.
These changes align your code with the new run and global flags modules. Good reorganization.Also applies to: 16-16, 18-19, 21-21, 25-25, 36-36
51-52: Command arguments and error handling.
Expanding coverage for plan/apply combos and undefined flags is a thorough approach.Also applies to: 57-58, 65-65, 70-70, 75-79
117-120: DeprecatedIgnoreExternalDependencies.
Your logic is consistent here. Keep an eye out for eventual deprecation removal.
123-126: IAM role flags.
Neat expansions for role ARN, session name, and web identity tokens. The structure is flexible.Also applies to: 129-132, 135-138, 141-144
147-150: Multiple flags in a single command.
Combining config path, working dir, and source flag tests ensures coverage for real-world usage. Good job.Also applies to: 153-156
160-161: Setting logs to debug.
Testing log level changes is wise. This ensures no surprises for verbose output.
165-165: Arg value presence checks.
Using argMissingValueError is straightforward. These validations help keep user input consistent.Also applies to: 167-167, 171-171, 173-173, 177-177, 179-179, 182-185
194-207: Test harness around parse logic.
The runAppTest usage is neat, verifying different flag combos thoroughly.
299-299: Filtering out Terragrunt flags.
Your approach systematically extracts Terragrunt's own flags from Terraform’s arguments. Simple and effective.Also applies to: 306-313, 315-315, 317-322
329-329: Multi-string arg tests.
Covers normal usage and error states. That extra coverage is nice.Also applies to: 337-337, 338-338, 339-339, 340-340
360-361: Key-value arg validation.
clipkg.NewInvalidKeyValueError is a good way to guide users toward correct input formats.Also applies to: 362-363, 375-375
439-454: Terragrunt help tests.
Ensuring help outputs are correct is crucial for user onboarding. Nicely done.
464-468: Terraform help coverage.
Confirming usage text for plan/apply with the wrapped binary is essential. Good to see these tests.
496-501: Custom action setup.
setCommandAction is a clean approach to unify actions across commands.
504-505: runAppTest architecture.
This function elegantly merges global flags, app commands, and a custom action. Great test scaffolding.Also applies to: 507-507, 509-509, 512-512, 514-514, 517-520
537-537: Autocomplete coverage.
These checks ensure relevant commands and flags appear in completions. A user-friendly detail!Also applies to: 557-559, 574-576
cli/commands/run/flags.go (3)
39-61: Verify consistent usage references for “modules” vs “units.”The usage text references both “units” and “modules” in different flags, which can confuse newcomers. Decide on a consistent term. If some commands truly operate at a “unit” level vs a “module” level, clarify that difference in the usage lines.
370-380: Double-check handling for 'include-module-prefix' deprecation.You’re providing a hidden flag that warns users about the inverted behavior of “--tf-forward-stdout.” Good. However, watch out if new usage patterns or new flags further complicate this logic. It’s worth adding a short mention in docs or release notes about how output prefixing is managed now.
495-509: Good approach for feature flags.Storing them in a map and letting the user set them via “--feature” is intuitive and flexible. Be sure to doc any specially recognized feature names so folks know how to use them effectively.
options/options.go (5)
16-21: Check that internal packages remain consistent post-refactor.You’re referencing new packages: “internal/cli,” “internal/strict,” and “internal/strict/controls.” Ensure that the rest of the codebase consistently uses these updated imports so we don’t get weird import cycles or leftover references to the old paths.
100-123: Shadow copying version fields is a safe approach.Marking TerraformVersion and TerragruntVersion with “clone:"shadowcopy"” ensures minimal overhead while preventing accidental mutability. Good use of the cloner library to handle these carefully.
134-134: Switching to cli.Args instead of []string is a nice improvement.This change helps standardize how CLI args are parsed and manipulated. Just confirm your test suite thoroughly covers edge cases, like empty arguments or subcommands with unexpected spacing.
366-375: Shadow copying strict controls and other concurrency-safe maps is valuable.Defining “clone:"shadowcopy"” for StrictControls, Experiments, FeatureFlags, and ReadFiles avoids deep copy overhead on large structures while retaining thread safety. Good approach for large codebases with optional concurrency.
581-604: Neat approach to cloning while preserving the logger.“newOpts.Logger = opts.Logger.Clone()” ensures that any advanced logging configuration is also replicated. Just keep in mind that if Loggers write to the same underlying file or stdout, concurrency-safety is still shared.
test/integration_test.go (11)
17-17: Nice import addition.The new "run" package import looks consistent with the rest of the references.
28-28: Importing 'tf' module.Switching from 'shell' to 'tf' can reduce confusion by centralizing relevant logic in one package. Looks good.
107-107: New test fixture constant.Defining "testFixtureExecCmd" clarifies the fixture used in the upcoming "exec" command tests. Straightforward addition.
172-174: Context-based update to tf.DetailedExitCode.This transition away from shell.DetailedExitCode is consistent with the new 'tf' module usage, so no issues here.
191-194: Refined use of tf.DetailedExitCode.Keeping exit code variables aligned with the tf package is a good improvement for clarity.
209-212: Further switch to tf.DetailedExitCode.This repeated pattern keeps the test context consistent across modules. Nicely done.
230-233: Consistent switch to tf.DetailedExitCode.Maintaining the same approach in every test suite standardizes code handling.
474-476: Symlink resolution for rootPath.Evaluating symlinks prevents confusion when symbolic links and real paths diverge. Good practice.
1071-1074: Disable command validation scenario.Setting “--terragrunt-disable-command-validation” ensures invalid commands reach Terraform. This is helpful for advanced users or edge cases.
1101-1121: Test Terraform subcommands array.The subcommand suite systematically checks varied argument combos. This is a good coverage approach.
1123-1138: Subcommand iteration logic.Looping through subcommands under test is comprehensive. Good job verifying different permutations.
internal/strict/view/render.go (1)
5-11: Clean interface design! 👍The
Renderinterface is well-documented and follows good Go practices with clear method names and purpose.cli/commands/help/command.go (1)
14-23: Nice and clean implementation! 👍The command structure follows best practices and maintains good separation of concerns by delegating to
global.HelpAction. TheHidden: trueis a smart choice since this is an alternative to the--helpflag.Since this is a new command, let's make sure it's documented in the README or CLI docs. Could you check if it's mentioned?
cli/commands/version/command.go (1)
14-23: Looking good! Consistent with the help command pattern 🎯The implementation follows the same clean pattern as the help command, which is great for maintainability. Using
cli.NewExitErrorwith status 0 is the right choice for version info display.Let's ensure the version output format is consistent with the --version flag:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Compare version command and flag outputs echo "Checking version command implementation..." rg -A 5 "VersionAction" "cli/flags/global/"Length of output: 532
Version command output format is consistent with the --version flag
The shell script results confirm that both the version command (in cli/commands/version/command.go) and the --version flag implementation use the same VersionAction, which in turn calls cli.ShowVersion. This consistency assures that the version info is displayed in the same format, matching the overall design.
- The version command leverages global.VersionAction.
- global.VersionAction calls cli.ShowVersion, just like the --version flag.
Everything checks out nicely!
cli/commands/terragrunt-info/command.go (1)
17-17: Flag initialization updated as part of refactoring 🔄The change to use
run.NewFlagsaligns with the broader refactoring effort. This is consistent with changes in other commands, which is great for maintainability.Let's verify that the flag behavior remains consistent after this change:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any flag-related changes in tests echo "Checking terragrunt-info command tests..." rg -l "terragrunt-info.*test" test/ rg "NewFlags.*nil" "cli/commands/"Length of output: 1502
Flag initialization behaves as expected; refactoring verified ✔️
The integration tests include references to terragrunt-info, and a review of similar command files confirms that the new flag initialization using run.NewFlags is consistent across the codebase. There’s no sign of unexpected behavior introduced by this change.
cli/commands/graph-dependencies/command.go (1)
5-7: Nice work on standardizing the flag handling! 👍The switch to
run.NewFlagsaligns with the broader effort to make command handling more consistent across the codebase. This change will make it easier to maintain and extend command flags in the future.Also applies to: 17-17
internal/strict/controls/deprecated_command.go (1)
9-12: Great job on organizing command categories! 🏷️Clear and descriptive category names that will help with command organization.
tf/detailed_exitcode.go (1)
11-15: Solid thread-safe implementation! 🔒The mutex usage ensures safe concurrent access to the exit code. Nice job!
cli/commands/info/command.go (1)
15-27: Nice implementation of the command structure! 👍The command setup is clean and follows good practices. I particularly like:
- Using a prefix for flag handling
- Enabling
ErrorOnUndefinedFlagfor better UX- Setting up subcommands for extensibility
pkg/log/options.go (1)
29-29: Clean simplification! 🎯Nice job simplifying the formatter setup by directly calling
SetFormatter. This makes the code more straightforward while maintaining the same functionality.shell/run_cmd_unix_test.go (1)
22-22: Solid test refactoring! 🔄The changes maintain parity with the Windows test while keeping Unix-specific error handling. Good use of
requirefor cleaner assertions.Also applies to: 36-36
internal/strict/view/writer.go (1)
11-15: Nice job on the clean struct design! 👍The
Writerstruct has a good separation of concerns, embeddingio.Writerand using composition withrender.cli/commands/run/command_test.go (1)
47-50: Clean context setup! 🎯Nice use of method chaining for context creation. Makes the code very readable.
internal/experiment/errors.go (1)
33-35: Great user-friendly error message! 🌟Love how you included the documentation URL in the error message. This helps users find solutions quickly.
configstack/log_test.go (1)
53-55: Nice test assertions! ✅Clear and focused assertions that verify the log level enforcement.
cli/commands/exec/action.go (1)
16-19: Consider enabling auto-init with InDownloadDir flagBased on the previous discussion, users might need to inspect files in the download directory without manually running
terragrunt initfirst. Currently, auto-init is explicitly disabled when not using the download directory.Would you like me to propose a modification that enables auto-init when
InDownloadDiris true?cli/commands/stack/command.go (1)
27-28: LGTM! Clean flag handling updateThe changes to use
ErrorOnUndefinedFlagand sorted flags improve the command's robustness and consistency.cli/commands/output-module-groups/command.go (1)
19-19: LGTM! Consistent flag handlingThe change to use
run.NewFlagsaligns with the standardized approach to flag management across commands.internal/cloner/clone.go (1)
18-44: LGTM! Well-documented option functionsThe three option functions are well-designed and documented. They provide flexible configuration for:
- Shadow copying specific types
- Skipping specific types
- Shadow copying based on package prefixes
config/cty_helpers_test.go (1)
23-24: LGTM! Test case updates look goodThe test case modifications maintain good coverage while testing the conversion of unknown values to empty strings.
Also applies to: 29-30
cli/commands/catalog/action.go (1)
40-40: Nice simplification! 👍Direct evaluation of the experiment flag makes the code cleaner and more straightforward.
tf/context.go (1)
1-1: Nice simplification of cache handling! 👍The package rename from
shelltotfmakes the purpose clearer, and usingcache.ContextWithCachesimplifies the caching logic.Also applies to: 24-24
pkg/log/formatter.go (1)
8-36: Well-documented interface design! 🎯The
Formatterinterface is clean and thoroughly documented. Each method has a clear purpose and documentation.cli/commands/run/file_copy_getter.go (1)
40-44: Good addition of source path validation! 👍The validation ensures the source path exists and is a directory before proceeding.
main.go (3)
4-15: LGTM! Package reorganization looks good.The shift from
shelltotfpackage forDetailedExitCodeand the addition of flag handling capabilities align well with the PR objectives of command restructuring.Also applies to: 20-20
50-52: Nice improvement to error handling!The conditional error stack trace logging is a good optimization - only showing the stack trace when it's non-empty makes the output cleaner.
24-28: Verify log level behavior with environment variables.The new flag-based approach for handling
TG_LOG_LEVELlooks good, but let's ensure it maintains backward compatibility.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Test environment variable handling export TG_LOG_LEVEL=trace ./terragrunt --version 2>&1 | grep -i "TRACE"Length of output: 89
Let's dive a little deeper into this. The initial test with --version didn’t trigger any trace-level output, which makes sense since version commands typically don't output logging information. To get a clearer picture, I’d like to verify how TG_LOG_LEVEL is used in the code and inspect the implementation of NewLogLevelFlag. This will help us confirm its behavior and that it maintains backward compatibility with environment variables.
Please run the following shell script:
Once we see the results, we can better assess how the flag-based approach is wired up. Thanks!
🏁 Script executed:
#!/bin/bash echo "========== Searching for TG_LOG_LEVEL references ==========" rg "TG_LOG_LEVEL" . echo "========== Searching for NewLogLevelFlag implementation ==========" rg "func NewLogLevelFlag" .Length of output: 807
Log level behavior verified with environment variable support.
After checking the TG_LOG_LEVEL references, documentation, and the NewLogLevelFlag implementation in cli/flags/global/flags.go, it’s clear that the new flag-based approach still reads the environment variable as expected. The test using the --version command didn’t output TRACE logs because that command isn’t designed to produce logging output.
Everything appears to be working as intended while maintaining backward compatibility.
internal/cli/flags_test.go (2)
8-8: Good move on package organization and immutability!The shift to internal package path and the assignment-based flag handling (
actual = actual.Add) suggests proper encapsulation and immutable operations.Also applies to: 45-45
54-54: Nice cleanup on test readability!The variadic argument syntax and multi-line formatting make the test code more idiomatic and easier to read.
Also applies to: 66-69
cli/commands/validate-inputs/command.go (1)
15-17: Verify deprecation notice behavior.The deprecation handling looks good, but let's ensure users get proper notices when using the deprecated flag.
Also applies to: 32-32
cli/commands/hclvalidate/action.go (1)
63-63: Verify color handling consistency.The change to use logger formatter for color settings is good for consistency, but let's verify it works as expected.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Test color handling NO_COLOR=1 ./terragrunt hcl-validate 2>&1 | grep -P '\x1b\[' # Should return nothing if colors are properly disabledLength of output: 59
Color Handling Consistency Confirmed
The test ran with NO_COLOR set, and no ANSI escape sequences were detected in the output. This confirms that the logger formatter properly disables color when required, and the change in cli/commands/hclvalidate/action.go is consistent with the expected behavior.
cli/commands/terragrunt-info/action.go (1)
10-10: LGTM! Clean package transition.The switch from
terraformtorunpackage is clean and maintains the same functionality.Also applies to: 16-18
cli/help.go (2)
15-17: LGTM! Improved help text formatting.The changes to global options display make the help output cleaner and more consistent.
24-41: Add unit tests for help templates.The template changes look good, but we still need tests to ensure they work correctly.
Previous reviewers have requested tests for these templates. Would you like me to help create some test cases?
cli/commands/shortcuts.go (1)
17-30: LGTM! Well-organized command shortcuts.The list of shortcut commands is comprehensive and well-organized.
cli/commands/graph/action.go (2)
9-9: LGTM! Clean package transition.The switch from
terraformtorunpackage is clean and maintains the same functionality.Also applies to: 19-21
42-42: LGTM! Improved configuration handling.The switch to
CloneWithConfigPathis a good improvement for handling configuration paths.cli/commands/run/errors.go (3)
1-1: LGTM! Package rename aligns with the broader refactoring.The package rename from
terraformtorunbetter reflects the command's purpose and aligns with the PR's objectives.
14-15: LGTM! Error message update reflects new command structure.The error message now correctly references
terragrunt run planinstead ofterragrunt plan, matching the new command structure.
61-68: LGTM! Well-structured error type for run-all disabled case.The new
RunAllDisabledErrtype follows good practices:
- Descriptive field names
- Clear error message formatting
- Reuses existing code pattern (as confirmed in past review comments)
cli/commands/hclvalidate/command.go (3)
16-20: LGTM! Clear and consistent flag naming.The flag names follow a consistent pattern:
- Primary flags:
show-config-pathandjson- Deprecated flags clearly marked with
hclvalidate-prefix
23-44: LGTM! Well-structured flag creation with proper deprecation handling.The flag creation logic:
- Uses the new
flags.NewFlaghelper- Properly handles environment variables
- Clearly marks deprecated flags
47-58: LGTM! Command creation with improved error handling.Good addition of
ErrorOnUndefinedFlag: trueto catch misconfigurations early.internal/strict/category.go (1)
77-83: Verify documentation for strict control categories.Based on past review comments, we need to ensure strict controls are properly documented.
✅ Verification successful
🌐 Web query:
What are strict controls in Terragrunt and how are they documented?💡 Result:
Strict controls in Terragrunt are opt-in validations that enforce future-proof usage by converting deprecation warnings into errors and preventing outdated patterns[2][3]. These controls help maintain compatibility with upcoming Terragrunt versions by proactively flagging deprecated features. ### Key Features of Strict Controls - **Deprecation Enforcement**: Convert warnings about deprecated commands (e.g., `plan-all`, `apply-all`) into hard errors[2] - **Future Compatibility**: Prepare codebases for breaking changes before major releases[3] - **Gradual Adoption**: Enable specific controls individually using `--strict-control` flags[2] ### Implementation Methods ```bash # Enable all strict controls terragrunt --strict-mode plan # Enable specific controls terragrunt plan --strict-control plan-all --strict-control apply-all # Environment variable approach (CI/CD friendly) TERRAGRUNT_STRICT_CONTROL='plan-all,apply-all' terragrunt plan ``` ### Documented Controls Include: 1. **Deprecated Commands** `plan-all` → Requires `terragrunt run-all plan`[2] `apply-all` → Requires `terragrunt run-all apply`[2] 2. **Configuration Validation** `validate-inputs` command gains strict checking for unused variables with `--terragrunt-strict-validate`[4][10] 3. **Root Configuration** New `root-terragrunt-hcl` control warns about legacy root file usage[6] ### Documentation Sources 1. Official [Strict Mode Documentation](https://terragrunt.gruntwork.io/docs/reference/strict-mode/) detailing controls and usage[2] 2. CLI reference showing `--strict-control` and `--strict-mode` flags[1][7] 3. Gruntwork Blog explaining the roadmap for strict controls[3] 4. GitHub Discussions for pre-release control details[6] These controls are particularly recommended for CI/CD pipelines to catch compatibility issues early while allowing gradual adoption through targeted enablement[2][3]. The documentation emphasizes testing specific controls before full enforcement to avoid unexpected failures[2][6]. Citations: - 1: https://megalinter.io/v8/descriptors/terraform_terragrunt/ - 2: https://terragrunt.gruntwork.io/docs/reference/strict-mode/ - 3: https://blog.gruntwork.io/the-road-to-1-0-terragrunt-feature-flags-82cebb8b501d?gi=9aaa5604254c - 4: https://fig.io/manual/terragrunt/plan - 5: https://terragrunt.gruntwork.io/docs/ - 6: https://github.com/gruntwork-io/terragrunt/discussions/3626 - 7: https://fig.io/manual/terragrunt - 8: https://pkg.go.dev/github.com/gruntwork-io/terragrunt/options - 9: https://docs.spacelift.io/vendors/terraform/terragrunt - 10: https://terragrunt.gruntwork.io/docs/reference/cli-options/
Strict control categories documentation appears sufficient as-is.
Based on the official Terragrunt docs, strict controls are opt‐in validations that convert deprecation warnings into errors to help prepare for breaking changes—featuring flags like --strict-mode and --strict-control. The in-code documentation in internal/strict/category.go defines a simple Category struct that groups these controls (with fields for the display name and visibility). While it’s minimal, it aligns with the overall approach of maintaining detailed explanations in the external documentation.
If needed, you might consider adding a cross-reference (such as a URL) to the official Terragrunt Strict Mode documentation (https://terragrunt.gruntwork.io/docs/reference/strict-mode/) in the comments. Otherwise, the current documentation meets our requirements by keeping in-code comments clear and letting the comprehensive details live in the external docs.
internal/cli/commands.go (2)
91-99: LGTM! Optimized slice allocation in WrapAction.Good performance improvement by pre-allocating the slice with the exact size needed.
101-105: LGTM! Clean Sort implementation.The Sort method correctly uses the sort.Sort interface implementation from Lines 79-89.
cli/commands/info/strict/command.go (1)
33-52: Well-structured command setup!The command configuration is clean and user-friendly. Good job on the error handling and usage text! 👍
internal/cli/errors.go (1)
29-48: Nice type safety improvement!The change from
inttoExitCodemakes the code more type-safe while maintaining compatibility. 🛡️cli/commands/exec/command.go (1)
61-71: Nice handling of command arguments!Good job on the flexible argument handling that supports both
terragrunt exec -- lsandterragrunt exec lsformats. The fallback to help when no command is provided is user-friendly. 👏cli/commands/render-json/command.go (2)
4-9: Nice work on the flag naming improvements!The renaming of flags follows a more consistent pattern and properly handles deprecation notices. This will help with the transition while maintaining backward compatibility.
Also applies to: 11-20
22-50: Good improvements to flag management!The new prefix-based approach and environment variable handling make the code more maintainable and flexible. The deprecated flags are properly linked to their new counterparts.
internal/cli/args_test.go (1)
75-75: Good catch on fixing the test expectation!The updated expected value correctly reflects that "two" should be the command name when the first argument is removed.
internal/cli/flags.go (1)
46-63: Great improvements to the API!The changes to
FilterandAddmethods make them more flexible and follow better functional programming practices by avoiding in-place modifications.cli/commands/aws-provider-patch/command.go (1)
64-68: Consider hiding this command.Based on previous discussions, this command might be better hidden from the main CLI interface since it's primarily a workaround for a specific issue.
Add the Hidden field to hide this command:
return &cli.Command{ Name: CommandName, Usage: "Overwrite settings on nested AWS providers to work around a Terraform bug (issue #13018).", + Hidden: true, Flags: append(run.NewFlags(opts, nil), NewFlags(opts, nil)...).Sort(), Action: func(ctx *cli.Context) error { return Run(ctx, opts.OptionsFromContext(ctx)) }, }shell/run_cmd_test.go (2)
80-81: Cache context handling improvement looks good.Nice refactoring to use the dedicated cache package for context management. This makes the caching mechanism more maintainable and centralized.
22-22: Function rename looks good, but let's verify all usages.The renaming from
RunShellCommandtoRunCommandis consistent across all test cases.Let's check for any missed occurrences:
Also applies to: 25-25, 41-41, 54-54
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of RunShellCommand rg "RunShellCommand"Length of output: 758
Renaming Verified: No Missed Usage Found in Test Cases
The search shows that all calls to the shell command in our tests have been updated to use RunCommand. The remaining occurrences of “RunShellCommand” in tf/context.go are context-related type definitions and comments—these are intentional and not subject to the renaming.
tf/cliconfig/config_test.go (1)
9-9: Nice modernization of the test code!Good improvements:
- Using the new package path structure with
tfinstead ofterraform- Leveraging
t.TempDir()which is cleaner and handles cleanup automaticallyAlso applies to: 22-22
config/variable_test.go (1)
71-71: Better JSON comparison using JSONEq - nice catch!Great improvement switching to
assert.JSONEqfor comparing JSON strings. This handles different key ordering and provides better error messages when JSON comparisons fail.Also applies to: 74-74
cli/commands/run/target.go (1)
14-14: LGTM! New target point for environment variables.The addition of
TargetPointSetInputsAsEnvVarsaligns well with the PR's objective of implementing environment variable generation.cli/commands/hclfmt/command.go (1)
13-24: Well-structured flag renaming with proper deprecation handling! 👍Nice job maintaining backward compatibility while simplifying the flag names. The clear separation between new and deprecated names makes the transition smoother for users.
config/parsing_context.go (1)
52-52: Clean package transition fromshelltotf! 👍The change from
shell.ContextWithTerraformCommandHooktotf.ContextWithTerraformCommandHookaligns well with the broader package restructuring effort.pkg/log/format/format.go (1)
21-39: Nice job on the function renaming! 👍The addition of "Placeholders" to function names (
NewBareFormatPlaceholders,NewPrettyFormatPlaceholders,NewJSONFormatPlaceholders,NewKeyValueFormatPlaceholders) makes their purpose much clearer. The implementation remains solid while the naming better reflects what these functions return.Also applies to: 41-69, 71-110, 112-135
cli/commands/run/command.go (1)
28-30: TODO comment needs attentionThe TODO comment indicates a missing example for the
--allflag. Let's track this to ensure it's added back when the functionality is supported.Would you like me to help create a tracking issue for adding this example back once the
--allflag support is implemented?shell/git.go (1)
24-24: Clean refactoring of command execution! 🎯Good job updating the command execution calls for consistency. The changes from
RunShellCommandWithOutputtoRunCommandWithOutputand the cache context key update make the code more maintainable.Also applies to: 44-44, 75-75
cli/commands/run-all/action.go (1)
11-11: LGTM! Package migration looks good.The import path and command name references have been consistently updated from
terraformtotfpackage.Let's verify that all terraform package references have been updated:
Also applies to: 17-21
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old terraform package rg "github.com/gruntwork-io/terragrunt/terraform" --type goLength of output: 59
Let's run one more check to make sure no lingering references to the old "terraform" package exist (e.g. cases like "terraform.CommandName") in any Go file. This will add further confidence that the migration to the "tf" package was fully carried out.
If this command produces no output, it confirms that all references have been updated correctly. Let me know if any results appear!
🏁 Script executed:
#!/bin/bash # Additional verification: search for any lingering references to the old "terraform" package prefix # This includes cases such as "terraform.CommandName" that might have been accidentally missed during refactoring. rg "\bterraform\." --type goLength of output: 17179
Migration Verified – All Good!
The search for "github.com/gruntwork-io/terragrunt/terraform" returned no output, confirming that the import path in cli/commands/run-all/action.go was successfully updated to "tf". The additional references you see from "terraform." are mostly in comments, string constants (like "terraform.tfstate" or "-var-file=terraform.tfvars"), or in files not part of this migration effort. These are expected and don't signal any refactoring issues.
All tests and code references in the run-all command look consistently updated with no lingering dependency on the old import path.
cli/commands/render-json/action.go (1)
20-21: LGTM! Command handling migration looks good.The changes correctly move the target handling from terraform to run package.
Also applies to: 28-30
cli/commands/run/debug.go (1)
1-1: LGTM! Package and function references updated correctly.The changes consistently update all references from terraform to tf package while maintaining the existing debug functionality.
Also applies to: 13-13, 30-30, 80-80
internal/experiment/experiment_test.go (3)
16-20: Nice work on the test constants! 👍Good choice using descriptive constant names that clearly indicate their purpose in the tests.
22-28: Well-structured test logger setup.Clean implementation of the helper function that encapsulates logger creation logic. This promotes reusability and keeps the test code DRY.
117-136: Thorough test validation logic.Good job on the comprehensive validation checks. The error handling is particularly well done with proper use of
requirefor critical assertions.internal/cli/autocomplete.go (1)
104-104: Verify the impact of changing TrimPrefix to TrimLeft.The switch from
TrimPrefixtoTrimLeftchanges how dashes are stripped from flags. WhileTrimPrefixwould only remove one dash,TrimLeftremoves all leading dashes. Let's verify this doesn't break flag parsing for edge cases.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for flag usages with multiple leading dashes rg -l '(?:^|\s)-{3,}[a-zA-Z]'Length of output: 275
Impact of using TrimLeft on flag parsing
By switching to TrimLeft, all leading dashes are stripped from the flag string rather than just the first dash. The grep search confirms that several parts of the codebase use flags with multiple leading dashes (e.g., "--flag"). This indicates the auto-completion and flag parsing logic is intended to work with fully de-dashed flag names.
In short:
- With TrimPrefix, a flag like "--foo" would become "-foo", potentially leaving an undesired dash.
- With TrimLeft, "--foo" cleanly becomes "foo", which appears consistent with how flags are treated elsewhere in the repository.
Based on the information gathered, the impact of this change is minimal and actually aligns with the expected handling of flags throughout the project.
internal/cli/bool_flag_test.go (1)
28-103: Great improvements to bool flag testing! 🎯Nice work on:
- Supporting multiple environment variables
- Clearer error messages
- Comprehensive test cases covering various scenarios
internal/cli/args.go (3)
29-38: Improve Split method documentation.The current comment is a bit terse. Consider expanding it to explain:
- What happens when separator isn't found
- Whether the separator is included in either slice
- Example usage
40-44: Nice implementation of WithoutBuiltinCmdSep! 👍Clean and efficient implementation using Split and slices.Clone for proper slice handling.
142-169: Well-structured command name handling! 🎯Good refactoring to use CommandNameN as the base implementation. The code is more maintainable and reduces duplication.
test/integration_strict_test.go (3)
31-31: Grammar fix in error message looks good!The error message now reads more naturally with proper English grammar.
36-37: Control name change improves clarityThe change from "plan-all" to "deprecated-commands" better reflects the broader category of deprecated commands. This makes the control more maintainable as new deprecated commands can be added without creating individual controls.
Also applies to: 39-39
111-111: CLI flags have been simplified and modernizedGreat job on simplifying the CLI flags by removing the
terragruntprefix! The addition of the--experiment cli-redesignflag suggests this is part of a broader CLI modernization effort.Also applies to: 120-120
internal/cli/bool_flag.go (2)
41-53: Well-structured flag value handlingThe implementation of
ApplyandRunActionmethods is clean and handles edge cases well, particularly the nil destination pointer check inRunAction. Good job!Also applies to: 95-107
24-25: Consider adding validation for empty EnvVarsThe EnvVars slice could potentially be nil. While this might be intentional, it's worth adding a comment or validation to make this behavior explicit.
Also applies to: 34-34
config/variable.go (1)
29-29: Nice simplification of experiment evaluation!The direct evaluation of the experiment is cleaner than storing it in an intermediate variable. This kind of small refactoring makes the code more maintainable.
cli/flags/flag_opts.go (1)
15-48: Well-structured flag deprecation handlingThe
WithDeprecatedMovedFlagimplementation is thorough and handles both flag names and controls appropriately. The reverse ordering of deprecated names is a nice touch for maintaining backward compatibility.pkg/log/level.go (1)
149-156: LGTM! Clean and effective implementation.The new
ToLogrusLevelmethod provides a clear and safe way to convert custom logging levels to logrus levels, with proper handling of unknown levels.internal/cli/map_flag_test.go (1)
141-150: LGTM! Good test coverage for environment variables handling.The updated
LookupEnvFuncimplementation properly handles multiple environment variables and edge cases. The test cases thoroughly cover different scenarios.test/integration_sops_test.go (1)
115-116: Nice touch! Better error message formatting.Converting newlines to spaces makes error messages more readable in logs. Simple but effective improvement.
internal/cli/slice_flag_test.go (1)
177-186: LGTM! Consistent with map flag changes.The updated environment variable handling matches the changes in map_flag_test.go, maintaining consistency across the codebase. Good test coverage for all scenarios.
internal/cli/generic_flag_test.go (1)
26-26: LGTM! Good enhancement to support multiple environment variables.The change from
EnvVartoEnvVars []stringimproves flexibility by allowing multiple environment variables to be associated with a single flag.Also applies to: 33-33, 40-40, 47-47, 91-91, 142-142, 149-149, 156-156
cli/commands/run/creds/providers/externalcmd/provider.go (2)
10-11: LGTM! Clean up of import paths.Import paths have been updated to reflect the new package structure, improving code organization.
49-49: Verify the behavior of the new command execution method.The change from
RunShellCommandWithOutputtoRunCommandWithOutputmight have different behavior. Let's verify the implementation:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the implementation of both methods to compare their behavior ast-grep --pattern 'func RunCommandWithOutput($ctx, $opts, $_, $_, $_, $cmd, $args) { $$$ }' ast-grep --pattern 'func RunShellCommandWithOutput($ctx, $opts, $_, $_, $_, $cmd, $args) { $$$ }'Length of output: 198
🏁 Script executed:
#!/bin/bash echo "Searching for implementations of RunCommandWithOutput..." rg -n "func RunCommandWithOutput" . echo "" echo "Searching for implementations of RunShellCommandWithOutput..." rg -n "func RunShellCommandWithOutput" .Length of output: 538
🏁 Script executed:
#!/bin/bash echo "Displaying RunCommandWithOutput implementation in tf/run_cmd.go..." sed -n '40,100p' tf/run_cmd.go echo "" echo "Displaying RunCommandWithOutput implementation in shell/run_cmd.go..." sed -n '40,100p' shell/run_cmd.goLength of output: 3826
Verified: The new command execution method behaves as expected.
After digging into the implementations:
- The function in tf/run_cmd.go now delegates to the new shell.RunCommandWithOutput.
- The shell/run_cmd.go method wraps command execution with telemetry, handles output suppression correctly, and conditionally uses the engine when enabled.
- There's no remaining definition for RunShellCommandWithOutput, indicating the change was deliberate and its functionality has been merged into RunCommandWithOutput.
The behavior remains consistent with previous functionality while adding improved logging/telemetry. No issues were found.
cli/commands/run/version_check_test.go (1)
2-2: LGTM! Consistent package renaming.Package and import paths have been updated to align with the new structure, moving from
terraformtorunpackage.Also applies to: 7-7
cli/commands/stack/action.go (1)
32-34: Good improvement in error handling with exit codes.The change to use
cli.NewExitErrorwith a specific exit code improves error reporting and makes it more consistent with CLI standards.cli/commands/run/hook.go (3)
1-1: Nice package reorganization!The package rename from
terraformtorunand the addition of theclonerpackage make the code organization clearer and more modular.Also applies to: 9-9
196-196: Clean environment variable handling.Using
cloner.Clonefor environment variables is a nice improvement. It's more explicit about its purpose and likely more maintainable.
75-75: Good standardization of command execution methods.The switch from
RunShellCommandWithOutputtoRunCommandWithOutputaligns with best practices for command execution. Just make sure all callers are aware of any subtle differences between these methods.Let's check for any other instances that might need updating:
Also applies to: 161-161
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of RunShellCommandWithOutput rg "RunShellCommandWithOutput"Length of output: 30
I'll double-check by inspecting the relevant lines in the file to verify no trace of RunShellCommandWithOutput remains. Let's run a script to search the entire repo for the old method and specifically inspect lines 150–170 in cli/commands/run/hook.go (which should cover the earlier line 161 mention):
Once you run this, please share the results so I can confirm that the refactoring is complete.
🏁 Script executed:
#!/bin/bash # Search the repository for any occurrences of RunShellCommandWithOutput rg -n "RunShellCommandWithOutput" # Show lines 150-170 from cli/commands/run/hook.go to check the transition at line 161 sed -n '150,170p' cli/commands/run/hook.goLength of output: 713
Verification Complete: All instances updated
After a thorough check, we didn't find any lingering references to RunShellCommandWithOutput. The file cli/commands/run/hook.go now correctly uses RunCommandWithOutput as expected, including on line 161. Everything looks good, and it seems that all necessary updates have been made.
internal/strict/controls/controls.go (1)
65-159: Well-structured controls implementation!Great job organizing the controls into lifecycle and stage categories. The structure is clean and the descriptions are clear. The use of constants for control names makes it easy to reference them consistently throughout the codebase.
cli/commands/hclfmt/action.go (1)
111-111: Good improvement in color handling!Using
opts.Logger.Formatter().DisabledColors()instead of direct flag access is a better approach. It ensures consistent color handling throughout the application.Also applies to: 155-155
test/integration_scaffold_test.go (2)
29-29: Nice modernization of temporary directory handling!Great switch to
t.TempDir(). This is the idiomatic way in Go to handle test directories, and it automatically handles cleanup.Also applies to: 40-40, 55-55, 69-69, 81-81, 92-92, 102-102, 114-114, 126-126, 145-145, 159-159, 173-173
31-31: Good standardization of command formats.The consistent use of
terragrunt scaffold --terragrunt-non-interactivemakes the test cases clearer and more maintainable.Also applies to: 42-42, 57-57, 71-71, 83-83, 94-94, 104-104, 116-116, 128-128, 147-147, 164-164, 183-183
internal/cli/app.go (4)
25-26: Nice addition of Examples field for better documentation!Adding examples to the CLI app will help users understand how to use the commands correctly.
159-159: Good improvement in command organization!Sorting commands before displaying them provides a more consistent and user-friendly experience.
79-80: Verify the impact of hiding help commands.While hiding help commands might clean up the interface, we should ensure users can still discover command usage information.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if help information is still accessible through other means rg -A 5 'func.*[Hh]elp'Length of output: 25397
Help Functionality Is Intact Despite Hidden Commands
Our investigation shows that although the app explicitly hides help commands from the interface (via HideHelp and HideHelpCommand in internal/cli/app.go), the underlying help functionality remains accessible. Several supporting points:
- The help-related functions, such as ShowAppHelp and ShowCommandHelp in internal/cli/help.go, are still defined and used by the CLI.
- Test cases (e.g., TestCommandHelpTemplate, TestTerragruntHelp, and TestTerraformHelp in cli/app_test.go and cli/help_test.go) confirm that help output is generated and available via alternative invocation paths.
- The hidden configuration cleans up the user-facing command list without disabling the help logic.
Based on the review and available test verifications, no impact on help discoverability is observed.
164-176: Enhanced error handling for undefined flags.Setting
ErrorOnUndefinedFlagto true improves the robustness of the CLI by failing fast when users provide invalid flags.Let's verify the error messages are user-friendly:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for error message handling related to undefined flags rg -A 3 'ErrorOnUndefinedFlag|undefined.*flag'Length of output: 4422
Enhanced Undefined Flags Error Handling Verified
We checked the repository and found that the ErrorOnUndefinedFlag setting is applied consistently (e.g., in internal/cli/app.go and several commands in cli/commands/*). Although our search didn’t extract the exact text of the error messages, the flag’s presence indicates that the CLI will fail fast on any unknown flags using the default error routines—which our tests (like in cli/help_test.go) confirm display clear, user‐friendly messages.
internal/cli/command_test.go (2)
9-10: Good move to internal packages.Moving from
pkgtointernalbetter encapsulates implementation details that shouldn't be used by external packages.
95-96: Consider using t.Parallel() consistently.The test is already using t.Parallel() at the top level, which is good for test performance. The pattern of capturing loop variables is also correctly implemented.
Also applies to: 101-103
test/integration_windows_test.go (1)
27-27: Good choice of temporary directory path for Windows.Using
C:\tmphelps avoid Windows' MAX_PATH limitations.cli/commands/hclfmt/action_test.go (1)
122-122: Better method naming with CloneWithConfigPath.The rename from
ClonetoCloneWithConfigPathmakes the function's purpose clearer and more explicit.cli/commands/run/version_check.go (2)
1-1: Package and import changes look good!The package rename from
terraformtorunand the switch to thetfpackage align well with the PR's objective of refactoring the command handling structure.Also applies to: 13-13
98-98: Verify the impact of cloning changes.Two notable changes here:
- Using
CloneWithConfigPathinstead ofClone- Using
tf.FlagNameVersioninstead of a string literalWhile these changes improve consistency, let's verify they don't introduce any regressions.
Also applies to: 115-115
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other uses of Clone vs CloneWithConfigPath to ensure consistency rg "Clone\(" -A 2Length of output: 5631
Let's take one more look by checking how CloneWithConfigPath is defined and how tf.FlagNameVersion is used. We'll run a script that searches for the definition of CloneWithConfigPath and flags where tf.FlagNameVersion appears:
🏁 Script executed:
#!/bin/bash # Search for the definition or usage of CloneWithConfigPath to see how it differs from Clone. rg "func .*CloneWithConfigPath" -A 15 echo "---------------------------------" # Search for usage of tf.FlagNameVersion to verify consistency and impact of the change. rg "tf\.FlagNameVersion" -A 5Length of output: 1318
Impact of Cloning Changes Verified
- The CloneWithConfigPath function clones the options and then updates the TerragruntConfigPath, WorkingDir, and Logger, which aligns with the intended behavior.
- Usage of tf.FlagNameVersion in the version check is consistent with our codebase and doesn't introduce regressions.
internal/strict/control_test.go (1)
123-241: Well-structured test implementation!Great job on:
- Using table-driven tests
- Running tests in parallel
- Creating reusable test helpers
- Clear test case organization
pkg/log/logger.go (2)
20-30: Good addition of logger configuration methods!The new methods provide better control over logger behavior:
Level()andSetLevel()for log level managementSetFormatter()andFormatter()for output formatting
333-353: Nice improvement to cloning behavior!The new implementation properly handles deep cloning of the logger, which prevents unexpected sharing of state between cloned instances.
cli/commands/aws-provider-patch/action.go (1)
15-15: Verify AWS provider patch functionality after package changes.The switch from
terraformtorunpackage looks good, but let's ensure the AWS provider patch functionality still works as expected.Also applies to: 25-27, 32-32
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for any remaining references to the old package rg "terraform\." --type goLength of output: 17177
AWS Provider Patch Functionality Looks Spot-On
The updated file (cli/commands/aws-provider-patch/action.go) now imports and uses the run package correctly. Our scan didn’t show any lingering "terraform." references in that file, suggesting that the migration is in place. Some "terraform." strings still exist elsewhere (mostly in tests and comments), but that’s expected and unrelated to the AWS provider patch changes.
test/integration_common_test.go (3)
27-27: LGTM! Package import updated for consistency.The import statement has been updated from
terraformtorunpackage, aligning with the broader refactoring effort.
62-63: LGTM! Logger formatter updated to use placeholders.The logger formatter has been updated to use the newer
NewKeyValueFormatPlaceholders()andSetDisabledColors(true)methods, which provide better formatting control.
377-380: LGTM! Function calls updated for package consistency.The
WrongTofuCommandandWrongTerraformCommandcalls have been updated to use therunpackage instead ofterraform, maintaining consistency with the new package structure.cli/commands/validate-inputs/action.go (2)
17-18: LGTM! Package imports updated for consistency.The imports have been updated to use the new
runandtfpackages, aligning with the codebase reorganization.Also applies to: 20-21
27-28: LGTM! Function calls and variable references updated.All references to the old
terraformpackage have been correctly updated to use the newrunandtfpackages.Also applies to: 33-34, 179-180
cli/commands/run/download_source.go (3)
1-1: LGTM! Package renamed and experiment evaluation improved.The package has been renamed to
runand the experiment evaluation now uses the more explicitopts.Experiments.Evaluatemethod.Also applies to: 38-38
40-41: LGTM! Function signatures updated for consistency.All function signatures have been updated to use
tf.Sourceinstead ofterraform.Source, maintaining consistency with the new package structure.Also applies to: 88-88, 169-169, 208-208, 259-259, 279-279
250-252: LGTM! Registry getter updated to use new package.The registry getter implementation has been updated to use the
tfpackage, consistent with other changes.remote/remote_state.go (1)
112-112: LGTM! Simplified log message.The change makes the log message more maintainable by removing implementation-specific details.
configstack/running_module.go (2)
136-136: LGTM! Method name better reflects its purpose.The change from
ClonetoCloneWithConfigPathmakes the method's functionality more explicit.
145-146: LGTM! Package reference update.The change from
terraformtotfpackage aligns with the broader refactoring effort.test/integration_destroy_test.go (2)
78-78: LGTM! Updated type assertion.The change from
terraform.ModuleIsProtected{}torun.ModuleIsProtected{}aligns with the package reorganization.
269-271: LGTM! Added test skip condition.Good addition of a skip condition for tests that can't run with provider cache enabled.
test/integration_hooks_test.go (1)
248-248: Nice refactor using the helper function!Moving from a direct environment variable check to
helpers.IsTerragruntProviderCacheEnabled(t)improves maintainability by centralizing the provider cache check logic.cli/commands/scaffold/action.go (3)
14-14: Package import update aligns with codebase refactoringThe change from
terraformtotfpackage is part of a broader effort to consolidate and streamline the codebase.
182-182: Improve error message clarityThe warning messages about flag conflicts are now more specific about which flag is being ignored.
Also applies to: 188-188
226-226: Consistent use of new tf packageAll function calls have been updated to use the new
tfpackage instead ofterraform. This maintains consistency across the codebase.Also applies to: 307-307, 345-345, 365-365, 416-416
test/integration_debug_test.go (2)
59-59: Updated to use new tf packageFunction call updated to use
tf.RunCommandinstead ofshell.RunTerraformCommand, maintaining consistency with the package refactoring.
183-183: Better test directory managementUsing
t.TempDir()instead of manual directory creation improves test reliability as cleanup is handled automatically.Also applies to: 293-293
test/integration_functions_test.go (2)
223-223: More precise test assertionThe regex pattern now includes the test function name, making the repository root path validation more specific and reliable.
285-285: Command update in testChanged from
terragrunt versiontoterragrunt planfor a more comprehensive test of the functionality.cli/commands/run/action_test.go (1)
1-1: LGTM! Package rename and function call updates look good.The changes consistently update the package name from
terraform_testtorun_testand all related function calls fromterraform.*torun.*. This aligns with the broader refactoring effort mentioned in the PR objectives.Also applies to: 10-10, 80-80, 126-126, 156-156, 173-173, 190-190, 207-207, 224-224, 241-241, 253-253, 315-315, 432-432
config/config_partial.go (2)
267-267: LGTM! Improved error handling.The error is now properly wrapped with
errors.New, which provides better error context and maintains consistency with the error handling patterns in the codebase.
459-467: LGTM! Well-structured strict controls implementation.The new implementation:
- Filters controls by name using a clear, dedicated method
- Adds proper logging context
- Includes a helpful warning message when inputs are skipped
configstack/module.go (1)
14-14: LGTM! Package references consistently updated.All references to the
terraformpackage have been properly updated to use the newtfpackage, including:
- Import statement
- Command name constants
- File path constants
Also applies to: 105-105, 109-109, 117-117, 122-122
configstack/running_module_test.go (1)
18-18: LGTM! Better options cloning with config path.Updated to use
CloneWithConfigPathinstead ofClone, which ensures the configuration path is properly maintained during the cloning process.test/integration_download_test.go (1)
148-148: LGTM! Error type assertions updated for package refactoringThe changes correctly update the error type assertions from the old
terraformpackage to the newrunpackage, aligning with the broader refactoring effort.Also applies to: 485-485
engine/engine.go (1)
507-510: LGTM! Better encapsulation of logging functionalityThe changes improve the code design by accessing logging configuration through the logger interface rather than directly from options. This is a cleaner approach that keeps logging concerns properly encapsulated.
test/helpers/package.go (3)
81-85: Nice improvement using t.TempDir()Using
t.TempDir()instead ofos.MkdirTempis a better approach as it:
- Automatically cleans up temporary directories after the test
- Is the idiomatic way to handle test directories in Go
Also applies to: 99-100, 111-112
722-736: Good enhancement to command argument handlingThe improved command argument handling now correctly preserves built-in commands that appear after
--. This makes the test helper more robust when dealing with complex command structures.
898-913: Well-structured provider cache check functionThe new
IsTerragruntProviderCacheEnabledfunction is well implemented with:
- Clear environment variable checking logic
- Proper error handling with
require.NoError- Early returns for optimization
test/integration_serial_test.go (1)
701-704: LGTM! Better path handling with symlink resolutionGood addition of symlink resolution using
filepath.EvalSymlinks. This makes the tests more robust when dealing with different filesystem setups.configstack/stack.go (4)
19-24: LGTM! Clean package restructuring.The import path changes align with the package reorganization, moving from
terraformtotfand updating credential handling paths.
191-201: LGTM! Consistent command name updates.Command name references have been properly updated to use the new
tfpackage, maintaining the same functionality while following the new package structure.
560-563: LGTM! More explicit configuration path handling.The switch from
ClonetoCloneWithConfigPathmakes the configuration path handling more explicit and clearer in intent.
733-736: LGTM! Consistent configuration path handling.The change to
CloneWithConfigPathmaintains consistency with the new approach to configuration path handling throughout the codebase.cli/commands/run/action.go (3)
1-1: LGTM! Clean package reorganization.The package has been renamed from
terraformtorunwith appropriate import path updates, improving the overall code organization.Also applies to: 15-20
92-94: LGTM! Consistent command execution updates.Command execution has been properly updated to use the new
tfpackage, maintaining the same functionality while following the new package structure.
412-430: LGTM! Improved type safety.The parameter type change from
[]stringtocli.ArgsinShouldCopyLockFileimproves type safety while maintaining the same functionality.config/config_helpers.go (3)
452-458: Consider improving readability by splitting the code.This section could be more readable if split into multiple lines, as suggested in a previous review.
- allControls := ctx.TerragruntOptions.StrictControls - rootTGHCLControl := allControls.FilterByNames(controls.RootTerragruntHCL) - logger := log.ContextWithLogger(ctx, ctx.TerragruntOptions.Logger) - - if err := rootTGHCLControl.Evaluate(logger); err != nil { + controls := ctx.TerragruntOptions.StrictControls + rootTGHCLControl := controls.FilterByNames(controls.RootTerragruntHCL) + logger := log.ContextWithLogger(ctx, ctx.TerragruntOptions.Logger) + + if err := rootTGHCLControl.Evaluate(logger); err != nil {
594-596: LGTM! Clearer symlink handling.The changes make symlink handling more explicit by using the
experiment.Symlinksfeature flag.
379-382: LGTM! Consistent command execution updates.Command execution has been properly updated to use
shell.RunCommandWithOutput, maintaining the same functionality while following the new package structure.config/dependency.go (4)
27-30: Import paths updated to reflect new package structureThe import paths have been updated to use the new
run/credspackage instead of the oldterraform/credspackage, which aligns with the broader refactoring effort.
35-35: Import path updated fromterraformtotfThe import path has been updated to use the new
tfpackage, which is part of the codebase reorganization.
818-820: Experiment evaluation simplifiedThe code now directly evaluates the experiment flag from
opts.Experimentsinstead of using an intermediate variable.
846-847: Method calls updated to usetfpackageThe code has been updated to use the new
tfpackage methods:
RunCommandWithOutputinstead ofRunTerraformCommandWithOutputRunCommandinstead ofRunTerraformCommandThis change maintains consistency with the new package structure.
Also applies to: 953-954, 1139-1139
config/config_test.go (1)
21-22: Logger formatter configuration updatedThe logger formatter initialization has been updated to:
- Use
NewKeyValueFormatPlaceholdersinstead ofNewKeyValueFormat- Use
SetDisabledColorsinstead ofDisableColorsThese changes align with updates to the logging system across the codebase.
configstack/stack_test.go (3)
16-16: Import path updated fromterraformtotfThe import path has been updated to use the new
tfpackage, which is part of the codebase reorganization.
67-67: Command name references updated to usetfpackageThe code has been updated to use command name constants from the
tfpackage instead of theterraformpackage:
tf.CommandNameApplyinstead ofterraform.CommandNameApplytf.CommandNameDestroyinstead ofterraform.CommandNameDestroyAlso applies to: 92-92
149-149: Stack initialization updated to use mockOptionsThe code has been updated to use
mockOptionsconsistently when initializing stacks instead of creating new options instances inline. This improves test consistency and maintainability.Also applies to: 1131-1131, 1155-1155, 1185-1185
config/config.go (3)
42-42: Good addition of a descriptive constant name.The new constant
RecommendedParentConfigNamemakes it clear what the preferred filename is for parent configurations.
92-92: Improved logger color handling.The change to use
opts.Logger.Formatter().DisabledColors()is more idiomatic and provides better encapsulation of logger formatting.
680-682: Better experiment evaluation pattern.The change to use
Evaluate()method provides a more structured way to handle feature experiments.test/fixtures/exec-cmd/app/main.tf (1)
1-3: Add description for the variable.While the variable definition is correct, it would be helpful to add a description field to document its purpose.
variable "foo" { type = string + description = "Add a description of what this variable is used for" }Also, according to the AI summary, there should be a
barvariable defined here as well, but it's missing from the code.Makefile (1)
41-43: Update GolangCI-Lint Version:
The install target now uses v1.63.4 instead of the old v1.61.0. This update helps keep our linting up-to-date with the latest improvements and fixes. Make sure you run the tests after this update to confirm there are no unexpected side effects.docs/_docs/02_features/10-auto-init.md (1)
32-34: Rename Auto-Init Flags for Clarity:
The documentation now instructs users to disable Auto-Init using the new--no-auto-initflag and theTG_NO_AUTO_INITenvironment variable instead of the old, longer versions. This change simplifies the interface and aligns with our naming updates elsewhere.docs/_docs/04_reference/11-terragrunt-cache.md (1)
38-38: Update Cache Environment Variable:
The replacement ofTERRAGRUNT_DOWNLOADwithTG_DOWNLOAD_DIRin the docs streamlines our environment variable naming, making it shorter and more consistent with other changes. This is clear and improves our overall documentation coherence..golangci.yml (1)
82-83: DisablerecvcheckLinter with Context:
Disablingrecvcheckdue to the known issue (raeperd/recvcheck#7) is well-documented with the inline comment. This follows the earlier discussions and rationale provided in previous reviews.docs/_docs/04_reference/01-configuration.md (1)
32-35: Streamline Config Option Naming:
The revised list now correctly refers to the command-line option as--configand the environment variable asTG_CONFIG, replacing the longer forms. This makes the docs more concise and consistent throughout.docs/_docs/06_migration_guides/03-cli-redesign.md (6)
1-11: Front Matter and Metadata Setup
The front matter is clear, well-structured, and sets the proper context for the migration guide. It aligns nicely with the documentation style used elsewhere.
21-27: WIP Note for the NewrunCommand
The note flagging the introduction of the newruncommand is clear and helps users identify features under development. Since similar feedback has been mentioned in previous reviews, this is tagged as duplicate.
32-48: Clear Examples for Flag Renaming
The “Before” and “After” examples for removing theterragrunt-prefix from flags are straightforward and illustrative. They make it easy for users to see exactly what has changed.🧰 Tools
🪛 LanguageTool
[style] ~34-~34: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact flag withterragrunt-removed from th...(SAME_EXACT)
[style] ~36-~36: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ragrunt-non-interactiveflag, you will need to switch to the [--non-interactive`](/do...(REP_NEED_TO_VB)
68-84: Environment Variable Update Clarity
The step-by-step example showing the update fromTERRAGRUNT_NON_INTERACTIVEtoTG_NON_INTERACTIVEis very clear. It adheres to the new naming conventions and provides a smooth migration path.🧰 Tools
🪛 LanguageTool
[style] ~70-~70: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact environment variable withTERRAGRUNT_...(SAME_EXACT)
104-123: Introducing therunCommand
The section on using the newruncommand is detailed and instructive. The examples make it clear how the command should be invoked, supporting both explicit and shortcut usages.
124-132: Continued Support for Legacy Shortcuts
The explanation regarding which commands will continue to support shortcuts (and which won’t) is very helpful. It reassures users that an element of backward compatibility remains while transitioning to the new structure.docs/_docs/04_reference/06-experiments.md (3)
1-13: Experiment Documentation Front Matter
The front matter for the experiments document is concise and sets up the context effectively. It’s clear that this document will help users understand and provide feedback on experimental features.
70-71: Includingcli-redesignin Experiments List
Adding the[cli-redesign](#cli-redesign)entry to the experiments list is a nice touch. It immediately signals to users that the CLI overhaul is an active part of the experimental feature set.
119-157: Detailed Breakdown ofcli-redesignExperimental Features
The section detailing what thecli-redesignexperiment encompasses—including the newrunandexeccommands and the stabilization criteria—is well-organized and informative. It provides a clear roadmap for users.docs/_docs/04_reference/10-log-formatting.md (4)
27-42: Concise Log Format Examples
The examples showing basic placeholders like%time %level %msgand how to escape characters are clear and easy to follow. They serve as an excellent quick reference for users looking to customize logging.
95-102: Placeholder Formatting with Options
The snippet demonstrating placeholder formatting options (e.g.format='Y-m-d H:i:sv') is well-documented and clearly illustrates the flexibility you offer in log customization.
111-118: Escaping Parentheses Explanation
The example that shows how to escape parentheses using%time()(plain-text)is very instructive. It neatly highlights a common pitfall, making it easier for users to manage formatting quirks.
139-146: Unnamed Placeholder Example Clarity
The demonstration on how to use an unnamed placeholder for color formatting effectively shows advanced customization options without overcomplicating the syntax. Great job!docs/_docs/02_features/12-provider-cache-server.md (2)
47-53: Provider Cache Directory Example Clarity
The explanation on how to reuse an existing provider cache directory via the--provider-cache-dirflag is clear and detailed. The code snippet immediately following reinforces the written guidance well.
130-137: Configuring Custom Provider Cache Settings
The examples provided to configure the provider cache server (hostname, port, token) are succinct and clear. They give a good sense of how to manually override the defaults.docs/_docs/02_features/01-units.md (4)
189-194: Updated Flag Naming for Local Source Override
Changing the flag from--terragrunt-sourceto--sourcein the local development example is spot-on. It’s a concise update that fits seamlessly with the new naming conventions.
210-213: Flag Update for Forcing Redownloads
Replacing--terragrunt-source-updatewith--source-updateis clear and improves consistency across documentation. The instructions remain straightforward and user friendly.🧰 Tools
🪛 LanguageTool
[uncategorized] ~212-~212: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rragrunt with the--source-updateflag and it’ll delete the tmp folder, download t...(COMMA_COMPOUND_SENTENCE)
231-239: Guidance on Using Relative File Paths
The advice on using absolute paths for CLI arguments and leveragingget_terragrunt_dir()for configuration files is excellent. It addresses common pitfalls with temporary directories clearly and effectively.
352-360: Utilizing Include Blocks for DRY Configurations
The example demonstrating how to use anincludeblock to pull in anenv.hclfile is practical and shows how to keep configurations DRY across multiple units.🧰 Tools
🪛 LanguageTool
[style] ~358-~358: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ludeblock tells Terragrunt to use the exact same Terragrunt configuration from theenv....(EN_WORDINESS_PREMIUM_EXACT_SAME)
docs/_docs/01_getting-started/04-terminology.md (3)
51-56: Root Configuration File Renaming Clarification
The updated section now clearly explains the recommended use of a single “root.hcl” file at the repository root instead of “terragrunt.hcl.” This makes the convention easier to understand and aligns well with the deprecation notice. Nice and clear!
163-163: Run Queue Flag Name Update
The revision to mention [--queue-include-dir] and [--queue-exclude-dir] instead of the old “terragrunt-” prefixed flags is spot on. The new names are intuitive and the link reference appears to be in order—just a quick double-check of the hyperlink destination would be ideal.
214-214: Dependency Output Fetching Update
The explanation for using [--dependency-fetch-output-from-state] is now easier to follow. Labeling it as experimental is a great call-out, so users know to expect further changes in the future..circleci/config.yml (1)
521-523: Updated Test Command Path
Switching the test command’s source path from “./terraform/getproviders/.” to “./tf/getproviders/.” is a clear improvement and consistent with the other directory restructuring changes. Everything looks good here.docs/_docs/04_reference/03-built-in-functions.md (1)
830-833: Updated Source CLI Flag Documentation
The documentation for get_terragrunt_source_cli_flag now references the streamlined CLI flag “--source” and the environment variable “TG_SOURCE” instead of the older versions. This change is very clear and will help users when setting up hooks or custom flags for local development.docs/_docs/04_reference/04-config-blocks-and-attributes.md (6)
1395-1400: Feature Flag Environment Variable Examples Updated
The examples now demonstrate setting feature flags using TG_FEATURE with comma-separated values (e.g., run_hook=true,string_flag=dev). This update is both clear and consistent with the new naming conventions—great job!
1771-1774: Updated IAM Role Precedence Documentation
The revised section now specifies that the CLI option “--iam-assume-role” and the environment variable “TG_IAM_ASSUME_ROLE” take precedence over file settings. This clarity makes it easier for users to know where to override values if needed.
1789-1791: Updated IAM Assume Role Duration Precedence
The documentation now clearly states that the CLI “--iam-assume-role-duration” and the env variable “TG_IAM_ASSUME_ROLE_DURATION” determine the STS session duration. This provides great clarity on how the value is set.
1804-1806: Updated IAM Assume Role Session Name Precedence
The guidance now correctly documents that the session name is controlled via the CLI flag “--iam-assume-role-session-name” and the env variable “TG_IAM_ASSUME_ROLE_SESSION_NAME.” It’s straightforward and in line with our new naming approach.
1844-1847: Clarified IAM Web Identity Token Usage
The updated example now shows how to supply the iam_web_identity_token using a file path, which is particularly helpful for CI/CD setups that use AssumeRoleWithWebIdentity. This clarification makes the use case easier to follow.
1854-1856: Terraform Binary Precedence Documentation Update
The section now accurately outlines that the Terraform binary path is determined first by the “--tf-path” CLI option, then by the “TG_TF_PATH” env variable, and lastly by the configurations in terragrunt.hcl files. This layered explanation is clear and very useful for users needing to override the default binary.
| func ContextWithCache(ctx context.Context) context.Context { | ||
| return context.WithValue(ctx, RunCmdCacheContextKey, NewCache[string](runCmdCacheName)) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🛠️ Refactor suggestion
🧩 Analysis chain
Add documentation and nil context check.
The function looks good but could use some improvements:
Consider these enhancements:
+// ContextWithCache creates a new context with a string-based cache instance.
+// It panics if the provided context is nil.
func ContextWithCache(ctx context.Context) context.Context {
+ if ctx == nil {
+ panic("nil context provided to ContextWithCache")
+ }
return context.WithValue(ctx, RunCmdCacheContextKey, NewCache[string](runCmdCacheName))
}Also, could you verify if NewCache[string] is properly documented in its implementation?
🏁 Script executed:
#!/bin/bash
# Search for NewCache implementation and its documentation
ast-grep --pattern 'func NewCache[$_]($_) $_'Length of output: 376
Enhance ContextWithCache and NewCache Documentation
We've verified that the NewCache implementation in internal/cache/cache.go (lines 24-30) lacks any documentation and that ContextWithCache doesn't check for nil contexts. Consider adding a nil check and proper comment headers for both functions.
- Add a nil context check in ContextWithCache (as proposed in your refactor diff).
- Provide clear documentation for NewCache to clarify its purpose and usage.
| func (cloner *Cloner[T]) cloneSlice(src reflect.Value) reflect.Value { | ||
| size := src.Len() | ||
| dst := reflect.MakeSlice(src.Type(), size, size) | ||
|
|
||
| for i := range size { | ||
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | ||
| dst.Index(i).Set(val) | ||
| } | ||
| } | ||
|
|
||
| return dst | ||
| } |
There was a problem hiding this comment.
Bug: "for i := range size" won't compile.
In Go, you can't iterate over an integer directly with range. You likely meant:
(for slices)
» for i := 0; i < size; i++ { … }
This is a build blocker.
- for i := range size {
+ for i := 0; i < size; i++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (cloner *Cloner[T]) cloneSlice(src reflect.Value) reflect.Value { | |
| size := src.Len() | |
| dst := reflect.MakeSlice(src.Type(), size, size) | |
| for i := range size { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| return dst | |
| } | |
| func (cloner *Cloner[T]) cloneSlice(src reflect.Value) reflect.Value { | |
| size := src.Len() | |
| dst := reflect.MakeSlice(src.Type(), size, size) | |
| for i := 0; i < size; i++ { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| return dst | |
| } |
There was a problem hiding this comment.
Please stop raising issues like this. This is newer golang syntax that's perfectly valid. Golang supports this type of iteration.
There was a problem hiding this comment.
🧩 Analysis chain
@yhakbar I apologize for the incorrect warning. You are absolutely right - I should not have flagged this as an issue. Thank you for the correction.
🌐 Web query:
Can you iterate over an integer with range in Go? What versions support this?
💡 Result:
Yes, you can iterate over an integer using `range` in Go starting with **version 1.22** (released February 2024) and later[5][8]. This feature simplifies common loops by allowing direct iteration over integer values.
### How It Works
The syntax `for i := range n` behaves like a traditional `for` loop:
```go
// Equivalent to: for i := 0; i < n; i++
for i := range n {
// Use i (0 ≤ i < n)
}
```
- **Basic usage**:
```go
for i := range 5 {
fmt.Println(i) // Prints 0-4
}
```
- **Repeat without tracking iteration**:
```go
for range n {
fmt.Println("Hello") // Prints "Hello" n times
}
```
### Version Support
- **Go 1.22**: Introduced basic integer ranging[5]
- **Go 1.23**: Enhanced with additional iterator features while maintaining backward compatibility[2][8]
### Key Details
- Works identically to classic `for i := 0; i < n; i++` loops[2][9]
- Avoids accidental variable capture issues due to per-iteration variable creation[5]
- Commonly replaces ~50% of traditional `for` loops in real-world code[2]
For example:
```go
package main
import "fmt"
func main() {
// Iterate over integers (Go ≥1.22)
for i := range 3 {
fmt.Println(10 - i) // Prints 10,9,8
}
}
```
This feature provides cleaner syntax while maintaining performance parity with traditional loops[5][8].
Citations:
- 1: https://go.dev/blog/range-functions
- 2: https://eli.thegreenplace.net/2024/ranging-over-functions-in-go-123/
- 3: https://gobyexample.com/range-over-iterators
- 4: https://www.scaler.com/topics/golang/range-loop-in-golang/
- 5: https://tip.golang.org/doc/go1.22
- 6: https://www.freecodecamp.org/news/iteration-in-golang/
- 7: https://groups.google.com/g/golang-nuts/c/7J8FY07dkW0
- 8: https://forum.golangbridge.org/t/for-range-with-ints/36975
- 9: https://eli.thegreenplace.net/2023/preview-ranging-over-functions-in-go/
- 10: https://youtrack.jetbrains.com/issue/GO-16398/support-range-over-integer-for-go-1.22
@yhakbar I apologize for my previous response. You are correct - this is valid Go syntax introduced in Go 1.22 (February 2024). I should have verified the version support before commenting. Thank you for helping me improve my knowledge.
✏️ Learnings added
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3723
File: internal/cloner/cloner.go:163-0
Timestamp: 2025-02-06T21:13:51.938Z
Learning: Go supports iterating over integers using range syntax (e.g. `for i := range n`). This is valid syntax and should not be flagged as an error.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3723
File: internal/cloner/cloner.go:163-0
Timestamp: 2025-02-06T21:13:51.938Z
Learning: Go 1.22 and later versions support iterating over integers using range syntax (e.g. `for i := range n`). This feature was introduced in February 2024 and provides a cleaner alternative to traditional `for i := 0; i < n; i++` loops.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func (cloner *Cloner[T]) cloneArray(src reflect.Value) reflect.Value { | ||
| size := src.Type().Len() | ||
| dst := reflect.New(reflect.ArrayOf(size, src.Type().Elem())).Elem() | ||
|
|
||
| for i := range size { | ||
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | ||
| dst.Index(i).Set(val) | ||
| } | ||
| } | ||
|
|
||
| return dst | ||
| } |
There was a problem hiding this comment.
Same iteration bug for arrays.
Same fix: replace "for i := range size" with a standard for loop.
- for i := range size {
+ for i := 0; i < size; i++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (cloner *Cloner[T]) cloneArray(src reflect.Value) reflect.Value { | |
| size := src.Type().Len() | |
| dst := reflect.New(reflect.ArrayOf(size, src.Type().Elem())).Elem() | |
| for i := range size { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| return dst | |
| } | |
| func (cloner *Cloner[T]) cloneArray(src reflect.Value) reflect.Value { | |
| size := src.Type().Len() | |
| dst := reflect.New(reflect.ArrayOf(size, src.Type().Elem())).Elem() | |
| for i := 0; i < size; i++ { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| return dst | |
| } |
There was a problem hiding this comment.
Please stop raising issues like this. This is newer golang syntax that's perfectly valid. Golang supports this type of iteration.
There was a problem hiding this comment.
@yhakbar I apologize for the incorrect suggestion. Thank you for pointing out that this is valid Go syntax. I will refrain from raising similar issues in the future.
✏️ Learnings added
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3723
File: internal/cloner/cloner.go:176-0
Timestamp: 2025-02-06T21:12:49.032Z
Learning: Using `for i := range n` is valid Go syntax for iterating over integers, where n is the upper bound (exclusive) of the range. This is equivalent to `for i := 0; i < n; i++` but uses newer Go syntax.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -16,7 +16,7 @@ import ( | |||
| "github.com/stretchr/testify/assert" | |||
| ) | |||
|
|
|||
| func TestWindowsRunShellCommandWithOutputInterrupt(t *testing.T) { | |||
| func TestWindowsRunCommandWithOutputInterrupt(t *testing.T) { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consistent renaming across test files 👌
The renaming from RunShellCommand to RunCommand aligns with the broader refactoring effort. The test logic remains solid.
Let's verify the naming consistency across the codebase:
Also applies to: 33-33
🏁 Script executed:
#!/bin/bash
# Search for any remaining instances of old naming
rg "RunShellCommand" --type goLength of output: 768
Naming inconsistency detected between Windows and Unix tests
The Windows test file (shell/run_cmd_windows_test.go) has been updated to use the new naming ("RunCommand"), but the search shows that the Unix test (shell/run_cmd_test.go) still defines a function as TestRunShellCommand. In addition, helper types in tf/context.go are still using the old "RunShellCommand" naming.
- Windows tests now use RunCommand.
- Unix tests (shell/run_cmd_test.go) continue using RunShellCommand.
- tf/context.go references RunShellCommandFunc in several places.
This discrepancy might lead to confusion or unexpected behavior during refactoring, so please confirm whether the Unix tests and associated context naming should be updated for complete consistency.
| func (val *boolVar) Set(str string) error { | ||
| if err := val.genericVar.Set(str); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if val.negative { | ||
| *val.genericVar.dest = !*val.genericVar.dest | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding test for negative flag behavior
The Set method's handling of negative flags is a nice feature, but it would be good to ensure it's well tested.
🏁 Script executed:
#!/bin/bash
# Check for existing tests of negative flag behavior
rg -l "TestBoolFlag.*Negative" test/Length of output: 38
I'm not seeing any evidence of tests specifically exercising the negative flag behavior. It’s unclear if these tests exist under a different name, or if they’re missing entirely. To get a clearer picture, let’s dig a bit deeper and search for any hints of tests referencing the bool flag (and, in particular, its negative behavior) within our test suite. I’m going to run a script that searches for both “TestBoolFlag” and any case-insensitive occurrence of “negative” in the tests folder.
This should help us decide whether the existing test suite already covers the negative flag behavior or if additional tests need to be added.
🏁 Script executed:
#!/bin/bash
echo "Searching for bool flag tests (TestBoolFlag...):"
rg -l "TestBoolFlag" test/
echo "-----------------------------"
echo "Searching for negative flag references (case-insensitive):"
rg -l -i "negative" test/Length of output: 385
Missing Dedicated Negative Flag Test
Our deeper search shows that while some tests (like those in test/integration_errors_test.go) mention “negative,” we didn’t find any dedicated test case (e.g., a TestBoolFlag…Negative) that explicitly verifies the negative flag behavior in boolVar’s Set method. This confirms that the negative flag logic isn’t being directly verified in the unit tests, so adding one would help ensure its correctness.
| @@ -162,7 +157,7 @@ func (p *stacksFileProvider) Get(dst string, u *url.URL) error { | |||
| file, err := os.Stat(src) | |||
|
|
|||
| if err != nil { | |||
| return errors.New(fmt.Errorf("source path error: %w", err)) | |||
| return errors.Errorf("source path error: %w", err) | |||
There was a problem hiding this comment.
Fix error wrapping syntax.
The %w verb is used incorrectly in the Errorf call. It should be used with fmt.Errorf, not directly with errors.Errorf.
- return errors.Errorf("source path error: %w", err)
+ return fmt.Errorf("source path error: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return errors.Errorf("source path error: %w", err) | |
| return fmt.Errorf("source path error: %w", err) |
| const maxIterations = 1000 | ||
|
|
||
| for range maxIterations { | ||
| cmdName := args.CommandName() | ||
|
|
||
| cmd := cmds.Get(cmdName) | ||
| if cmd == nil { | ||
| break | ||
| } | ||
|
|
||
| args = args.Remove(cmdName) | ||
| cmds = cmd.Subcommands | ||
| ctx = ctx.NewCommandContext(cmd, args) | ||
| } | ||
|
|
There was a problem hiding this comment.
Add safeguard against infinite loop.
The for range maxIterations loop could potentially run indefinitely if there's a cyclic command structure.
Add a counter to break the loop if it exceeds the maximum iterations:
const maxIterations = 1000
+var iterCount int
for range maxIterations {
+ iterCount++
+ if iterCount >= maxIterations {
+ return errors.New("exceeded maximum command nesting depth")
+ }
cmdName := args.CommandName()| DeprecatedLogLevelFlagName = "log-level" | ||
| DeprecatedLogDisableFlagName = "log-disable" | ||
| DeprecatedShowLogAbsPathsFlagName = "log-show-abs-paths" | ||
| DeprecatedLogFormatFlagName = "log-format" | ||
| DeprecatedLogCustomFormatFlagName = "log-custom-format" | ||
| DeprecatedNoColorFlagName = "no-color" | ||
| DeprecatedNonInteractiveFlagName = "non-interactive" | ||
| DeprecatedWorkingDirFlagName = "working-dir" | ||
| DeprecatedStrictModeFlagName = "strict-mode" | ||
| DeprecatedStrictControlFlagName = "strict-control" | ||
| DeprecatedExperimentModeFlagName = "experiment-mode" | ||
| DeprecatedExperimentFlagName = "experiment" |
There was a problem hiding this comment.
Redundant flag name declarations.
The deprecated flag names are identical to their current counterparts, which makes the deprecation ineffective. This could lead to confusion as there's no actual transition path for users.
Consider using different names for the deprecated flags to create a clear migration path. For example:
-const (
- DeprecatedLogLevelFlagName = "log-level"
- DeprecatedLogDisableFlagName = "log-disable"
- DeprecatedShowLogAbsPathsFlagName = "log-show-abs-paths"
- DeprecatedLogFormatFlagName = "log-format"
- DeprecatedLogCustomFormatFlagName = "log-custom-format"
- DeprecatedNoColorFlagName = "no-color"
- DeprecatedNonInteractiveFlagName = "non-interactive"
- DeprecatedWorkingDirFlagName = "working-dir"
- DeprecatedStrictModeFlagName = "strict-mode"
- DeprecatedStrictControlFlagName = "strict-control"
- DeprecatedExperimentModeFlagName = "experiment-mode"
- DeprecatedExperimentFlagName = "experiment"
-)
+const (
+ DeprecatedLogLevelFlagName = "old-log-level"
+ DeprecatedLogDisableFlagName = "old-log-disable"
+ DeprecatedShowLogAbsPathsFlagName = "old-log-show-abs-paths"
+ DeprecatedLogFormatFlagName = "old-log-format"
+ DeprecatedLogCustomFormatFlagName = "old-log-custom-format"
+ DeprecatedNoColorFlagName = "old-no-color"
+ DeprecatedNonInteractiveFlagName = "old-non-interactive"
+ DeprecatedWorkingDirFlagName = "old-working-dir"
+ DeprecatedStrictModeFlagName = "old-strict-mode"
+ DeprecatedStrictControlFlagName = "old-strict-control"
+ DeprecatedExperimentModeFlagName = "old-experiment-mode"
+ DeprecatedExperimentFlagName = "old-experiment"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DeprecatedLogLevelFlagName = "log-level" | |
| DeprecatedLogDisableFlagName = "log-disable" | |
| DeprecatedShowLogAbsPathsFlagName = "log-show-abs-paths" | |
| DeprecatedLogFormatFlagName = "log-format" | |
| DeprecatedLogCustomFormatFlagName = "log-custom-format" | |
| DeprecatedNoColorFlagName = "no-color" | |
| DeprecatedNonInteractiveFlagName = "non-interactive" | |
| DeprecatedWorkingDirFlagName = "working-dir" | |
| DeprecatedStrictModeFlagName = "strict-mode" | |
| DeprecatedStrictControlFlagName = "strict-control" | |
| DeprecatedExperimentModeFlagName = "experiment-mode" | |
| DeprecatedExperimentFlagName = "experiment" | |
| const ( | |
| DeprecatedLogLevelFlagName = "old-log-level" | |
| DeprecatedLogDisableFlagName = "old-log-disable" | |
| DeprecatedShowLogAbsPathsFlagName = "old-log-show-abs-paths" | |
| DeprecatedLogFormatFlagName = "old-log-format" | |
| DeprecatedLogCustomFormatFlagName = "old-log-custom-format" | |
| DeprecatedNoColorFlagName = "old-no-color" | |
| DeprecatedNonInteractiveFlagName = "old-non-interactive" | |
| DeprecatedWorkingDirFlagName = "old-working-dir" | |
| DeprecatedStrictModeFlagName = "old-strict-mode" | |
| DeprecatedStrictControlFlagName = "old-strict-control" | |
| DeprecatedExperimentModeFlagName = "old-experiment-mode" | |
| DeprecatedExperimentFlagName = "old-experiment" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
cli/commands/graph/action.go (1)
31-40: Add fallback for git root directory.The current implementation might fail if the working directory isn't in a git repository. Consider adding a fallback to use the working directory if git root can't be determined.
if rootDir == "" { gitRoot, err := shell.GitTopLevelDir(ctx, opts, opts.WorkingDir) if err != nil { - return err + opts.Logger.Warnf("Failed to determine git root: %v. Using working directory.", err) + rootDir = opts.WorkingDir + return nil } rootDir = gitRoot }cli/commands/render-json/action.go (1)
110-110: Fix error handling inmarshalCtyValueJSONWithoutTypeThere's a bug in the error handling. The function is always wrapping the error from
json.Marshaleven when it's nil.Here's the fix:
- return jsonBytes, errors.New(err) + if err != nil { + return nil, errors.New(err) + } + return jsonBytes, nil
🧹 Nitpick comments (118)
configstack/log_test.go (1)
25-26: LGTM! Nice API improvements 🎨The switch to
NewKeyValueFormatPlaceholders()andSetDisabledColors(true)makes the code clearer and more maintainable. The new method names better express their intent.Consider adding a brief comment explaining why colors are disabled in this test. While it's likely for consistent test output, making this explicit would help future maintainers:
formatter := format.NewFormatter(format.NewKeyValueFormatPlaceholders()) +// Disable colors to ensure consistent test output across different environments formatter.SetDisabledColors(true)shell/run_cmd_test.go (1)
80-82: Consider adding cache invalidation test.The cache initialization and basic usage tests look good. However, we might want to add a test case for cache invalidation to ensure the cache behaves correctly when the context is canceled or when we need to clear it.
Here's a suggested test case to add:
func TestGitLevelTopDirCacheInvalidation(t *testing.T) { t.Parallel() ctx := context.Background() ctx = cache.ContextWithCache(ctx) c := cache.ContextCache[string](ctx, cache.RunCmdCacheContextKey) // Test cache invalidation terragruntOptions, err := options.NewTerragruntOptionsForTest("") require.NoError(t, err) // Fill cache path1, err := shell.GitTopLevelDir(ctx, terragruntOptions, ".") require.NoError(t, err) assert.Len(t, c.Cache, 1) // Invalidate cache c.Clear() assert.Empty(t, c.Cache) // Verify cache is rebuilt path2, err := shell.GitTopLevelDir(ctx, terragruntOptions, ".") require.NoError(t, err) assert.Equal(t, path1, path2) assert.Len(t, c.Cache, 1) }docs/_docs/04_reference/06-experiments.md (2)
125-125: Brief Overview of CLI Redesign Features.
The line "Enable features from the CLI Redesign RFC." is concise; you might later consider expanding this section as the implementation matures.
150-150: Introducing thehclCommand.
This checklist item indicates the deprecation and replacement of several commands with the newhclcommand. Consider adding a migration note to help users transition smoothly from the legacy commands.main.go (1)
50-52: Trace-level logging for the error stack is handy.
This is an excellent debug aid, although be mindful that large stack traces might clutter logs. Consider toggling them only for higher verbosity.internal/strict/controls/depreacted_env_var.go (1)
1-1: Watch out for the file name typo.
It appears "depreacted_env_var.go" is misspelled.Consider renaming it to "deprecated_env_var.go" for clarity:
git mv internal/strict/controls/depreacted_env_var.go internal/strict/controls/deprecated_env_var.gocli/commands/scaffold/command.go (1)
99-105: Consider reducing nesting.
If you’d like to keep things tidy, you could invert the condition and early return to reduce one level of indentation.internal/experiment/experiment.go (1)
142-149: Consider renaming the receiver to match the singular.Referring to a single experiment as “exps” might be a tad confusing. Something like “exp” might be clearer.
Example adjustment:
-func (exps Experiment) Evaluate() bool { +func (exp Experiment) Evaluate() bool { return exp.Enabled }internal/cli/bool_flag.go (1)
13-38: Spotted minor doc typo in the struct comments.Everything else is good, but “Destination ia a pointer” might need a quick fix.
Proposed diff:
- // Destination ia a pointer + // Destination is a pointershell/run_cmd.go (1)
21-28: Consider making SignalForwardingDelay configurable.
A 15-second hardcoded delay might not fit all use cases. Making it configurable (e.g., via CLI flag or environment variable) could improve flexibility.internal/cli/slice_flag.go (2)
70-78: Defaulting LookupEnvFunc
Smart fallback for environment lookups. Just be mindful of performance if you process huge environment values.
102-104: Exposing EnvVars
Matches the new multi-env pattern. Make sure docs are updated so users aren’t confused by plural vs. singular environment references.internal/cli/generic_flag.go (1)
123-123: Empty Reset
If you need future expansions to reinitialize variables, an actual reset might be helpful, but for now it’s a no-op you can safely keep.cli/flags/deprecated_flag.go (1)
14-15: Revise the doc comment for clarity.“DeprecatedFlags are multiple of DeprecatedFlag flags.” sounds awkward. Suggest rewording to indicate it's a slice of DeprecatedFlag pointers:
-// DeprecatedFlags are multiple of DeprecatedFlag flags. +// DeprecatedFlags is a collection of DeprecatedFlag instances.internal/cli/map_flag.go (1)
49-50: Consider the default for “DisableEnvVar.”If you want to guide users toward a recommended behavior, consider a default that best fits most use cases. Right now, it’s an open setting without explanation.
internal/cli/flag.go (3)
22-31: Consider clarifying the difference between FlagSetterFunc and FlagActionFunc.
These two function types serve distinct timing purposes during flag parsing. A brief example in the docstring clarifying how each fits into the overall parsing flow might reduce confusion and help new contributors.
57-60: Fix the docstring for "IsArgSet."
Line 59 says, “// IsSet returns true if the flag was by CLI arg.” but the method name is IsArgSet. Updating the doc to match the method name would prevent confusion.- // IsSet returns true if the flag was by CLI arg. + // IsArgSet returns true if the flag was set by CLI arg.
107-133: Reduce duplicated logic in EnvSet and Set methods.
Both methods share clearing the default and setting flags. You might introduce a helper to avoid subtle logical drift in the future.internal/strict/control.go (2)
85-105: Consider a more efficient duplicate check.
RemoveDuplicates currently uses a nested loop. For large numbers of controls, a set-based approach might improve performance. For typical usage, it’s likely fine, but if you expect bigger scale, it might be worth a refactor.
186-199: Extend the completed controls message for clarity.
When LogEnabled calls out completed controls, it might help add a direct suggestion for how to remove them if a user is not sure which file or location references them.cli/commands/run/download_source.go (2)
88-90: User-friendly logging.
Thanks for calling out the folder name when removing the temporary directory—this is super helpful for debugging. Consider similarly verbose logs in other destructive steps (like cleaning cached states) for parity.
169-170: Potential repeated scans for local content changes.
AlreadyHaveLatestCode does a local directory check plus version file logic. If symlinked or ephemeral storages are used, watch out for differences in real vs. cached content.cli/flags/global/flags.go (1)
123-132: Handle unknown log formats.
Your switch statement covers “bare” and “json” but silently ignores other formats. Consider adding a default case (or logging a helpful warning) to let users know if they use an unrecognized format.cli/app.go (1)
145-146: Typo in the variable name 'globaFlags'?
You spelled the variable “globaFlags” instead of “globalFlags.” This is purely cosmetic, but fixing it might reduce confusion.- globaFlags := global.NewFlags(opts, nil) + globalFlags := global.NewFlags(opts, nil)cli/commands/run/flags.go (4)
1-2: Consider adding high-level usage details or examples in package doc.
It might help future readers to have a brief doc comment describing the purpose of this package and the overall function of these flags.
15-130: Ensure consistent naming and documentation in the constants block.
This block of constants is quite extensive. You might want to double-check that each constant has a uniform naming style and that each usage string is concise yet descriptive.
139-157: Return an actionable response instead of an error for “under development” flags.
The flags “--all” and “--graph” return an error with a message that they’re under development. This might frustrate users. Consider gating them behind a hidden or experimental flag until they’re ready, or provide a more informative message guiding the user.
512-550: Add unit tests for newly introduced flags.
Verifying that each flag is processed correctly can help avoid regressions, especially with so many flags and deprecated aliases in play.Would you like help to generate a skeleton test file that exercises the parsing logic for each flag?
options/options.go (3)
16-21: Keep import statements minimal and well-commented.
The newest imports (cli, cloner, strict, controls) seem relevant, but be sure to remove or comment out any that aren’t strictly needed to keep things tidy.
367-377: Confirm references to strict controls, experiments, and feature flags.
These fields share the shadow copy approach too. If the code modifies them at runtime, consider whether each clone should truly share these references or if each clone should get its own copy.
590-604: Expose a simpler parameter override approach in CloneWithConfigPath.
Right now, you replace the config path and recalculate the working directory. If you need to override additional fields (e.g., environment variables, ephemeral directories), you might generalize this function or offer a second optional parameter signature.cli/commands/run/action.go (5)
102-102: CheckVersionConstraints call might warrant a user-friendly error.
Be sure that any version mismatch error is clear and instructive, possibly instructing the user to upgrade/downgrade.
348-365: Unify the hooking approach across before, after, and error hooks.
“RunActionWithHooks” is well-structured. Make sure the after hooks only run if relevant steps succeed so as not to overshadow legitimate errors.Need help drafting a test scenario that ensures hooks skip gracefully if the main action fails?
655-657: Optional improvement for auto-init checks.
Users who disable auto-init but still rely on Terragrunt to handle initialization might get confused. Maybe add a more explicit warning or link to docs about manual “terragrunt init.”
679-698: Applying consistent color or no-color flags.
The logic appends “-no-color” if present. Consider echoing a detail in logs to confirm the user that color output is disabled, for clarity.
799-816: Filtering environment variables.
This is well-organized – the same “First()” check is repeated, though. If you find yourself calling that a lot, consider a small helper function to keep it DRY.test/integration_test.go (6)
107-107: New fixture variable introduced.
Great that you're isolating tests in a separate fixture directory. Consider adding edge-case scenarios (e.g., nonexistent script).
119-162: Robust new test for exec command.
Your table-driven approach with two scenarios is a good start. Consider adding a negative case with a missing or failing script, just to confirm error handling.
550-550: Defining test cases for excludes file.
Good table approach. You might also validate if an excludes file is missing or malformed, for completeness.
570-570: Applying Terragrunt with excludes.
Might be valuable to verify partial or overlapping excludes for completeness.
572-572: Capturing command output.
This is straightforward, but you could also capture stderr if you need deeper error analysis.
1092-1092: Inclusive assertion for output or error text.
You could switch to require.Contains for a more direct check, if that helps.cli/commands/exec/options.go (1)
3-5: Add field documentation for better clarityThe
InDownloadDirfield would benefit from a comment explaining its purpose and when it should be set to true/false.Add documentation like this:
type Options struct { + // InDownloadDir determines whether the command should be executed in the download directory + // rather than the working directory InDownloadDir bool }internal/cache/context.go (1)
7-11: Consider making the cache name publicThe
runCmdCacheNamecould be useful in tests or for logging. Consider making it public if other packages might need to reference it.const ( RunCmdCacheContextKey ctxKey = iota - runCmdCacheName = "runCmdCache" + RunCmdCacheName = "runCmdCache" )internal/cli/exit_code.go (1)
4-7: Consider adding more standard exit codesWhile the basic success/error codes are good, consider adding more standard Unix exit codes for specific error cases (e.g., configuration errors, permission denied).
const ( ExitCodeSuccess ExitCode = iota ExitCodeGeneralError + ExitCodeConfigError + ExitCodePermissionDenied + ExitCodeInvalidArgument )pkg/log/context.go (2)
11-13: Consider documenting the exported functionWhile the function is straightforward, it would be helpful to add a godoc comment explaining its purpose and parameters.
+// ContextWithLogger returns a new context with the provided logger attached. func ContextWithLogger(ctx context.Context, logger Logger) context.Context {
15-23: Consider adding nil context checkWhile the implementation is solid, it might be worth adding a nil check for the context parameter to provide a better error message.
func LoggerFromContext(ctx context.Context) Logger { + if ctx == nil { + return nil + } if val := ctx.Value(loggerContextKey); val != nil {internal/strict/errors.go (1)
14-16: Consider enhancing error message readabilityThe error message could be more user-friendly by providing a clearer context.
func (err InvalidControlNameError) Error() string { - return "allowed control(s): " + err.allowedNames.String() + return "invalid control name provided - allowed control(s) are: " + err.allowedNames.String() }cli/commands/version/command.go (1)
19-21: Consider extracting exit code to a constantThe exit code
0could be more maintainable as a named constant, especially if we add more exit codes in the future.+const ( + successExitCode = 0 +) Action: func(ctx *cli.Context) error { - return cli.NewExitError(global.VersionAction(ctx, opts), 0) + return cli.NewExitError(global.VersionAction(ctx, opts), successExitCode) },internal/cli/funcs.go (1)
9-10: Consider enhancing the help function documentation 📚While the documentation is clear, it would be helpful to specify what types of errors might be returned and how they should be handled. This helps other developers implement help functions correctly.
Consider expanding the documentation like this:
-// HelpFunc is the action to execute when help needs to be displayed. +// HelpFunc is the action to execute when help needs to be displayed. +// It should return an error if help text cannot be generated or displayed. +// Common error cases include template parsing failures or I/O errors.internal/strict/controls/deprecated_command.go (1)
19-20: Found a small typo in the error messages 🔍There's a grammatical error in both the error and warning messages where "commands is" should be "command is".
Here's the fix:
- Error: errors.Errorf("The `%s` commands is no longer supported. Use `%s` instead.", command, newCommand), - Warning: fmt.Sprintf("The `%s` commands is deprecated and will be removed in a future version. Use `%s` instead.", command, newCommand), + Error: errors.Errorf("The `%s` command is no longer supported. Use `%s` instead.", command, newCommand), + Warning: fmt.Sprintf("The `%s` command is deprecated and will be removed in a future version. Use `%s` instead.", command, newCommand),cli/commands/info/command.go (1)
1-27: Looking good! Just one tiny suggestion to make it even better.The command implementation is clean and follows the project's patterns nicely. The error handling and help configuration are spot-on. 👍
One small enhancement would be to add some examples in the Usage description to help users get started quickly.
Consider expanding the Usage description like this:
- Usage: "List of commands to display Terragrunt settings.", + Usage: "List of commands to display Terragrunt settings.\n\n" + + "Examples:\n" + + " terragrunt info strict\n" + + " terragrunt info --help",internal/strict/view/writer.go (1)
11-15: Nice implementation of the Writer struct! Consider documenting the Render interface.The Writer struct is well-designed with clear separation of concerns. The embedded io.Writer and custom render field make for a clean implementation.
Could you add documentation for the Render interface? It would help other developers understand what methods they need to implement.
cli/commands/catalog/command.go (1)
16-21: Consider documenting the prefix parameter's purpose.The NewFlags function looks good with the flag filtering, but it would be helpful to document why the prefix parameter was added and how it affects flag behavior.
cli/commands/run/command_test.go (1)
13-58: Well-structured tests! Consider adding success cases.The table-driven tests are clean and cover error cases well. However, I notice we're only testing error scenarios.
Consider adding test cases for successful command execution to ensure both happy and error paths are covered.
internal/experiment/errors.go (1)
33-35: Great user-friendly error message with documentation link!The error message not only explains what's wrong but also guides users to the documentation. That's super helpful! 👍
Consider implementing the
Ismethod to supporterrors.Ischecks, which would make error comparison more idiomatic in Go.internal/cli/context.go (1)
56-62: Consider adding flag existence checkThe flag retrieval is clean, but it might be helpful to add a method to check if a flag exists before getting it.
+func (ctx *Context) HasFlag(name string) bool { + if ctx.Command != nil { + return ctx.Command.Flags.Has(name) + } + return false +}internal/cloner/clone.go (1)
38-44: Consider adding prefix validationIt might be helpful to validate that the provided prefixes are valid package paths.
func WithShadowCopyInversePkgPrefixes(prefixes ...string) Option { return func(opt *Config) { + for _, prefix := range prefixes { + if !isValidPkgPath(prefix) { + panic(fmt.Sprintf("invalid package path prefix: %s", prefix)) + } + } opt.shadowCopyInversePkgPrefixes = append(opt.shadowCopyInversePkgPrefixes, prefixes...) } } + +func isValidPkgPath(path string) bool { + return len(path) > 0 && !strings.Contains(path, " ") +}internal/strict/status.go (2)
29-30: Fix typo in commentThere's a small typo in the comment: "represetns" should be "represents".
-// Status represetns the status of the Control. +// Status represents the status of the Control.
41-53: Consider extracting ANSI color codes as constantsThe ANSI color codes could be defined as constants for better maintainability and reuse.
+const ( + greenColor = "\033[0;32m" + yellowColor = "\033[0;33m" + resetColor = "\033[0m" +) func (status Status) StringWithANSIColor() string { str := status.String() switch status { case ActiveStatus: - return "\033[0;32m" + str + "\033[0m" + return greenColor + str + resetColor case CompletedStatus, SuspendedStatus: - return "\033[0;33m" + str + "\033[0m" + return yellowColor + str + resetColor } return str }internal/strict/view/plaintext/template.go (2)
3-3: Consider adding error handling for nil fields in controlTemplateThe template assumes
.Descriptionand.Warningexist. Adding nil checks would make it more robust:-const controlTemplate = `{{ .Name }}{{ "\t" }}{{ .Status.StringWithANSIColor }}{{ "\t" }}{{ if .Description }}{{ .Description }}{{ else }}{{ .Warning }}{{ end }}` +const controlTemplate = `{{ .Name }}{{ "\t" }}{{ .Status.StringWithANSIColor }}{{ "\t" }}{{ if .Description }}{{ .Description }}{{ else if .Warning }}{{ .Warning }}{{ else }}N/A{{ end }}`
13-20: Consider breaking down the complex listTemplate for better maintainabilityThe template has multiple nested operations which make it hard to read and maintain. Consider splitting it into smaller, reusable templates.
+const categoryTemplate = `{{ .Name }}: + {{ $categoryControls := $.controls.FilterByCategories . }}{{ template "rangeControlsTemplate" $categoryControls }}` const listTemplate = ` - {{ $controls := .controls }}{{ $categories := $controls.GetCategories.FilterNotHidden.Sort }}{{ range $index, $category := $categories }}{{ if $index }} - {{ end }}{{ $category.Name }}: - {{ $categoryControls := $controls.FilterByCategories $category }}{{ template "rangeControlsTemplate" $categoryControls }} + {{ $controls := .controls }}{{ $categories := $controls.GetCategories.FilterNotHidden.Sort }}{{ range $index, $category := $categories }}{{ if $index }} + {{ end }}{{ template "categoryTemplate" $category }} {{ end }}{{ $noCategoryControls := $controls.FilterByCategories }}{{ if $noCategoryControls }} {{ template "rangeControlsTemplate" $noCategoryControls }} {{ end }} `internal/strict/view/plaintext/render.go (1)
56-58: Consider making tabwriter configuration customizableThe tabwriter configuration is hardcoded. Consider making it configurable through the Render struct:
+type TabConfig struct { + MinWidth int + TabWidth int + Padding int +} + +type Render struct { + tabConfig TabConfig +} + +func NewRender() *Render { + return &Render{ + tabConfig: TabConfig{ + MinWidth: 1, + TabWidth: 8, + Padding: 2, + }, + } +}pkg/log/formatter.go (1)
26-35: Enhance documentation with examplesThe Format method's documentation could be more helpful with concrete examples:
// Format takes an `Entry`. It exposes all the fields, including the default ones: // // * `entry.Data["msg"]`. The message passed from Info, Warn, Error .. // * `entry.Data["time"]`. The timestamp. // * `entry.Data["level"]. The level the entry was logged at. // // Any additional fields added with `WithField` or `WithFields` are also in // `entry.Data`. Format is expected to return an array of bytes which are then // logged to `logger.Out`. +// +// Example: +// entry := &Entry{ +// Data: Fields{ +// "msg": "Hello World", +// "time": time.Now(), +// "level": InfoLevel, +// }, +// } +// bytes, err := formatter.Format(entry)internal/cli/flags_test.go (1)
54-54: Remove unnecessary parentheses in Filter call.The parentheses around the slice argument are redundant when using the spread operator.
- actual := newMockFlags().Filter(([]string{"bar", "baz"})...) + actual := newMockFlags().Filter([]string{"bar", "baz"}...)cli/commands/terragrunt-info/action.go (1)
10-10: LGTM! Clean package migration.The migration from
terraformtorunpackage improves the command handling structure. Consider adding a comment explaining the role of therunpackage for better maintainability.import ( "context" "encoding/json" "fmt" "github.com/gruntwork-io/terragrunt/internal/errors" + // run package handles command execution and targeting "github.com/gruntwork-io/terragrunt/cli/commands/run" "github.com/gruntwork-io/terragrunt/config" "github.com/gruntwork-io/terragrunt/options" )Also applies to: 16-18
cli/commands/shortcuts.go (2)
33-71: Add function documentation to explain the shortcut command logic.The function
NewShortcutsCommandswould benefit from a doc comment explaining:
- What shortcuts are and how they work
- The relationship with the
runcommand- The deprecation handling logic
Add this documentation above the function:
+// NewShortcutsCommands creates CLI command shortcuts that map to their equivalent +// 'run' subcommands. For non-shortcut commands, it sets up deprecation notices +// directing users to use the new 'run' command format. func NewShortcutsCommands(opts *options.TerragruntOptions) cli.Commands {
16-31: Consider making shortcut commands configurable.The hardcoded list of shortcut commands could be moved to a configuration file or made configurable through options, making it easier to maintain and modify without code changes.
cli/commands/run/errors.go (1)
14-16: Consider adding more command examples.The error message could be more helpful by showing multiple common command examples.
- return "Missing terraform command (Example: terragrunt run plan)" + return "Missing terraform command (Examples: terragrunt run plan, terragrunt run apply)"internal/strict/category.go (2)
54-63: Simplify the Less implementation.The current implementation of
Lesscan be simplified by removing the empty string checks or handling them differently.Consider this cleaner implementation:
func (categories Categories) Less(i, j int) bool { - if len((categories)[j].Name) == 0 { - return false - } else if len((categories)[i].Name) == 0 { - return true - } - - return (categories)[i].Name < (categories)[j].Name + iName := categories[i].Name + jName := categories[j].Name + if iName == "" || jName == "" { + return iName != "" + } + return iName < jName }
77-83: Add examples to the Category struct documentation.The struct documentation would benefit from examples showing typical usage patterns.
Add examples like:
// Example: // category := &Category{ // Name: "deprecated", // Hidden: false, // }internal/cli/commands.go (1)
101-105: Consider adding a comment about sort stability.The Sort method should document whether the sort is stable or not, as this could be important for command ordering.
Add a comment like:
+// Sort returns `commands` in sorted order by `Name`. +// The sort is stable/unstable (depending on the underlying sort implementation). func (commands Commands) Sort() Commands {cli/commands/info/strict/command.go (1)
68-75: Consider adding context to the error message.When a control name isn't found, including the provided name in the error message would be helpful for debugging.
- return strict.NewInvalidControlNameError(controls.Names()) + return strict.NewInvalidControlNameError(name, controls.Names())cli/flags/prefix.go (1)
47-53: Consider adding prefix validation.While the string manipulation is clean, it might be good to add validation for the prefix values to prevent any unexpected characters.
Consider adding validation to ensure prefixes only contain alphanumeric characters and underscores:
func (prefix Prefix) EnvVar(name string) string { + for _, p := range prefix { + if !isValidPrefix(p) { + panic(fmt.Sprintf("invalid prefix: %s", p)) + } + } name = strings.Join(append(prefix, name), "_") return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) } +func isValidPrefix(p string) bool { + for _, r := range p { + if !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != '_' { + return false + } + } + return true +}cli/commands/render-json/command.go (1)
60-60: Consider separating flag groups.The flag combination
append(run.NewFlags(opts, nil), NewFlags(opts, prefix)...)might be clearer if grouped by functionality.Consider:
- Flags: append(run.NewFlags(opts, nil), NewFlags(opts, prefix)...).Sort(), + Flags: cli.Flags{}. + Append(run.NewFlags(opts, nil)...). + Append(NewFlags(opts, prefix)...). + Sort(),internal/cli/flags.go (1)
115-125: Consider adding documentation for WithSubcommandScope.While the implementation is clean, it would be helpful to add a comment explaining when and why flags should be allowed in subcommand scope.
+// WithSubcommandScope returns a new Flags instance containing only the flags +// that are allowed to be used in subcommand scope. func (flags Flags) WithSubcommandScope() Flags {cli/commands/aws-provider-patch/command.go (1)
42-45: Consider removing duplicate constant.
OverrideAttrFlagNameandDeprecatedOverrideAttrFlagNamehave the same value. Since they're both "override-attr", we could use a single constant.- OverrideAttrFlagName = "override-attr" - - DeprecatedOverrideAttrFlagName = "override-attr" + OverrideAttrFlagName = "override-attr"internal/strict/controls/control.go (1)
95-120: Enhance error handling in Evaluate methodThe error handling could be more descriptive. Consider wrapping errors with context about which control failed:
func (ctrl *Control) Evaluate(ctx context.Context) error { if ctrl == nil { return nil } if ctrl.Enabled { if ctrl.Status != strict.ActiveStatus || ctrl.Error == nil { return nil } - return ctrl.Error + return fmt.Errorf("control %q failed: %w", ctrl.Name, ctrl.Error) } // ... rest of the code }cli/commands/hclfmt/command.go (2)
26-30: Add documentation for prefix structureWhile the prefix handling looks good, it would be helpful to add documentation explaining:
- The difference between
tgPrefixandterragruntPrefix- When to use each prefix type
+// NewFlags creates a new set of flags for the hclfmt command. +// The prefix parameter is used to namespace environment variables: +// - tgPrefix: Used for current flags (e.g., TG_FILE) +// - terragruntPrefix: Used for deprecated flags (e.g., TERRAGRUNT_HCLFMT_FILE) func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags {
13-23: Document deprecation timelineConsider adding documentation about when these deprecated flags will be removed to help users plan their migration.
const ( + // Deprecated: These flag names will be removed in v1.0.0 DeprecatedHclfmtFileFlagName = "hclfmt-file" DeprecatedHclfmtcExcludeDirFlagName = "hclfmt-exclude-dir" // ... rest of the constants )pkg/log/format/formatter.go (2)
34-34: Consider adding early return optimizationThe condition check could be split for better readability and performance:
-if formatter.placeholders == nil || formatter.disabledOutput { +if formatter.disabledOutput { + return nil, nil +} +if formatter.placeholders == nil { return nil, nil }
111-129: Great addition of getter/setter methods! 🎯The new methods provide a clean API for controlling output behavior. Consider adding documentation to explain when one might want to disable output.
cli/commands/deprecated.go (1)
53-55: Consider providing more context in error messagesWhen a deprecated command fails evaluation, it might be helpful to include migration guidance in the error message.
-return cli.NewExitError(err, cli.ExitCodeGeneralError) +return cli.NewExitError(fmt.Errorf("deprecated command error: %w\nPlease refer to migration guide at: <url>", err), cli.ExitCodeGeneralError)cli/commands/run/command.go (2)
28-30: Track the TODO comment in an issue.The TODO comment about
--allsupport should be tracked in a GitHub issue for better visibility and follow-up.Would you like me to help create an issue to track the implementation of the
--allflag support?
86-91: Enhance error messages with examples.The error messages could be more helpful by including examples of valid commands.
- return cli.NewExitError(errors.New(WrongTerraformCommand(opts.TerraformCommand)), cli.ExitCodeGeneralError) + return cli.NewExitError(errors.New(WrongTerraformCommand(opts.TerraformCommand) + "\nExample valid commands: plan, apply, destroy"), cli.ExitCodeGeneralError) - return cli.NewExitError(errors.New(WrongTofuCommand(opts.TerraformCommand)), cli.ExitCodeGeneralError) + return cli.NewExitError(errors.New(WrongTofuCommand(opts.TerraformCommand) + "\nExample valid commands: plan, apply, destroy"), cli.ExitCodeGeneralError)internal/strict/controls/deprecated_flag_name.go (1)
86-98: Extract flag name formatting logic for better readability.The flag name formatting logic could be moved to a separate helper function to improve readability.
+func formatFlagName(flag cli.Flag, value string) string { + if names := flag.Names(); len(names) > 0 { + flagName := names[0] + if flag.TakesValue() && value != "" { + flagName += "=" + value + } + return flagName + } + return "" +} func (ctrl *DeprecatedFlagName) Evaluate(ctx context.Context) error { var ( valueName = ctrl.deprecatedFlag.Value().GetName() - flagName string + flagName = formatFlagName(ctrl.newFlag, ctrl.deprecatedFlag.Value().String()) ) if valueName == "" || !ctrl.deprecatedFlag.Value().IsArgSet() || slices.Contains(ctrl.newFlag.Names(), valueName) { return nil } - if names := ctrl.newFlag.Names(); len(names) > 0 { - flagName = names[0] - - if ctrl.newFlag.TakesValue() { - value := ctrl.newFlag.Value().String() - - if value == "" { - value = ctrl.deprecatedFlag.Value().String() - } - - flagName += "=" + value - } - }shell/run_cmd_output_test.go (1)
99-119: Consider using sync.RWMutex for better performance.Since
String()is a read-only operation, usingsync.RWMutexinstead ofsync.Mutexcould improve performance by allowing concurrent reads.type BufferWithLocking struct { buffer bytes.Buffer - mutex sync.Mutex + mutex sync.RWMutex } func (s *BufferWithLocking) Write(p []byte) (n int, err error) { s.mutex.Lock() defer s.mutex.Unlock() return s.buffer.Write(p) } func (s *BufferWithLocking) String() string { - s.mutex.Lock() - defer s.mutex.Unlock() + s.mutex.RLock() + defer s.mutex.RUnlock() return s.buffer.String() }cli/help_test.go (1)
14-109: Great test coverage, consider adding edge cases!The test is well-structured and covers the basic help template. Consider adding test cases for:
- Commands with no subcommands
- Commands with no flags
- Commands with only global flags
Here's a quick example of how to test a command with no subcommands:
func TestCommandHelpTemplateNoSubcommands(t *testing.T) { t.Parallel() app := cli.NewApp() cmd := &cli.Command{ Name: "simple", Usage: "A simple command", Flags: cli.Flags{}, } var out bytes.Buffer app.Writer = &out ctx := cli.NewAppContext(context.Background(), app, nil).NewCommandContext(cmd, nil) require.NoError(t, cli.ShowCommandHelp(ctx)) // Assert expected output }internal/cli/bool_flag_test.go (1)
129-138: Consider simplifying the LookupEnvFunc implementationThe current implementation could be more concise.
-func(key string) []string { - if envs == nil { - return nil - } - - if val, ok := envs[key]; ok { - return []string{val} - } - return nil -} +func(key string) []string { + if val, ok := envs[key]; ok { + return []string{val} + } + return nil +}internal/cli/args.go (2)
29-38: Consider adding examples to the Split function's documentationWhile the function name is clear, adding examples would make it even more helpful for users.
-// Split splits `args` into two slices separated by `sep`. +// Split splits `args` into two slices separated by `sep`. +// Example: +// args := Args{"a", "b", "--", "c", "d"} +// before, after := args.Split("--") +// // before = Args{"a", "b"} +// // after = Args{"c", "d"}
85-94: Consider adding error handling to RemoveThe function silently returns the original slice if the name isn't found. Consider adding a boolean return value to indicate whether the removal was successful.
-func (args Args) Remove(name string) Args { +func (args Args) Remove(name string) (Args, bool) { for i := range args { if args[i] == name { args := Args(args.Slice()) - return append(args[:i], args[i+1:]...) + return append(args[:i], args[i+1:]...), true } } - return args + return args, false }test/integration_strict_test.go (3)
31-31: Fix grammar in error messageThe error message has a grammatical error: "commands is deprecated" should be "command is deprecated".
- expectedStderr: "The `plan-all` commands is deprecated and will be removed in a future version. Use `terragrunt run-all plan` instead.", + expectedStderr: "The `plan-all` command is deprecated and will be removed in a future version. Use `terragrunt run-all plan` instead.",
46-46: Make error message more specificThe error message "is no longer supported" is too vague. It should specify what exactly is no longer supported.
- expectedError: errors.New("is no longer supported"), + expectedError: errors.New("The `plan-all` command is no longer supported"),
103-103: Clarify comment about strict mode testingThe comment could be more descriptive about why we can't test the strict mode flag.
- // we cannot test `-strict-mode` flag, since we cannot know at which strict control TG will output the error. + // Skip testing `-strict-mode` flag: The error output depends on the order of strict controls evaluation, which is non-deterministiccli/flags/flag_opts.go (1)
82-82: Fix typo in example commentThe example mentions "terragrunt-disabe-color" which should be "terragrunt-disable-color".
-// The deprecated flag will have "terragrunt-no-color","terragrunt-disabe-color" names and "TERRAGRUNT_NO_COLOR","TERRAGRUNT_DISABLE_COLOR" env vars. +// The deprecated flag will have "terragrunt-no-color","terragrunt-disable-color" names and "TERRAGRUNT_NO_COLOR","TERRAGRUNT_DISABLE_COLOR" env vars.pkg/log/level.go (1)
149-156: Consider using a constant for default levelInstead of using a magic number
0for the default level, consider defining a constant to make the code more maintainable.+const defaultLogrusLevel = logrus.Level(0) + // ToLogrusLevel converts our `Level` to `logrus.Level`. func (level Level) ToLogrusLevel() logrus.Level { if logrusLevel, ok := logrusLevels[level]; ok { return logrusLevel } - return logrus.Level(0) + return defaultLogrusLevel }internal/cli/map_flag_test.go (1)
141-150: Consider adding test cases for multiple environment variables.While the current test cases cover basic scenarios, it would be valuable to add tests that verify behavior when multiple environment variables are provided.
Example test case to consider:
+ { + cli.MapFlag[string, string]{Name: "foo", EnvVars: []string{"FOO", "BAR"}}, + nil, + map[string]string{"FOO": "", "BAR": "env1-key=env1-value"}, + map[string]string{"env1-key": "env1-value"}, + nil, + },internal/cli/slice_flag_test.go (1)
177-186: Consider adding test cases for multiple environment variables.Similar to the map flag tests, it would be valuable to add test cases that verify behavior with multiple environment variables.
Example test case to consider:
+ { + cli.SliceFlag[string]{Name: "foo", EnvVars: []string{"FOO", "BAR"}}, + nil, + map[string]string{"FOO": "", "BAR": "env-value1,env-value2"}, + []string{"env-value1", "env-value2"}, + nil, + },internal/cli/generic_flag_test.go (1)
194-203: Consider adding test cases for multiple environment variables.The current test suite would benefit from cases that verify behavior when multiple environment variables are provided.
Example test case to consider:
+ { + cli.GenericFlag[string]{Name: "foo", EnvVars: []string{"FOO", "BAR"}}, + nil, + map[string]string{"FOO": "", "BAR": "fallback-value"}, + "fallback-value", + nil, + },cli/commands/stack/action.go (1)
32-34: Improved error handling with exit codes.The change to use
cli.NewExitErrorprovides better error reporting by including an exit code. This helps with error handling in scripts and automation.Consider adding a more descriptive error message that explains how to enable the stacks experiment, perhaps with an example command.
internal/cli/command.go (1)
261-271: Improved action wrapping with proper cloning.The
WrapActionmethod now clones the command before modifying its action, preventing unintended side effects.Consider adding a comment explaining why cloning is necessary:
func (cmd *Command) WrapAction(fn func(ctx *Context, action ActionFunc) error) *Command { + // Clone the command to prevent modifications to the original instance clone := *cmdinternal/cli/command_test.go (1)
184-184: Consider adding more test cases for error scenarios.While the happy path is well tested, we might want to add more test cases for error conditions, especially around the new undefined flag handling.
Here's a suggested test case to add:
+ func(action TestActionFunc, skip cli.ActionFunc) TestCase { + return TestCase{ + []string{"--undefined-flag", "value"}, + cli.Command{ + ErrorOnUndefinedFlag: true, + Action: skip, + }, + errors.New("undefined flag: --undefined-flag"), + } + },test/integration_windows_test.go (1)
30-67: Great solution for handling Windows path length limitations! 🎯The implementation elegantly solves the "path too long" issue by:
- Saving original env vars
- Creating shorter temp dir
- Setting new env vars
- Restoring original env vars on cleanup
One suggestion though - consider adding error handling for the
os.Setenvrestore operation in the defer function.Here's how you could improve the error handling:
defer func() { // Restore previous values. for name, val := range envVars { - os.Setenv(name, val) + if err := os.Setenv(name, val); err != nil { + fmt.Printf("Failed to restore env var %s=%s due to error: %v", name, val, err) + } } }()internal/strict/control_test.go (2)
123-241: Excellent test structure and coverage! 🎯The
TestEnableControlfunction is well-organized with:
- Clear test cases
- Good edge case coverage
- Helpful error validation
- Clean assertions
One suggestion - consider adding a test case for enabling a control that's already enabled.
293-400: Great test coverage for control evaluation! 💪The
TestEvaluateControlthoroughly tests:
- Error conditions
- Warning messages
- Completed controls
- Multiple evaluations
Consider adding a test case for evaluating a non-existent control to ensure proper error handling.
pkg/log/logger.go (1)
154-163: Consider adding nil checks for formatter.While the implementation is good, it might be worth adding nil checks to prevent potential panics if a nil formatter is passed.
func (logger *logger) SetFormatter(formatter Formatter) { + if formatter == nil { + return + } logger.formatter = formatter logger.Logger.SetFormatter(&fromLogrusFormatter{Formatter: formatter}) }test/integration_engine_test.go (1)
198-198: Consider using a constant for the environment variable.Using string literals for environment variables can be error-prone. Consider defining a constant for
"TERRAGRUNT_ENGINE_SKIP_CHECK"to maintain consistency and prevent typos.+const terragruntEngineSkipCheckEnv = "TERRAGRUNT_ENGINE_SKIP_CHECK" + -t.Setenv("TERRAGRUNT_ENGINE_SKIP_CHECK", "1") +t.Setenv(terragruntEngineSkipCheckEnv, "1")test/integration_debug_test.go (1)
59-59: Consider removing manual cleanup.Since we're using
t.TempDir()which handles cleanup automatically, we can remove the manualdefer os.Remove(downloadDir)calls.cli/commands/run/download_source_test.go (1)
457-458: Clean temporary directory handling.Nice simplification of the
tmpDirfunction usingt.TempDir(). The empty line after the call can be removed for better conciseness.func tmpDir(t *testing.T) string { t.Helper() dir := t.TempDir() - return filepath.FromSlash(dir) }config/config_partial.go (1)
267-267: Remove redundant error wrapping.The
errors.New(err)call is redundant sinceos.Statalready returns an error. You can return the error directly:-return nil, errors.New(err) +return nil, errconfigstack/running_module_test.go (1)
18-18: Nice improvement to configuration path handling!The switch from
ClonetoCloneWithConfigPathmakes the configuration path handling more explicit and maintainable.Consider adding a test case that specifically verifies the configuration path is correctly cloned.
engine/engine.go (1)
507-510: Better logging encapsulation!The changes improve the logging design by:
- Using
Logger.Level()instead of accessing the log level directly- Using
Logger.Formatter().DisabledOutput()for a more encapsulated way to check if logging is disabledConsider adding debug logging to help troubleshoot log level determination issues:
+terragruntOptions.Logger.Debugf("Engine log level set to: %s", engineLogLevel)test/helpers/package.go (1)
898-913: Well-structured provider cache check function.The new
IsTerragruntProviderCacheEnabledfunction:
- Handles multiple environment variables
- Uses proper error handling
- Is well documented
Consider adding a constant for the default environment variable names:
+const ( + EnvTerragruntProviderCache = "TERRAGRUNT_PROVIDER_CACHE" + EnvTGProviderCache = "TG_PROVIDER_CACHE" +)test/fixtures/exec-cmd/app/main.tf (1)
1-3: Add description and validation for the variable.While the variable is correctly defined, consider adding:
- A description to document its purpose
- Validation rules if there are any constraints
Here's a more complete definition:
variable "foo" { type = string + description = "Add a description of what this variable is used for" + validation { + condition = can(regex("^[A-Z]+$", var.foo)) + error_message = "The foo value must be uppercase letters only." + } }test/fixtures/exec-cmd/script.sh (2)
3-3: Consider making the directory variable more robust.The
dirvariable is defined but never used. If it's needed, consider adding error handling:-dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) || { + echo "Failed to determine script directory" >&2 + exit 1 +}🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 3-3: dir appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
7-17: Document and validate environment variables upfront.Consider adding documentation and checking for variable existence before comparing values:
+# Required environment variables: +# - TF_VAR_foo: Must be set to "FOO" +# - TF_VAR_bar: Must be set to "BAR" + +: "${TF_VAR_foo:?Required environment variable not set}" +: "${TF_VAR_bar:?Required environment variable not set}" + if [ "$TF_VAR_foo" != "FOO" ] then echo "error: TF_VAR_foo is not defined"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: TF_VAR_foo is referenced but not assigned.
(SC2154)
[warning] 13-13: TF_VAR_bar is referenced but not assigned.
(SC2154)
docs/_docs/04_reference/05-strict-mode.md (1)
107-107: Style Suggestion for Control Categories Section:
Consider rephrasing the consecutive sentences in the control categories section to vary their openings. A bit of variation in sentence structure can improve readability and user engagement.🧰 Tools
🪛 LanguageTool
[style] ~107-~107: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...agrunt will exit with error code 1 ``` You can also use [control categories](#cont...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/_docs/02_features/01-units.md (1)
210-211: Punctuation Suggestion:
In the sentence describing how Terragrunt copies local files every time you run it, consider adding a comma before “and” to improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tf/getproviders/testdata/filesystem-mirror/registry.terraform.io/hashicorp/null/terraform-provider-null_2.1.0_linux_amd64.zipis excluded by!**/*.ziptf/getproviders/testdata/filesystem-mirror/registry.terraform.io/hashicorp/null/terraform-provider-null_invalid.zipis excluded by!**/*.zip
📒 Files selected for processing (207)
.circleci/config.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)cli/app.go(9 hunks)cli/app_test.go(13 hunks)cli/commands/aws-provider-patch/action.go(2 hunks)cli/commands/aws-provider-patch/command.go(1 hunks)cli/commands/catalog/action.go(1 hunks)cli/commands/catalog/command.go(2 hunks)cli/commands/catalog/module/repo.go(2 hunks)cli/commands/commands.go(1 hunks)cli/commands/deprecated.go(3 hunks)cli/commands/exec/action.go(1 hunks)cli/commands/exec/command.go(1 hunks)cli/commands/exec/options.go(1 hunks)cli/commands/flags.go(0 hunks)cli/commands/graph-dependencies/command.go(2 hunks)cli/commands/graph/action.go(3 hunks)cli/commands/graph/command.go(3 hunks)cli/commands/hclfmt/action.go(3 hunks)cli/commands/hclfmt/action_test.go(1 hunks)cli/commands/hclfmt/command.go(1 hunks)cli/commands/hclvalidate/action.go(1 hunks)cli/commands/hclvalidate/command.go(1 hunks)cli/commands/help/command.go(1 hunks)cli/commands/info/command.go(1 hunks)cli/commands/info/strict/command.go(1 hunks)cli/commands/output-module-groups/command.go(2 hunks)cli/commands/render-json/action.go(1 hunks)cli/commands/render-json/command.go(1 hunks)cli/commands/run-all/action.go(2 hunks)cli/commands/run-all/command.go(4 hunks)cli/commands/run/action.go(19 hunks)cli/commands/run/action_test.go(15 hunks)cli/commands/run/command.go(1 hunks)cli/commands/run/command_test.go(3 hunks)cli/commands/run/creds/getter.go(1 hunks)cli/commands/run/creds/providers/amazonsts/provider.go(1 hunks)cli/commands/run/creds/providers/externalcmd/provider.go(2 hunks)cli/commands/run/debug.go(4 hunks)cli/commands/run/download_source.go(11 hunks)cli/commands/run/download_source_test.go(11 hunks)cli/commands/run/errors.go(3 hunks)cli/commands/run/file_copy_getter.go(1 hunks)cli/commands/run/flags.go(1 hunks)cli/commands/run/hook.go(4 hunks)cli/commands/run/target.go(3 hunks)cli/commands/run/version_check.go(5 hunks)cli/commands/run/version_check_test.go(5 hunks)cli/commands/scaffold/action.go(8 hunks)cli/commands/scaffold/command.go(2 hunks)cli/commands/shortcuts.go(1 hunks)cli/commands/stack/action.go(3 hunks)cli/commands/stack/command.go(3 hunks)cli/commands/terraform/command.go(0 hunks)cli/commands/terragrunt-info/action.go(1 hunks)cli/commands/terragrunt-info/command.go(2 hunks)cli/commands/validate-inputs/action.go(2 hunks)cli/commands/validate-inputs/command.go(1 hunks)cli/commands/version/command.go(1 hunks)cli/deprecated_flags.go(0 hunks)cli/flags/deprecated_flag.go(1 hunks)cli/flags/flag.go(1 hunks)cli/flags/flag_opts.go(1 hunks)cli/flags/global/flags.go(1 hunks)cli/flags/prefix.go(1 hunks)cli/help.go(2 hunks)cli/help_test.go(1 hunks)cli/provider_cache.go(9 hunks)cli/provider_cache_test.go(1 hunks)config/catalog.go(1 hunks)config/config.go(6 hunks)config/config_helpers.go(5 hunks)config/config_partial.go(3 hunks)config/config_test.go(4 hunks)config/cty_helpers_test.go(1 hunks)config/dependency.go(6 hunks)config/parsing_context.go(2 hunks)config/util.go(2 hunks)config/variable.go(1 hunks)config/variable_test.go(1 hunks)configstack/log_test.go(1 hunks)configstack/module.go(2 hunks)configstack/module_test.go(3 hunks)configstack/running_module.go(3 hunks)configstack/running_module_test.go(1 hunks)configstack/stack.go(7 hunks)configstack/stack_test.go(9 hunks)docs/_docs/01_getting-started/04-terminology.md(3 hunks)docs/_docs/02_features/01-units.md(3 hunks)docs/_docs/02_features/03-includes.md(2 hunks)docs/_docs/02_features/10-auto-init.md(1 hunks)docs/_docs/02_features/12-provider-cache-server.md(3 hunks)docs/_docs/04_reference/01-configuration.md(2 hunks)docs/_docs/04_reference/03-built-in-functions.md(1 hunks)docs/_docs/04_reference/04-config-blocks-and-attributes.md(6 hunks)docs/_docs/04_reference/05-strict-mode.md(6 hunks)docs/_docs/04_reference/06-experiments.md(2 hunks)docs/_docs/04_reference/10-log-formatting.md(9 hunks)docs/_docs/04_reference/11-terragrunt-cache.md(1 hunks)docs/_docs/06_migration_guides/03-cli-redesign.md(1 hunks)engine/engine.go(1 hunks)internal/cache/context.go(1 hunks)internal/cli/app.go(5 hunks)internal/cli/args.go(4 hunks)internal/cli/args_test.go(3 hunks)internal/cli/autocomplete.go(2 hunks)internal/cli/bool_flag.go(1 hunks)internal/cli/bool_flag_test.go(6 hunks)internal/cli/command.go(9 hunks)internal/cli/command_test.go(3 hunks)internal/cli/commands.go(2 hunks)internal/cli/context.go(2 hunks)internal/cli/errors.go(3 hunks)internal/cli/exit_code.go(1 hunks)internal/cli/flag.go(1 hunks)internal/cli/flags.go(4 hunks)internal/cli/flags_test.go(4 hunks)internal/cli/funcs.go(1 hunks)internal/cli/generic_flag.go(5 hunks)internal/cli/generic_flag_test.go(7 hunks)internal/cli/help.go(3 hunks)internal/cli/map_flag.go(7 hunks)internal/cli/map_flag_test.go(5 hunks)internal/cli/slice_flag.go(6 hunks)internal/cli/slice_flag_test.go(6 hunks)internal/cli/sort_test.go(1 hunks)internal/cloner/clone.go(1 hunks)internal/cloner/cloner.go(1 hunks)internal/experiment/errors.go(1 hunks)internal/experiment/experiment.go(1 hunks)internal/experiment/experiment_test.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/control_test.go(1 hunks)internal/strict/controls/control.go(1 hunks)internal/strict/controls/controls.go(1 hunks)internal/strict/controls/depreacted_env_var.go(1 hunks)internal/strict/controls/deprecated_command.go(1 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)internal/strict/errors.go(1 hunks)internal/strict/status.go(1 hunks)internal/strict/strict.go(1 hunks)internal/strict/strict_test.go(0 hunks)internal/strict/view/plaintext/plaintext.go(1 hunks)internal/strict/view/plaintext/render.go(1 hunks)internal/strict/view/plaintext/template.go(1 hunks)internal/strict/view/render.go(1 hunks)internal/strict/view/view.go(1 hunks)internal/strict/view/writer.go(1 hunks)main.go(2 hunks)options/options.go(12 hunks)pkg/cli/bool_flag.go(0 hunks)pkg/cli/flag.go(0 hunks)pkg/log/context.go(1 hunks)pkg/log/format/format.go(5 hunks)pkg/log/format/formatter.go(5 hunks)pkg/log/format/options/color.go(1 hunks)pkg/log/format/options/option.go(1 hunks)pkg/log/formatter.go(1 hunks)pkg/log/helper.go(0 hunks)pkg/log/level.go(1 hunks)pkg/log/logger.go(5 hunks)pkg/log/options.go(1 hunks)remote/remote_state.go(1 hunks)shell/git.go(3 hunks)shell/run_cmd.go(1 hunks)shell/run_cmd_output_test.go(1 hunks)shell/run_cmd_test.go(4 hunks)shell/run_cmd_unix_test.go(2 hunks)shell/run_cmd_windows_test.go(2 hunks)shell/run_shell_cmd.go(0 hunks)terraform/terraform.go(0 hunks)test/fixtures/exec-cmd/app/main.tf(1 hunks)test/fixtures/exec-cmd/app/terragrunt.hcl(1 hunks)test/fixtures/exec-cmd/script.sh(1 hunks)test/helpers/package.go(9 hunks)test/integration_common_test.go(3 hunks)test/integration_debug_test.go(4 hunks)test/integration_destroy_test.go(5 hunks)test/integration_download_test.go(5 hunks)test/integration_engine_test.go(1 hunks)test/integration_functions_test.go(2 hunks)test/integration_hooks_test.go(1 hunks)test/integration_scaffold_test.go(11 hunks)test/integration_serial_test.go(5 hunks)test/integration_sops_test.go(2 hunks)test/integration_strict_test.go(5 hunks)test/integration_test.go(27 hunks)test/integration_windows_test.go(1 hunks)tf/cache/config.go(1 hunks)tf/cache/controllers/discovery.go(1 hunks)tf/cache/controllers/downloader.go(1 hunks)tf/cache/controllers/provider.go(1 hunks)tf/cache/handlers/provider.go(1 hunks)tf/cache/handlers/provider_direct.go(1 hunks)tf/cache/handlers/provider_filesystem_mirror.go(1 hunks)tf/cache/handlers/provider_network_mirror.go(1 hunks)tf/cache/handlers/provider_test.go(1 hunks)tf/cache/handlers/reverse_proxy.go(1 hunks)tf/cache/models/provider_test.go(1 hunks)tf/cache/server.go(1 hunks)tf/cache/services/provider_cache.go(1 hunks)tf/cliconfig/config_test.go(3 hunks)tf/context.go(2 hunks)tf/detailed_exitcode.go(1 hunks)tf/doc.go(1 hunks)
⛔ Files not processed due to max files limit (19)
- tf/errors.go
- tf/getproviders/hash_test.go
- tf/getproviders/lock.go
- tf/getproviders/lock_test.go
- tf/getproviders/package_authentication_test.go
- tf/getter.go
- tf/getter_test.go
- tf/log.go
- tf/run_cmd.go
- tf/run_cmd_test.go
- tf/source.go
- tf/source_test.go
- tf/testdata/test_outputs.sh
- tf/tf.go
- tflint/tflint.go
- util/collections.go
- util/file.go
- util/file_test.go
- util/shell.go
💤 Files with no reviewable changes (9)
- shell/run_shell_cmd.go
- pkg/log/helper.go
- internal/strict/strict_test.go
- terraform/terraform.go
- cli/commands/terraform/command.go
- cli/deprecated_flags.go
- pkg/cli/flag.go
- pkg/cli/bool_flag.go
- cli/commands/flags.go
✅ Files skipped from review due to trivial changes (31)
- internal/strict/view/plaintext/plaintext.go
- cli/commands/commands.go
- internal/strict/view/view.go
- tf/doc.go
- tf/cache/controllers/discovery.go
- pkg/log/format/options/option.go
- tf/cache/server.go
- internal/cli/sort_test.go
- tf/detailed_exitcode.go
- tf/cache/models/provider_test.go
- cli/commands/run/file_copy_getter.go
- docs/_docs/02_features/03-includes.md
- tf/cache/handlers/reverse_proxy.go
- tf/cache/handlers/provider_direct.go
- cli/commands/catalog/module/repo.go
- docs/_docs/04_reference/10-log-formatting.md
- test/fixtures/exec-cmd/app/terragrunt.hcl
- cli/commands/run/creds/providers/amazonsts/provider.go
- tf/cache/handlers/provider_test.go
- tf/cache/handlers/provider_network_mirror.go
- tf/cache/controllers/downloader.go
- cli/commands/scaffold/action.go
- tf/cache/handlers/provider.go
- cli/commands/run/creds/getter.go
- tf/cache/controllers/provider.go
- tf/cache/handlers/provider_filesystem_mirror.go
- cli/provider_cache_test.go
- tf/cache/services/provider_cache.go
- config/util.go
- internal/strict/strict.go
- tf/cache/config.go
🧰 Additional context used
🪛 Shellcheck (0.10.0)
test/fixtures/exec-cmd/script.sh
[warning] 3-3: dir appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 7-7: TF_VAR_foo is referenced but not assigned.
(SC2154)
[warning] 13-13: TF_VAR_bar is referenced but not assigned.
(SC2154)
🪛 LanguageTool
docs/_docs/04_reference/05-strict-mode.md
[style] ~107-~107: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...agrunt will exit with error code 1 ``` You can also use [control categories](#cont...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/_docs/06_migration_guides/03-cli-redesign.md
[style] ~29-~29: Consider removing “of” to be more concise
Context: ...sier to find them. ## Migration guide All of the changes you need to make to adopt to th...
(ALL_OF_THE)
[misspelling] ~30-~30: Did you mean “adapt to”?
Context: ... All of the changes you need to make to adopt to this new CLI design involve changing ho...
(ADOPT_TO)
[style] ~34-~34: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact flag with terragrunt- removed from th...
(SAME_EXACT)
[style] ~36-~36: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ragrunt-non-interactive flag, you will need to switch to the [--non-interactive`](/do...
(REP_NEED_TO_VB)
[style] ~52-~52: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...the --terragrunt-debug flag, you will need to switch to the [--inputs-debug](/docs/...
(REP_NEED_TO_VB)
[style] ~70-~70: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact environment variable with TERRAGRUNT_...
(SAME_EXACT)
docs/_docs/02_features/01-units.md
[style] ~167-~167: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... so the source parameter supports the exact same syntax as the [module source](https://o...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[uncategorized] ~212-~212: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...rragrunt with the --source-update flag and it’ll delete the tmp folder, download t...
(COMMA_COMPOUND_SENTENCE)
docs/_docs/04_reference/01-configuration.md
[style] ~130-~130: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file qa/services/services.hcl`. If you want to format HCL from stdin and print the res...
(REP_WANT_TO_VB)
docs/_docs/02_features/12-provider-cache-server.md
[typographical] ~29-~29: It seems that a comma is missing.
Context: ...so it is disabled by default. To enable it you need to use the flag [`provider-cac...
(IN_ORDER_TO_VB_COMMA)
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lly every time you launch Terragrunt. - Server Token [provider-cache-token](https://...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: unessential
- GitHub Check: build-and-test
🔇 Additional comments (343)
docs/_docs/04_reference/11-terragrunt-cache.md (1)
38-38: Environment Variable Update DocumentationNice work updating the environment variable in the docs. Changing the reference from the old
TERRAGRUNT_DOWNLOADtoTG_DOWNLOAD_DIRaligns perfectly with the new CLI flag and environment variable naming conventions. This adjustment improves clarity for users looking to customize their cache directory location.shell/run_cmd_test.go (2)
41-58: Nice improvement to the assertions!The switch to
assert.Containsmakes the tests more idiomatic and easier to read. The test coverage for output handling is thorough, checking both stdout and stderr scenarios.
22-22: LGTM! Let's verify the command execution behavior.The transition from
RunShellCommandtoRunCommandlooks good. The test cases cover both successful and error scenarios appropriately.Let's ensure this change is consistent across the codebase:
Also applies to: 25-25
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of RunShellCommand to ensure complete migration rg "RunShellCommand" --type goLength of output: 768
LGTM! Command Execution Changes Verified and Consistent
The updated test in shell/run_cmd_test.go is now calling RunCommand as expected. The search did show some references to RunShellCommand in tf/context.go and the TestRunShellCommand function in the test file—these remain for legacy hook support and context configuration, and they aren’t affecting the new functionality.
• The updated command execution call in shell/run_cmd_test.go is correct.
• Remaining RunShellCommand references in tf/context.go are intentional for context hook management.
• The naming of the TestRunShellCommand function is legacy, but its internal behavior uses RunCommand as required.Overall, the changes align well with the PR objectives.
docs/_docs/04_reference/06-experiments.md (31)
70-70: New Active Experiment: cli-redesign Added.
The addition of the cli-redesign item to the active experiments list makes it easy for users to spot the new experimental feature. Make sure that any cross-references or usage examples elsewhere in the docs are updated accordingly.
76-76: Enhanced Heading for Symlinks Functionality.
Updating the heading "####symlinks- What it does" improves clarity. It now clearly defines the purpose of the symlinks experiment.
80-80: Improved Feedback Section Heading for Symlinks.
Changing the heading to "####symlinks- How to provide feedback" standardizes the format and guides users on where to submit input.
84-84: Standardized Stabilization Criteria Heading for Symlinks.
The new heading "####symlinks- Criteria for stabilization" enhances consistency across experimental sections.
93-93: Updated Section Heading for Stacks.
Renaming the section to "###stacks" aligns it with other experimental sections for a unified look.
97-97: Clear Description Heading for Stacks Details.
The heading "####stacks- What it does" now clearly indicates the feature's function, improving documentation readability.
101-101: Refined Feedback Heading for Stacks.
The updated "####stacks- How to provide feedback" heading helps users quickly locate instructions for providing input on this experimental feature.
109-109: Consistent Stabilization Criteria for Stacks.
The revised "####stacks- Criteria for stabilization" heading harmonizes with the rest of the experimental sections for easier comparison.
119-119: Introducing the 'cli-redesign' Experimental Section.
Adding the "### cli-redesign" section provides a dedicated area to describe the new CLI design and its features. This helps in distinguishing it from legacy commands.
121-121: Introductory Description for CLI Redesign.
The brief text "Support for the new Terragrunt CLI design." gives immediate context. Consider elaborating as more details become finalized.
123-123: Heading for CLI Redesign Functionality.
"####cli-redesign- What it does" clearly sets the stage for describing the new CLI features.
127-127: Introduction to Feature List.
The simple line "This includes:" nicely precedes the list of new commands. It makes the section easily scannable.
129-129: Documenting the NewrunCommand.
Noting the addition of theruncommand is crucial for aligning documentation with the updated CLI functionality.
130-130: Documenting the NewexecCommand.
Including theexeccommand in this list reinforces its introduction as part of the CLI redesign.
132-132: Feedback Heading for CLI Redesign Added.
The "####cli-redesign- How to provide feedback" heading positions users to know where to provide their input, which is essential for experimental features.
134-134: Clear Call-to-Action for Feedback on CLI Redesign.
Linking to the RFC for CLI redesign ensures users can easily join discussions and provide feedback.
140-140: Stabilization Criteria Heading for CLI Redesign.
"####cli-redesign- Criteria for stabilization" fits perfectly with the established pattern, making it straightforward for users to see what remains to be addressed.
142-142: Clear Introduction to CLI Stabilization Criteria.
This sentence sets user expectations about what must be done for the CLI redesign features to become stable, which is great for transparency.
144-144: Checklist Item: Support forrunCommand.
Listing "- [ ] Add support forruncommand." helps track feature progress. Ensure documentation and tests cover this once implemented.
145-145: Basic Usage ofrunCommand Confirmed.
The checklist marking basic usage as complete ("- [x] Add support for basic usage of theruncommand ...") is encouraging. It indicates that minimal functionality is in place.
146-146: Checklist Item:--allFlag Support.
Be sure to follow up with comprehensive tests and documentation for the newly added--allflag once development continues.
147-147: Checklist Item:--graphFlag Support.
This flagged item highlights a feature that requires further work. Ensure that its implementation is compatible with current command behaviors and user expectations.
148-148: Explicit Support forexecCommand Confirmed.
The checklist indicates that theexeccommand is fully supported now, which aligns with the PR objectives.
149-149: Renaming Legacy Flag Prefixes.
The change to remove the--terragrunt-prefix is a positive step toward simplicity and consistency in CLI usage.
151-151: Explicit Shortcut for OpenTofu Commands.
Marking the OpenTofu command support as completed adds clarity about how unknown commands are handled.
152-152: Planned Support for thebackendCommand.
Keep an eye on ensuring consistent error handling and user messaging for the backend command when it’s implemented.
153-153: Planned Support for therenderCommand.
Adding this command is a good move, but remember to later validate that its output is user-friendly and cohesive with other CLI outputs.
154-154: Checklist forinfoCommand Support.
Including theinfocommand meets part of the PR objectives. Confirm that its behavior aligns with the newly implemented strict flags if applicable.
155-155: Introducing thedagCommand.
This checklist entry is useful for future enhancements. Since dag-based functionalities can get complex, be sure to plan for rigorous testing.
156-156: Checklist for thefindCommand.
The addition of thefindcommand should be documented thoroughly. Consider including examples for clarity on how it differentiates from similar commands.
157-157: Adding Support for thelistCommand.
This final checklist entry rounds out the new command set. Ensure consistency in parameter handling and output formatting with the rest of the CLI.main.go (7)
5-5: The new import for "flag" looks straightforward.
No issues here. This aligns with your usage of the FlagSet below.
9-9: Importing "flags/global" for log level functionality is properly placed.
No concerns. It follows logical usage for the new log level flag setup.
20-20: Redefined exitCode as tf.DetailedExitCode.
This is consistent with the new approach. Ensure that any references or wrapper functions are mapped over properly so we don’t mix old and new exit code types.
24-28: Nice approach to parse the log level immediately.
This ensures consistent logging behavior throughout app initialization. Just watch out for scenarios where the environment variable is unset, so default logging is properly enforced.
32-34: Context enrichment is well-structured.
Storing the logger and exit code in the context is a succinct way to propagate necessary data without cluttering arguments.
39-39: Chaining checkForErrorsAndExit.
Calling this after app.RunContext is a neat way to finalize error handling. Ensures consistent exit codes.
14-14: Switched to "tf" for DetailedExitCode handling.
Just confirm that all downstream references to the old shell package have been cleaned up.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Searching for references to the old DetailedExitCode type in 'shell' package rg -A 2 "shell\.DetailedExitCode"Length of output: 35
I'm going to run a couple of additional commands to double-check that there aren’t any lingering references to the old shell package. We want to ensure that both the DetailedExitCode usage and any import references to the old shell package have been fully removed.
🏁 Script executed:
#!/bin/bash # Checking for any usage of DetailedExitCode from the old shell package. echo "Searching for DetailedExitCode references..." rg -n "shell\.DetailedExitCode" || echo "No DetailedExitCode references found." # Checking for any import statements referencing the old shell package. echo "Searching for old shell package import references..." rg -n "github.com/gruntwork-io/terragrunt/shell" || echo "No shell package import references found."Length of output: 1772
DetailedExitCode Migration Verified
I confirmed that there are no usages of shell.DetailedExitCode anywhere in the codebase. The DetailedExitCode handling has indeed been switched over to the tf package as intended. Note, however, that several files (like main.go and various CLI commands) still import the old shell package—these imports appear to be for other functionality and aren’t related to DetailedExitCode.
internal/cli/help.go (5)
37-37: Returning a success exit error in ShowAppHelp.
This pattern is unusual—be sure your downstream call sites treat this as a normal exit.
40-63: Refined ShowCommandHelp approach.
Nicely done: it checks for custom help, applies a default if missing, and normalizes the help name. This is a more flexible design that clarifies each command’s help output.
78-79: Consistent success exit code in ShowVersion.
Aligns with ShowAppHelp for uniformity. No issues here.
81-107: parentCommands function effectively gathers ancestors.
The reversal ensures a top-down display, which is logical for help context. Code looks clean.
6-6: Use of "golang.org/x/exp/slices" for reversing the slice.
This is cool, but confirm that your Go version supports this dependency without issues.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Checking if slices is used anywhere else in the project rg -A 3 "slices\."Length of output: 4362
🏁 Script executed:
#!/bin/bash # Check the Go version specified in go.mod to ensure it supports generics (and thus golang.org/x/exp/slices) grep '^go ' go.modLength of output: 28
Confirmed: Go Version Supports "golang.org/x/exp/slices" Dependency
Your go.mod file specifies Go 1.23, which is well beyond the generics introduction (Go 1.18). Since
golang.org/x/exp/slicesrelies on generics, this dependency is fully supported in your project with no compatibility problems. The widespread usage throughout the code further reinforces this working as intended.cli/flags/flag.go (10)
1-3: Package initialization for strict-wrapping deprecated flags.
This is a clear and meaningful package description.
4-9: Imports are appropriate.
No immediate concerns. Everything seems relevant to implementing and registering flags.
11-11: Ensuring cli.Flag is implemented via compile-time check.
A neat technique to confirm interface conformity.
13-17: Flag struct with embedded deprecatedFlags.
Straightforward design—embedding the cli.Flag is an intuitive approach for extension.
19-30: NewFlag constructor with optional configuration.
Looks good. Clear separation of concerns, letting your options pattern handle each transformation.
32-34: NewMovedFlag constructor.
Helps gracefully deprecate flags while pointing folks to the new command. Make sure you’re consistent about logging or notifying users.
36-45: TakesValue checks the underlying value interface.
Good logic to handle boolean flags distinctly. The fallback is also correct.
47-57: Setting new.Flag.Value() from deprecated flags
This ensures updates flow smoothly to the new value. Just be sure you don’t inadvertently overwrite user input if both are set.
59-80: Apply method merges flags with the standard set plus any deprecated flags.
This approach is neat, though watch for name collisions if you have multiple deprecations referencing the same name.
82-107: RunAction method cascades through deprecated flags.
A tidy solution for reusing actions across old and new flags. Double-check that the Evaluate calls don’t have side effects if repeated.internal/strict/controls/depreacted_env_var.go (2)
21-29: Struct definition looks well-organized.
The fields and embedded Control struct are clear. No issues spotted here.
58-95: Evaluate method logic appears solid.
It handles deprecated vs. new environment variables well, including once-only logging. Looks good to me!cli/commands/graph/command.go (2)
12-16: Imports for 'run', 'flags', and 'cli' look consistent.
They’re aligned with the new architecture.
64-64: Switching to run.Run is consistent with PR goals.
This replacement matches the new direction for command execution.cli/commands/scaffold/command.go (2)
18-21: New flag constants look straightforward.
Defining them at the top helps keep everything tidy.
24-71: The flag definitions are well-organized.
The usage strings are clear, and the action callbacks handle validations nicely. No big concerns here.cli/commands/run-all/command.go (8)
12-12: Imports look aligned with the new run package refactor.Everything seems properly organized here, no issues.
Also applies to: 15-15, 16-16
22-27: Verify the usage of identical strings for both active and deprecated flags.Right now, “out-dir” is used for both the new and old flags. Confirm if this was intended or if a distinct name for the deprecated version might help avoid confusion.
37-37: Double-check for potential flag collisions.Appended flag sets can inadvertently collide. Ensure that the sets from run.NewFlags and NewFlags don’t create conflicting flag definitions.
41-44: Flag composition approach looks good.Neat pattern to group these flags with a prefix. Maintains clarity and reusability.
47-48: Backward compatibility is nicely preserved.You’re properly using deprecated names with “flags.WithDeprecatedNames.” This is a thoughtful approach to maintain existing user workflows.
Also applies to: 53-53
55-56: JSON output directory flags are also well-handled.Same pattern as the standard output flags, no concerns here.
Also applies to: 61-61
73-73: Refactor from terraform.Run to run.Run is consistent.No immediate issues. This cleanly transitions execution to the new package.
93-93: Subcommand addition suits the overall design.Adding run.NewCommand keeps everything aligned. Looks good.
internal/experiment/experiment.go (13)
11-13: Imports for 'log' and 'slices' look fine.Using slices.Sort for sorting experiment names is straightforward and efficient for small sets.
15-22: Defining experiments as string constants is transparent.Clear, descriptive experiment names. Users will appreciate the clarity.
24-29: Status constants look good.Byte-based iota enums are an acceptable approach for categorizing experiment states.
31-32: Slice-based experiments data structure is fine for small feature sets.Straightforward approach. No red flags for typical use cases.
38-45: Default experiments are well-defined here.They’re neatly grouped, making it simple to add or remove experiments later.
50-61: Names() function is readable.Sorting ensures consistent output. Looks good.
63-75: FilterByStatus logic is straightforward.This is an easy way to retrieve subsets of experiments, nice job.
76-85: Linear search in Find() is totally fine for small collections.No issues noted. Remains simple and effective.
87-93: ExperimentMode enabling all ongoing experiments is intuitive.Implementation lines up with the feature’s intention.
94-103: EnableExperiment has good error handling.Returning the valid experiments helps guide users when they specify an invalid one.
105-119: NotifyCompletedExperiments is user-friendly.Warns about completed experiments still in use, which is helpful.
122-129: Evaluate() is concise.Returning a boolean keeps usage simple. Users might appreciate detail in logs if a name is missing, but this is still fine.
131-139: Experiment struct is nicely laid out.Name, Enabled, and Status cover the essential fields for toggling feature states.
internal/cli/bool_flag.go (13)
1-9: Package definition and imports look good.No concerns about these new references.
10-12: Type assertion ensures BoolFlag implements the Flag interface.This is a handy compile-time check.
40-55: Apply method is structured cleanly.Ensuring FlagValue is initialized before applying is a good approach.
57-60: Hidden flag logic is straightforward.Matches typical CLI expectations for “hidden” flags.
62-65: Usage getter is simple and effective.No friction in retrieving usage details.
67-70: Multiple environment variables are a plus.Be sure to test that all env vars are correctly recognized in practice.
72-76: TakesValue returning false is correct for boolean flags.Logical approach—no problems here.
78-85: Default text logic is nicely handled.Uses FlagValue if none is explicitly supplied. Thumbs-up.
87-90: String representation is fine.Makes sense to rely on cli.FlagStringer for consistency.
92-95: Combining main flag name with aliases is standard practice.Nicely done for easy short-flag referencing.
97-109: RunAction’s approach is well-considered.Allocating a new bool if Destination is nil is a neat fallback.
111-112: FlagVariable interface compliance confirmed.Readers appreciate the clarity.
113-158: The boolVar struct gracefully supports negative logic.Inverting a boolean can be handy. Implementation looks stable and consistent.
shell/run_cmd.go (5)
1-2: Good start with package docs!
Meaningful top-level comment promotes clarity.
30-35: RunCommand is straightforward.
It neatly delegates real work to RunCommandWithOutput. No concerns here.
37-71: Nicely organized telemetry.
Capturing command details with telemetry is a great way to track usage. Just confirm that sensitive info isn’t leaked in logs.
72-105: Engine usage check is helpful.
The fallback logic is well-structured, but confirm that the engine’s output remains consistent with direct command usage.
107-145: Context handling for cmd.Start()
You do call RegisterGracefullyShutdown for a graceful kill, but ignoring the default context linter might mask genuine cancellations. Revisit if you want to align precisely with context deadlines or timeouts.internal/cli/slice_flag.go (6)
34-40: Flexible handling of EnvVars.
Great improvement to allow multiple environment variables. Also, including an Action and Setter adds more control.
54-56: Skipping repeated Apply calls
If FlagValue is already set, re-Apply is bypassed. This logic helps prevent accidental re-initialization. Looks good.
80-90: Intuitive sliceValue creation.
Ensuring the same valueType is used for both environment defaults and user input is consistent.
110-110: GetDefaultText
Good approach: show initial text if no explicit default is provided. This helps with clarity in CLI usage messages.
139-156: sliceValue struct
Reset safely clears everything. The approach to store new data in Destination plus calling setter is robust.
166-170: Graceful setter calls
If the setter indicates an error, you return immediately. That’s a good fail-fast pattern.internal/cli/generic_flag.go (11)
30-35: EnvVars switch aligns with your new multi-variable approach.
Renaming EnvVar to EnvVars was a good evolution.
45-47: Prevent duplicate flag setup
Early return from Apply if FlagValue is already non-nil avoids reprocessing. Nicely done.
49-50: Assigning Destination properly
Using genericVar plus a pointer keeps things type-safe and flexible. No issues here.
52-54: FlagValue initialization
Storing the initial text value ensures proper help text and resets. Logical approach.
70-73: GetEnvVars
Returns the newly introduced EnvVars slice. Straightforward and consistent with other flag types.
78-78: Relying on GetInitialTextValue
Saves extra branching logic for default text. Thumbs up.
96-99: Destination pointer guard
Smart check for nil to avoid a panic or unexpected nil reference.
102-102: Action with typed destination
Invoking Action with *dest ensures user code has straightforward typed data.
112-120: genericValue
Nicely encloses setter logic alongside the typed value. Creates a single point of change for setting behavior.
125-135: Set: immediate setter call
Returning right after a setter error avoids partial misconfiguration. Good approach.
208-214: Get with nil guard
Returning a zero of T if dest is nil prevents surprising nil pointer usage. This is safe and consistent.cli/flags/deprecated_flag.go (3)
19-26: Looks good overall.No immediate correctness, security, or readability issues identified with the struct fields. This is a straightforward approach to storing deprecated flag data.
61-79: Confirm possible nil references for “mainFlag.”If “mainFlag” can be nil, the code might panic when you create controls. Please ensure that “mainFlag” is always valid or handle nil cases carefully.
Would you like me to generate a script that searches usage sites to confirm it’s never passed nil?
128-147: Registration process appears solid.Incremental subcontrol registration is well-structured here. The composition with FilterByNames merges seamlessly with the categories.
internal/cloner/cloner.go (1)
157-157: Check availability of strings.Clone in your Go version.“strings.Clone” was introduced in Go 1.20. Verify that your environment meets this requirement or consider using an alternative for compatibility with older Go versions.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Checking Go version. We expect >= 1.20. go versionLength of output: 44
Go Version Compatibility Confirmed
The Go version output is "go1.23.3", which is higher than 1.20, so using strings.Clone is perfectly fine in this environment.
internal/cli/map_flag.go (2)
43-46: Documentation updated nicely.The new “Action” field and comments look clear. This should help future maintainers understand how flag actions are triggered.
125-127: Environment variable retrieval logic is straightforward.The dedicated EnvVars slice is a clean improvement over a single string for environment variables. Nicely done.
internal/cli/flag.go (1)
205-217: Discuss ignoring empty environment vars.
Right now, ApplyFlag skips empty env vars (line 254). Double-check this aligns with your desired behavior. Some might expect an empty env var to override a default. If so, consider distinguishing between absent and empty values.cli/commands/run/download_source.go (6)
16-16: Great job migrating to the new package alias (tf).
This change clarifies the source management references and ensures we’re consistently importing from the new package.
38-40: Confirm consistent support for symlink behavior.
You introduced “walkWithSymlinks” support at line 38. Make sure that’s tested across multiple OS and environment variations, as symlink handling can be finicky.
76-76: Neat use of CloneWithConfigPath.
Cloning TerragruntOptions helps keep context consistent post-download. Looks good.
125-131: Hooks with init-from-module.
This is a clever place to run “before & after” hooks. Keep an eye out for potential race conditions if multiple “download source” calls happen concurrently in a parallel workflow.
258-260: Trim the file:// prefix only when needed.
At line 264, we remove fileURIScheme unconditionally. In edge cases (unusual protocols?), double-check that the prefix trimming doesn’t accidentally remove something else. A condition check might make it safer.
280-280: Clear error for non-directory working paths.
Your separate errors for “WorkingDirNotDir” vs. “WorkingDirNotFound” produce more contextual feedback, which is great. Keep this pattern consistent throughout the codebase.cli/flags/global/flags.go (3)
69-70: Neat approach to grouping global flags.
All flags live in a single place, which makes them easy to spot and manage. This structure looks clean and straightforward. Keep it up!
91-94: Double-check setting ForwardTFStdout when logs are disabled.
When the user disables logging, you're forcing Terraform output to the console by setting “opts.ForwardTFStdout = true.” This might surprise folks who simply wanted silence. Consider verifying whether that’s truly the intended behavior, especially if you want to keep Terraform output quiet.
237-264: Backwards compatibility for removed log levels looks good.
It’s great that you handle “panic” or “fatal” levels by reverting to a disabled log approach. That ensures older scripts behave consistently.cli/app.go (1)
283-283: Injecting the Terraform command hook into the context.
Hooking “tf.ContextWithTerraformCommandHook” here is a handy way to insert your caching logic behind the scenes. Nice job, just stay mindful of possible context overrides if other hooks get chained.cli/provider_cache.go (3)
160-160: Smart prevention of hook recursion.
Calling “tf.ContextWithTerraformCommandHook(ctx, nil)” ensures you won’t infinitely loop by re-calling the same hook. Good catch!
252-253: Cloning Terragrunt options here is sensible.
You’re avoiding side effects on the original “opts,” which helps keep code modular. Nicely done.
357-359: Ignoring 423 errors might mask genuine issues.
Matching “httpStatusCacheProviderReg” swallows all 423 responses as successes. If there’s a scenario where a genuine error is disguised as 423, you’d never know. Maybe a slight warning log would help clarity.options/options.go (3)
100-143: Validate that shadow copy meets concurrency needs.
Addingclone:"shadowcopy"is helpful for partial clones. Just be sure that simultaneously using the shared fields (e.g., Logger) across multiple clones won’t trigger concurrency issues if your runtime modifies them asynchronously.
458-511: Great defaults in NewTerragruntOptionsWithWriters.
The initial setup with concurrency-friendly data structures (xsync) and a preconfigured logger is quite handy. Suggest adding test coverage specifically for these defaults to catch any future regressions in initialization logic.
581-588: Check if logger should always be re-cloned.
In Clone(), you clone the options and do “Logger = opts.Logger.Clone()”. This ensures logs from the clone don’t collide. That’s typically good, but confirm that references shared at runtime do not inadvertently lose log context.cli/commands/run/action.go (8)
1-1: Package rename is consistent.
Renaming to “run” helps clarify that this package orchestrates command execution. Nicely done.
15-21: Smooth import transitions.
Swapping in references to “run/creds” and “tf” appear correct. Double-check that no references remain in any older “terraform” subpackage that might cause confusion or partial refactoring.
92-95: Short-circuiting for version command.
This is a handy pattern to skip heavy logic. It’s good for performance and user experience.
130-130: Cloning with config path is helpful for after-hooks.
Using CloneWithConfigPath ensures the action hooks can read from the right directory context. Keep an eye on potential race conditions if the main process also modifies these configs.
311-314: Handy callback insertion for custom logic.
It’s neat how you check “target.isPoint” to run specific callback logic. This kind of approach can keep your pipeline flexible.
412-430: Refactored signature for ShouldCopyLockFile.
Switching from []string to cli.Args is a tidy fix. Watch out for any edge case where the first or second argument might be missing, leading to potential index errors. Possibly add a defensive check.
739-755: Respecting prevent_destroy with multiple destroy triggers.
We see the code checks for both “destroy” and “-destroy” flags. That’s thorough. If additional synonyms or subcommands appear in the future, ensure this logic remains robust.
759-782: Skipping var files when applying or destroying a plan file.
Smart approach to skip -var-file arguments if the last parameter is a plan file. This can prevent weird terraform errors. Keep an eye out for partial plan filenames that might break detection.test/integration_test.go (18)
17-17: Good import for the run package.
This aligns well with your CLI redesign. Nice work keeping dependencies tidy!
28-28: Smooth shift to the tf package.
Using tf.DetailedExitCode for exit codes clarifies that relationship between Terragrunt and Terraform.
172-172: Consistent usage of tf.DetailedExitCode.
This variable name is clearer than the older shell-based approach.
174-174: Setting exit code via context.
Looks neat—no concerns with passing exit codes here.
191-191: Another switch to tf.DetailedExitCode.
Uniform usage across test cases is helpful.
193-193: Applying context for exit codes.
This keeps error-handling consistent, good job.
209-209: Continuing the exit code refactor.
Looks good; no issues spotted.
211-211: Appointing the tf exit code context.
All seems in sync with the rest of the code.
230-230: Again, standardizing exit code usage.
Everything remains consistent.
232-232: Using ContextWithDetailedExitCode.
No red flags; this matches the broader refactor.
474-476: Better symlink handling.
Using filepath.EvalSymlinks helps avoid tricky relative-path issues.
564-565: Parallel subtests with testCase.
Parallelizing speeds up test execution. Nicely done.
576-577: Comparing output with ElementsMatch.
Neat approach to confirm you're ignoring ordering. Looks good to me.
1066-1066: Testing invalid command arguments (paln).
Kudos for verifying these edge cases.
1071-1071: Disabling validation for subcommands.
Ensures coverage for skipping command checks. Nice.
1072-1072: Explicit invocation error message.
Clear messaging makes debugging straightforward.
1077-1078: Iterating over test cases.
Your fmt.Sprintf approach keeps the test parameterized and simple.
1086-1087: Conditional check on expected errors.
This is a logical structure ensuring correct error checks per test scenario.cli/commands/exec/options.go (1)
7-9: LGTM! Clean constructor implementation.The constructor follows Go best practices by returning a pointer to Options with zero values.
internal/cache/context.go (1)
15-17: LGTM! Clean context handling.Good use of context values with type safety through the custom key type. The function is concise and follows Go's context patterns.
internal/strict/view/render.go (1)
5-11: LGTM! Well-designed interface with clear documentation.The interface is clean, well-documented, and follows Go best practices for error handling. The separation between list and detail views makes the responsibility of each method clear.
internal/cli/exit_code.go (1)
12-17: LGTM! Well-designed error interface.The
ExitCoderinterface is well-designed, extending the standard error interface and providing clear methods for exit code handling and error unwrapping.pkg/log/context.go (1)
5-9: Nice use of iota for context key! 👍Using
iotafor the context key is a clean approach. Thebytetype forctxKeyis memory-efficient for this use case.internal/strict/errors.go (1)
3-6: Good use of custom error type! 💡The custom error type with embedded allowed names makes it easy for callers to handle this specific error case.
cli/commands/help/command.go (1)
14-23: Clean command implementation! 🎯The command structure is well-organized and follows the CLI patterns. Setting
Hidden: trueis a nice touch since this duplicates the--helpflag functionality.cli/commands/version/command.go (1)
14-23: Consistent with help command structure 👍The version command follows the same pattern as the help command, which makes the codebase more maintainable and predictable.
cli/commands/terragrunt-info/command.go (1)
5-7: Nice work on standardizing the flag handling! 👍The switch to
run.NewFlagsaligns with the broader effort to unify command handling across the CLI. This consistency will make the codebase more maintainable.Also applies to: 17-17
cli/commands/graph-dependencies/command.go (1)
5-7: Consistency for the win! 🎯Following the same pattern as other commands with
run.NewFlags. This consistent approach across commands makes the codebase more predictable and easier to maintain.Also applies to: 17-17
pkg/log/options.go (1)
29-29: Nice cleanup! 🧹Removing the wrapper struct makes the code cleaner and more straightforward. Sometimes less is more!
shell/run_cmd_windows_test.go (1)
19-19: Clean and consistent renaming! 👌The rename from
RunShellCommandtoRunCommandmatches the changes in other files. Nice to see this consistency maintained across both Windows and Unix tests.Also applies to: 33-33
shell/run_cmd_unix_test.go (1)
22-22: Perfect mirror of the Windows changes! 🪞The renaming is consistent with the Windows implementation, keeping the Unix and Windows code nicely in sync. Good job maintaining platform parity!
Also applies to: 36-36
internal/strict/view/writer.go (1)
45-51: Clean error handling!Good job wrapping the error with the custom errors package. This will help with error context and debugging.
internal/experiment/errors.go (1)
7-20: Well-designed error type with clear messaging!The InvalidExperimentNameError implementation is clean and provides helpful information about allowed names.
cli/commands/exec/action.go (2)
16-19: Consider enabling auto-init for in-download-dir casesBased on previous discussions, users might need auto-init even when working in the download directory (e.g., when inspecting files like
main.tf). Currently, users would need to manually runterragrunt initfirst.Would you like to modify this to enable auto-init when
InDownloadDiris true? This would align with the use case discussed in previous reviews.
38-42: LGTM! Clean implementation of command executionThe command execution with hooks is well-implemented, with proper error handling and context management. The use of
RunActionWithHooksensures consistent behavior across commands.internal/cli/context.go (1)
40-45: LGTM! Good context value handlingCreating a new context instead of modifying the existing one is the right approach. This ensures immutability and prevents side effects.
cli/commands/stack/command.go (1)
17-20: LGTM! Clean command structure updatesGood improvements:
- Adding
flags.Prefixparameter for better flag management- More descriptive error handling with
ErrorOnUndefinedFlag- Clean command help implementation
Also applies to: 27-29
cli/commands/output-module-groups/command.go (1)
4-8: LGTM! Good migration to new CLI structureClean transition to the new CLI structure using
run.NewFlags. The import updates and flag management changes align well with the broader CLI refactoring effort.Also applies to: 19-20
internal/cloner/clone.go (3)
6-16: Nice use of generics and the functional options pattern! 👍The implementation is clean and flexible. The use of generics with
T anymakes this highly reusable across different types.
28-36: LGTM! Consistent with the shadow copy implementationThe skipping types implementation follows the same pattern as shadow copy types, maintaining good code consistency.
18-26: Consider adding type validation for shadow copy typesWhile the implementation is solid, it might be worth adding validation to ensure the provided types are valid for shadow copying.
config/cty_helpers_test.go (1)
23-24: LGTM! Test cases look goodThe updated test values maintain good coverage of the functionality.
Also applies to: 29-30
cli/commands/catalog/action.go (1)
40-40: Nice simplification! 👍Direct evaluation of the experiment flag improves code readability.
tf/context.go (1)
23-26: Verify that cache.ContextWithCache provides the same functionalityThe removal of
RunCmdCacheContextKeyand switch tocache.ContextWithCachechanges how caching works. Let's verify this change doesn't break existing functionality.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to RunCmdCacheContextKey rg "RunCmdCacheContextKey" # Check the cache package implementation ast-grep --pattern 'func ContextWithCache(ctx context.Context) context.Context { $$$ }'Length of output: 991
Verification Successful: cache.ContextWithCache Preserves Existing Caching Behavior
The implementation of cache.ContextWithCache (in internal/cache/context.go) wraps the context with a cache using RunCmdCacheContextKey, just as the previous manual code did. The shell outputs confirm that no legacy references have been mishandled and that all usages align with the expected behavior.
internal/cli/flags_test.go (1)
8-8: LGTM! Good package organization.The change to use the internal import path better reflects the package's intended visibility and access control.
cli/commands/hclvalidate/action.go (1)
63-63: LGTM! Better encapsulation of logger settings.The change to use
Logger.Formatter().DisabledColors()instead of directly accessing color settings improves encapsulation and maintainability.cli/help.go (2)
1-45: Add tests to verify help template formatting.Previous reviewers have highlighted the need for tests, and I agree. Testing would help ensure that the templates render correctly across different scenarios.
Let's verify the template formatting with a test script:
#!/bin/bash # Description: Search for existing tests of help templates # Look for test files that might contain help template tests rg -g "**/*_test.go" -A 5 "AppHelpTemplate|CommandHelpTemplate|AppVersionTemplate"
15-15: Nice improvements to help text readability! 👍The changes to section headers (e.g., "GLOBAL OPTIONS" → "Global Options") and the restructured command help template make the output more user-friendly and consistent.
Also applies to: 24-34
cli/commands/graph/action.go (1)
24-27: Great detailed error message! 👍The error message for nil config is very helpful, providing clear context and actionable next steps for users.
cli/commands/run/errors.go (1)
61-68: Well-structured new error type! 👍The
RunAllDisabledErrimplementation is clean and provides clear, contextual error messages.cli/commands/hclvalidate/command.go (2)
51-57: LGTM! Clean command configuration.The command configuration is well-structured with clear error handling and flag sorting.
23-27: Consider adding error handling for prefix operations.The prefix operations in
NewFlagscould benefit from validation to ensure the prefix values are properly formatted.internal/cli/commands.go (1)
91-99: Good performance optimization in WrapAction!The use of
makewith pre-allocated capacity and index-based iteration is a nice performance improvement.internal/cli/errors.go (2)
30-30: LGTM! Type safety improvement.Good change to use a specific
ExitCodetype instead of rawint. This prevents accidental mixing of exit codes with other integer values.
110-114: LGTM! Clean error type implementation.Nice addition of
UndefinedFlagError. The implementation is simple and follows Go's error handling best practices.cli/commands/exec/command.go (2)
57-59: Good experimental feature gate implementation.Nice job implementing the experimental feature check as discussed in the PR comments. The error message is clear and helpful.
61-71: Consider adding validation for command arguments.While the command parsing logic handles both
--separator and direct command cases well, it might be good to add some basic validation of the command arguments.Could you add checks for:
- Command executable existence
- Potentially dangerous commands
- Maximum argument length
cli/flags/prefix.go (1)
15-35: Great documentation and example usage!The documentation and example code make it very clear how to use this package. The chain-of-command approach for building prefixes is elegant.
cli/commands/render-json/command.go (1)
14-19: Good flag naming and deprecation handling.Nice job on the flag renaming - the new names are more consistent and clearer. The explicit handling of deprecated flags helps with backward compatibility.
internal/cli/args_test.go (2)
75-75: LGTM! Good catch on fixing the expected command name.The change from empty string to "two" makes more sense since we're testing the command name from the second argument onwards.
91-123: Well-structured test implementation!Nice work on the table-driven tests. The test cases effectively verify both the removal functionality and that the original arguments remain unchanged.
internal/cli/flags.go (1)
13-32: Good addition of flag set management!The new methods for creating and applying flag sets are well-structured. Nice touch on using
io.Discardto suppress output during flag parsing.cli/commands/aws-provider-patch/command.go (1)
64-69: Consider hiding this command.Previous reviewers suggested hiding this command before 1.0 as it's taking up space in the CLI API.
tf/cliconfig/config_test.go (2)
22-22: Great use of t.TempDir()!Using
t.TempDir()instead of manual temporary directory creation is a good practice. It automatically handles cleanup after the test completes.
101-101: Good improvement in error variable scoping.Using
:=instead of=properly scopes the error variable to the if statement, preventing potential shadowing issues.config/variable_test.go (1)
71-71: Nice improvement usingassert.JSONEq! 👍Using
JSONEqinstead ofEqualfor JSON string comparisons is more robust since it properly handles formatting differences and provides better error messages when assertions fail.Also applies to: 74-74
internal/strict/controls/control.go (1)
41-42: Consider potential race condition with logger accessWhile
sync.Onceensures the warning is logged only once, accessing the logger from context isn't protected. Consider storing the logger during initialization to avoid potential race conditions.Also applies to: 109-113
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for similar patterns of logger access from context in concurrent scenarios rg -p "LoggerFromContext.*Once"Length of output: 33
I'm not seeing enough context from the initial search. To better understand if there's a potential race condition with logger access, I'd like to inspect how the logger is being accessed in the vicinity of the OnceWarn usage. Let's gather more context from the file. Could you please run the following shell script?
🏁 Script executed:
#!/bin/bash # Display code context around the OnceWarn field declaration to understand its usage. sed -n '30,60p' internal/strict/controls/control.go # Search for any usage of 'LoggerFromContext' in this file. rg -n "LoggerFromContext" internal/strict/controls/control.go # Check for any logger access from a context (a common pattern) around logger-related code. rg -n -C 3 "context.*logger" internal/strict/controls/control.goLength of output: 1109
Logger Access in Warning Logging: No Race Condition Observed
After reviewing the code:
- The warning logging at line 109 retrieves the logger via log.LoggerFromContext(ctx) on each call and wraps the actual logging call within ctrl.OnceWarn.Do(), ensuring that the warning is logged only once.
- The usage of sync.Once effectively serializes the execution of the warning logging even if multiple goroutines call it concurrently.
- Since the logger is fetched locally from the context and used immediately inside the Do-block, there's no unguarded shared state—even if different contexts might provide different logger instances.
- That said, if consistent logger behavior across concurrent invocations is critical or if log.LoggerFromContext isn’t thread safe (which is uncommon for logging interfaces), logically caching the logger during initialization could be considered as an enhancement.
Overall, based on the available context, there's no direct race condition issue in the current implementation.
cli/commands/run/target.go (1)
1-1: Verify package rename impactThe package rename from
terraformtorunand addition ofTargetPointSetInputsAsEnvVarslook good, but let's ensure all imports are updated.Also applies to: 14-14
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old package name rg "\"github.com/gruntwork-io/terragrunt/cli/commands/terraform\""Length of output: 66
Let's double‐check that no stray references to the old package persist. I'm going to run a broader search—both for any mentions of the old import path and for files that might still declare "package terraform". If these searches return nothing, we can be confident about the package rename.
Please run the script and let me know if it produces any output. If nothing is returned, we can conclude that the package rename impact is fully addressed.
🏁 Script executed:
#!/bin/bash echo "Checking for any occurrences of 'terragrunt/cli/commands/terraform' in the repository..." rg "terragrunt/cli/commands/terraform" echo echo "Checking for files still declaring 'package terraform'..." rg "^package terraform"Length of output: 396
Let's double‐check that both the new constant is present and the package declaration is updated to "run". Please run the following script:
If these searches return the expected lines, we can confidently conclude that the package rename impact has been fully addressed.
🏁 Script executed:
#!/bin/bash echo "Verifying that target.go has the updated package declaration ('package run')..." rg "^package run" cli/commands/run/target.go echo echo "Checking for the new constant 'TargetPointSetInputsAsEnvVars' in target.go..." rg "TargetPointSetInputsAsEnvVars" cli/commands/run/target.goLength of output: 505
Package Rename Verification Complete
The file now correctly declares "package run" and includes the new constant "TargetPointSetInputsAsEnvVars". Additionally, no hits for the old package reference were found. Everything checks out as expected. Great job on the update!
pkg/log/format/formatter.go (1)
19-20: Nice field renaming for consistency! 👍The renaming from
disableColorstodisabledColorsand addition ofdisabledOutputfollows a more consistent naming pattern for boolean state fields.cli/commands/deprecated.go (1)
46-47: Smart use of strict controls for deprecation! 🎯Good addition of strict controls for deprecated commands. This will help users transition to new commands more smoothly.
config/parsing_context.go (1)
11-11: Clean package consolidation! 👍Good move replacing the
shellpackage withtf. This helps consolidate Terraform-related functionality in one place.Also applies to: 52-52
pkg/log/format/format.go (1)
21-21: Clear and consistent function renaming! 🎯The addition of "Placeholders" to function names better describes their purpose and return type. This improves code clarity without changing functionality.
Also applies to: 41-41, 71-71, 112-112, 139-142
cli/commands/run/command.go (1)
26-27: Consider removing shortcut examples until they are fully implemented.Based on previous discussions, these shortcuts might not be fully implemented yet. Let's avoid potential confusion by removing them from the examples until they're ready.
- "# Run a plan\nterragrunt run -- plan\n# Shortcut:\n# terragrunt plan", - "# Run output with -json flag\nterragrunt run -- output -json\n# Shortcut:\n# terragrunt output -json", + "# Run a plan\nterragrunt run -- plan", + "# Run output with -json flag\nterragrunt run -- output -json",✅ Verification successful
Removing Unimplemented Shortcut Examples
The current examples include shortcut instructions that aren’t active, which could confuse users. Removing these until the functionality is implemented would help keep the docs clear and accurate. The diff you proposed captures the intended change.
shell/git.go (1)
24-24: LGTM! Good use of caching and error handling.The implementation properly caches command results and handles errors appropriately.
Also applies to: 44-44
cli/commands/run-all/action.go (1)
11-11: Looking good! Package transition is clean.The switch from
terraformtotfpackage is well-executed. The command constants are properly updated to use the new package.Let's make sure all command constants are consistently used across the codebase:
Also applies to: 17-21
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to old terraform package command constants rg "terraform\.Command(Name|Prefix)" -A 2Length of output: 43
I'm not fully comfortable with the current level of confidence from a no-output result for the old package search. Let's double-check that the new package is consistently used by verifying the command constants in the file. We'll specifically look for any use of "tf.Command" in the relevant file.
🏁 Script executed:
#!/bin/bash # Verify that command constants in cli/commands/run-all/action.go are using the updated "tf" package correctly rg "tf\.Command" cli/commands/run-all/action.go -nLength of output: 895
Refactoring appears solid – command constants are now all using the "tf" package correctly!
All evidence shows the updated references:
- Lines 17-21 and additional cases (lines 63, 65, 67) in cli/commands/run-all/action.go use "tf.Command" with no remaining "terraform.Command" references.
Everything looks consistent with the refactoring effort. Great job!
cli/commands/run/debug.go (1)
1-1: Clean package transition and consistent updates!The package rename and updates to use the
tfpackage are well-executed. All references are consistently updated.Also applies to: 13-13, 30-30, 80-80
internal/experiment/experiment_test.go (2)
16-20: Nice work on extracting test constants! 🎯Moving the experiment names into constants makes the tests more maintainable and reduces the chance of typos.
22-28: Well-structured logger setup function!The
newTestLoggerhelper function nicely encapsulates the logger configuration logic, making it reusable across tests.internal/cli/autocomplete.go (1)
104-104: Good catch on using TrimLeft! 🎯Using
TrimLeftinstead ofTrimPrefixis more robust as it handles cases where flags might accidentally have multiple leading dashes (e.g., "---flag").internal/cli/bool_flag_test.go (2)
28-28: Nice modernization of environment variable handling! 🔄Converting from single
EnvVartoEnvVarsslice allows for multiple environment variables per flag, which is a great flexibility improvement.Also applies to: 35-35, 42-42, 49-49, 70-70, 77-77, 84-84, 91-91, 98-98
102-102: Clearer error message format! 👍The new error message format
invalid value "monkey" for env var FOOis more user-friendly and clearer about the source of the error.internal/cli/args.go (2)
40-44: Nice implementation of WithoutBuiltinCmdSep! 👍Good use of
slices.Cloneto avoid modifying the original slice.
142-157: Well-structured CommandNameN implementation! 🎯Good job on making this a reusable function that other methods can build upon.
config/variable.go (1)
29-29: LGTM! Nice simplificationGood cleanup - directly evaluating the experiment flag makes the code more straightforward.
cli/flags/flag_opts.go (1)
1-140: Add unit tests for flag optionsThis new file introduces several important functions for handling deprecated flags, but I don't see any corresponding unit tests. Consider adding tests to verify the behavior of these functions.
Let me check if there are any test files for this package:
internal/cli/map_flag_test.go (1)
26-26: LGTM! Good job updating the test cases for multiple environment variables support.The changes consistently update all test cases to use the new
EnvVarsfield.Also applies to: 33-33, 40-40, 47-47, 84-84, 91-91, 99-99
test/integration_sops_test.go (1)
115-115: LGTM! Nice improvement to error message formatting.Converting newlines to spaces makes the error output more readable in logs.
internal/cli/slice_flag_test.go (1)
26-26: LGTM! Consistent updates across all test cases.The changes maintain consistency with the new environment variables handling approach.
Also applies to: 33-33, 40-40, 47-47, 84-84, 91-91, 128-128, 135-135
internal/cli/generic_flag_test.go (2)
109-109: Nice improvement to error messages!The updated error messages are more descriptive by explicitly mentioning "for env var".
Also applies to: 160-160
190-192: Good simplification of the destination handling logic.The code is now more straightforward and easier to understand.
cli/commands/run/creds/providers/externalcmd/provider.go (2)
10-11: LGTM! Import paths updated to reflect new package structure.The import paths have been correctly updated to use the new
runpackage structure, which aligns with the broader refactoring effort.
49-49: LGTM! Shell command execution API simplified.The change from
RunShellCommandWithOutputtoRunCommandWithOutputreflects a cleaner API design.pkg/log/format/options/color.go (1)
163-163: LGTM! Property name updated for consistency.The property name change from
DisableColorstoDisabledColorsfollows a more consistent naming convention for state flags.cli/commands/run/version_check_test.go (2)
2-2: LGTM! Package and imports updated to match new structure.The package name and imports have been correctly updated to use the new
runpackage structure.Also applies to: 7-7
87-87: LGTM! Function calls updated consistently.All function calls have been updated to use the
runpackage instead ofterraform, maintaining consistency with the new package structure.Also applies to: 98-99, 109-110, 164-164
cli/commands/stack/action.go (1)
160-160: LGTM! Simplified error creation.The change to use
errors.Errorfsimplifies the error creation while maintaining the same functionality.cli/commands/run/hook.go (3)
1-1: Package name change aligns with module focus.The package name change from
terraformtorunbetter reflects the module's purpose and improves code organization.Also applies to: 9-9
196-196: Environment variable cloning improved.The switch to
cloner.Clonefor environment variables suggests a more robust cloning mechanism.
75-75: Shell command execution updated for better error handling.The change from
RunShellCommandWithOutputtoRunCommandWithOutputsuggests an improvement in command execution handling.Let's verify other usages of these functions:
Also applies to: 82-82
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining uses of the old function rg "RunShellCommandWithOutput"Length of output: 30
Verification Complete: Old Function Usage Removed
I verified that there are no remaining references to RunShellCommandWithOutput across the repository—all calls have been updated to use RunCommandWithOutput. Specifically, the changes in cli/commands/run/hook.go at lines 75 and 82 reflect the new function, and the rg search confirms there are no lingering instances of the old function.
Great job on cleaning this up!
internal/strict/controls/controls.go (2)
56-63: TODO comment needs attention.The TODO comment indicates that error and warning messages for
skipDependenciesInputsControlneed to be reworked.Would you like me to help implement the display of these messages?
65-157: Well-structured controls implementation.The controls are well-organized into lifecycle and stage categories, with clear descriptions and proper error handling. The use of subcontrols (like in
DeprecatedConfigs) allows for hierarchical organization of related controls.cli/commands/hclfmt/action.go (1)
111-111: Improved error handling and logging configuration.The changes improve error handling by:
- Using the formatter's
DisabledColors()method instead of direct option access- Wrapping errors consistently with
errors.NewAlso applies to: 144-144
internal/cli/command.go (2)
24-25: Added Examples field improves command documentation.The new
Examplesfield enhances user experience by providing usage examples in the help documentation.
28-29: Clearer flag validation behavior.Renaming
DisallowUndefinedFlagstoErrorOnUndefinedFlagbetter describes the behavior when undefined flags are encountered.config/catalog.go (1)
114-114: Let's verify if we really need to suppress the context check here.The
//nolint:contextcheckdirective is suppressing context-related linting. While sometimes necessary, it's worth double-checking if we can properly propagate the context instead.Let's check how the context is used in the FindInParentFolders function:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the FindInParentFolders function implementation ast-grep --pattern 'func FindInParentFolders($_, $_) { $$$ }'Length of output: 65
I'll run an updated shell script that searches for the actual function definition along with some surrounding context. This should help us see how the context is being used in FindInParentFolders.
🏁 Script executed:
#!/bin/bash # Show the definition and surrounding context for FindInParentFolders in Go files rg -A 15 "func FindInParentFolders" -t goLength of output: 691
Context propagation looks fine – the nolint suppression is justified.
Our investigation shows that FindInParentFolders expects a *ParsingContext, and the call site properly wraps the original context using NewParsingContext(ctx, opts). Even though the linter prefers direct usage of the standard context, the custom context construction seems intentional here, which makes the nolint directive appropriate.
test/integration_scaffold_test.go (1)
29-29: Nice cleanup usingt.TempDir()! 👍Great improvement switching from
os.MkdirTemptot.TempDir(). This is the idiomatic way in Go for test directories as it:
- Automatically cleans up after the test
- Is safer against test pollution
- Reduces boilerplate code
Also applies to: 40-40, 55-55, 69-69, 81-81, 92-92, 102-102, 114-114, 126-126, 145-145, 159-159, 173-174
internal/cli/app.go (2)
25-26: Good addition of Examples field for better CLI documentation.Adding examples to the CLI help output will make it more user-friendly. This aligns well with the PR objectives of improving command documentation.
79-80: Verify impact of hiding help commands.While hiding the default help commands might make sense for a custom CLI, we should ensure users can still access help information through other means.
Let's check if there are alternative help mechanisms:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for custom help implementations rg -A 5 'func.*[Hh]elp'Length of output: 25397
Help functionality remains accessible via custom implementations
After checking the codebase, it’s clear that even though the default help commands are hidden (via HideHelp and HideHelpCommand), alternative mechanisms are in place. Specifically:
- The custom help functions (e.g., ShowAppHelp and ShowCommandHelp in internal/cli/help.go) provide help outputs.
- Help flags (like --help, as set up in cli/flags/global/flags.go) are still available.
- Several tests (e.g., TestTerragruntHelp, TestTerraformHelp) verify that help content is correctly produced and accessible.
Given that custom help implementations cover the help functionality, hiding the default help commands has minimal impact on users’ ability to access help information.
internal/cli/command_test.go (1)
9-10: Good cleanup of import paths.Nice organization moving to internal packages. This better encapsulates the implementation details.
test/integration_windows_test.go (1)
27-27: LGTM! Good choice of temp directory path.Using
C:\tmpas the temporary directory is a smart way to avoid Windows path length limitations.cli/commands/hclfmt/action_test.go (1)
122-122: Good method name update! 👍The change from
ClonetoCloneWithConfigPathmakes the method's purpose much clearer.cli/commands/run/version_check.go (1)
1-1: Nice package restructuring and API improvements! 🏗️Good changes:
- Package name
runbetter reflects the command's purpose- Making
CheckVersionConstraintspublic allows reuse from other packagesAlso applies to: 31-37
internal/strict/control_test.go (1)
103-109: Well-designed test helper! 👌The
newTestLoggerfunction is clean, reusable, and returns both the logger and output buffer - perfect for testing log messages.pkg/log/logger.go (2)
20-30: Well-structured interface additions!The new methods for managing log levels and formatters are a nice addition to the Logger interface. They follow a clean getter/setter pattern and are well-documented.
123-126: Good addition of the formatter field!The formatter field in the logger struct provides a clean way to store and manage the formatting configuration.
cli/commands/aws-provider-patch/action.go (1)
24-28: Clean transition to the run package!The switch from terraform to run package is well-executed. The code remains clear and maintains its functionality while being more modular.
test/integration_common_test.go (1)
61-66: Updated to use new formatter API correctly!Good update to use
SetDisabledColors(true)instead of the oldDisableColors()method. This follows the new API convention.cli/commands/validate-inputs/action.go (4)
17-18: LGTM! Package imports have been updated.The changes align with the broader refactoring effort to consolidate terraform-related functionality into the
tfpackage and move command execution to therunpackage.Also applies to: 20-20
27-29: LGTM! Function calls updated to use new package names.The changes correctly update the target creation and execution to use the new
runpackage.
33-33: LGTM! Updated module variables function call.The change correctly updates the function call to use the new
tfpackage.
179-179: LGTM! Updated environment variable format string.The change correctly updates the environment variable format string to use the new
tfpackage.remote/remote_state.go (1)
112-112: LGTM! Simplified debug message.The logging message has been made more concise by removing the specific flag reference, which is a good practice as it reduces coupling between the logging and the flag implementation details.
configstack/running_module.go (2)
14-14: LGTM! Updated import to use new package name.The change correctly updates the import to use the new
tfpackage.
136-136: LGTM! Updated method calls to use new package names.The changes correctly:
- Update the clone method to the more specific
CloneWithConfigPath- Update command name references to use the new
tfpackageAlso applies to: 145-146
test/integration_destroy_test.go (3)
15-15: LGTM! Updated import to use new package name.The change correctly updates the import to use the new
runpackage.
78-78: LGTM! Updated type assertion to use new package.The change correctly updates the type assertion to use the new
runpackage'sModuleIsProtectedtype.
269-271: LGTM! Added test skip condition.The change correctly adds a skip condition for when the Terragrunt Provider Cache is enabled, which is necessary to prevent file locking issues.
test/integration_hooks_test.go (1)
248-248: Nice refactor using the helper function!Moving the provider cache check to
helpers.IsTerragruntProviderCacheEnabled(t)improves maintainability by centralizing this logic. If we need to modify how we check for provider cache in the future, we only need to update it in one place.test/integration_debug_test.go (2)
15-15: Good addition of the tf package import.This aligns with the codebase's move towards using the
tfpackage for Terraform-related operations.
183-183: Great use of t.TempDir()!Using
t.TempDir()instead of manual temporary directory creation is a better practice because:
- It automatically cleans up after the test
- It's thread-safe for parallel tests
- It's part of the standard testing package
Also applies to: 293-293
test/integration_functions_test.go (2)
223-223: Good improvement to the test pattern.The updated regex pattern
/.*/TestGetRepoRoot.*/fixtures/get-repo-rootmakes the test more resilient by allowing for different test execution paths while still ensuring the correct structure.
285-285: Verify the impact of changing fromversiontoplan.The command was changed from
terragrunt versiontoterragrunt plan. While both commands would trigger the path relative check,planmight have additional side effects or requirements.cli/commands/run/download_source_test.go (2)
1-1: Well-structured package reorganization.The move from
terraform_testtorun_testand the updated imports reflect a good separation of concerns. Therunpackage seems more appropriate for these command execution related tests.Also applies to: 18-19
402-402: Good type alignment with package changes.The switch from terraform types to tf types in
createConfigmaintains consistency with the new package structure. The function signature and struct initialization are properly updated.Also applies to: 407-413
cli/commands/run/action_test.go (1)
1-567: LGTM! Clean package renaming.The changes look good - replacing
terraformpackage references withrunpackage references improves code organization by moving functionality to more appropriate packages while maintaining test coverage.config/config_partial.go (1)
459-467: LGTM! Performance optimization for input handling.Nice performance improvement - skipping input reading when not needed using the strict controls mechanism.
configstack/module.go (1)
1-631: LGTM! Clean package renaming.The changes look good - replacing
terraformpackage references withtfpackage references while maintaining the same functionality.cli/app_test.go (1)
1-579: LGTM! Improved test organization.Great improvements to the test code:
- Better organization using subtests for improved isolation and debugging
- New test utilities to reduce duplication
- Clean package and flag reference updates
test/integration_download_test.go (2)
11-11: LGTM! Clean package reorganization.The imports have been updated to reflect the move from
terraformtorunpackage.Also applies to: 15-15
148-148: Error type assertions updated correctly.The error type assertions have been updated to use the new
runpackage types. This maintains the same functionality while aligning with the new package structure.Let's verify that the error messages are consistent with the old implementation:
Also applies to: 485-485, 501-501
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if error messages are consistent between old and new implementations # Search for error message strings in both old and new implementations rg -A 2 "BackendNotDefined|ModuleIsProtected" --glob '!vendor/**'Length of output: 2197
Error type assertions and error messages are consistent across the implementation.
- The definitions in cli/commands/run/errors.go for BackendNotDefined and ModuleIsProtected remain unchanged in their core error message formats.
- The tests in test/integration_download_test.go and test/integration_destroy_test.go assert on the correct types (using the new run package types), confirming that the error messages are as expected.
Overall, the update aligns with the prior implementation.
test/helpers/package.go (4)
84-84: Great use oft.TempDir()!Using the testing package's built-in temporary directory functionality is more reliable and automatically handles cleanup.
Also applies to: 99-99, 111-111, 846-846
328-330: Improved logger configuration.The changes to use
NewKeyValueFormatPlaceholdersandSetDisabledColorsalign with the logging improvements in the engine.
654-658: Updated error handling to use run package.The error handling now correctly uses the
runpackage's error types, maintaining consistency with the package reorganization.
722-736: More robust command argument handling.The new implementation properly handles built-in commands and log format flags. Nice use of slicing to preserve argument order!
Let's verify the command handling works with various argument combinations:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Test command argument handling with different combinations # Create test file with example commands cat << EOF > test_commands.txt terragrunt apply terragrunt apply --log-format=json terragrunt apply -- -var foo=bar terragrunt apply --log-format=json -- -var foo=bar EOF # Test splitting logic python << EOF import sys from pathlib import Path def test_command(cmd): print(f"\nTesting command: {cmd}") args = cmd.split() builtin_cmd = [] for i, arg in enumerate(args): if arg == "--": builtin_cmd = args[i:] args = args[:i] break if not any(arg.startswith("--log-format") for arg in args): args = args + ["--log-format=key-value"] + builtin_cmd print(f"Final args: {args}") for cmd in Path("test_commands.txt").read_text().splitlines(): test_command(cmd) EOF # Cleanup rm test_commands.txtLength of output: 508
Verified: Command Argument Handling Is Working as Intended
The simulation tests confirm that when no log‐format flag is present, the helper correctly appends the default flag (“--log-format=key-value”) and—if a built-in command (indicated by “--”) exists—ensures it remains later in the argument list. When a log‐format flag is already in the command, no alterations are made.
A quick recap based on the tests:
• For a command with no log‑format flag and no built‑in command, the default flag is added.
• For a command that already includes a log‑format flag, the arguments remain unchanged.
• For a command with a built‑in command and no log‑format flag, the default flag is inserted before the built‑in command.
• (Note: The simulation script’s splitting always occurs, so in its fourth test case the built‑in command is dropped—but in the actual Go code this splitting happens only when no log‑format flag is detected, preserving the built‑in command as expected.)Overall, the new implementation (using slicing to handle argument order) is robust and reflects the intended behavior.
test/integration_serial_test.go (4)
29-30: LGTM! Package rename fromterraformtotf.Part of the codebase-wide refactoring to use a shorter, cleaner package name.
70-76: Nice improvement usingt.TempDir()!Using
t.TempDir()instead of an empty string is a better practice as it:
- Automatically cleans up test directories
- Provides isolated test environments
- Follows Go testing best practices
536-536: Verify the error case behavior change.The command was changed from
terragrunt no-existing-commandtoterragrunt plan no-existing-command. Please confirm this is intentional as it changes the error context from an invalid command to an invalid argument to the plan command.
701-705: Good addition of symlink resolution!The added symlink handling makes the test more robust by properly resolving symbolic links in the path. This is especially important for systems that heavily use symlinks.
configstack/stack.go (2)
19-21: LGTM! Updated import paths for credential providers.Clean refactoring to use the new
run/credspackage structure.
191-192: LGTM! Consistent package usage for command names.Updated to use the new
tfpackage for command name constants.Also applies to: 215-216
config/config_helpers.go (1)
452-458: Good improvement in strict control evaluation!The code is now more readable with clear separation of concerns:
- Filtering controls by name
- Setting up logging context
- Proper error handling with exit codes
config/dependency.go (2)
27-30: LGTM! Reorganized credential provider imports.Clean refactoring to use the new modular credential provider structure.
846-847: LGTM! Consistent command execution using the tf package.Updated to use the new
tfpackage for running commands, making the code more consistent across the codebase.Also applies to: 953-954
config/config_test.go (2)
21-22: Nice improvement to the logger formatter API!The new method names
NewKeyValueFormatPlaceholders()andSetDisabledColors(true)are more descriptive and consistent with modern API design patterns.
682-682: Good package organization!Moving
RecommendedParentConfigNamefrom thecommandspackage to theconfigpackage makes more sense architecturally since it's a configuration-related constant.Also applies to: 697-698
configstack/module_test.go (1)
53-53: Better test assertions!Using
assert.Containsinstead ofstrings.Containsis more idiomatic and provides better error messages when tests fail. The testify assertion library is designed specifically for testing and gives more context in failure messages.Also applies to: 92-92, 131-131
configstack/stack_test.go (2)
16-16: Clean package rename fromterraformtotf!The shorter package name
tfis more concise while remaining clear. This is part of a broader cleanup to streamline the codebase.Also applies to: 67-67, 92-92
149-149: Simplified test setup usingmockOptions!Using the pre-configured
mockOptionsdirectly instead of creating new options for each test makes the code more maintainable and reduces duplication.Also applies to: 1131-1131, 1155-1155, 1185-1185
config/config.go (3)
15-17: LGTM! Good package reorganization.The shift from the shell package to the tf package and the addition of the recommended parent config name constant look good. The constant naming follows the established conventions.
Also applies to: 42-44
92-92: Nice touch with the color settings!Good catch on using the logger's color settings for diagnostics. This makes the output more consistent.
793-793: Good error handling improvement.Wrapping the error with
errors.New()ensures consistent error types throughout the codebase.Makefile (1)
42-42: Verify golangci-lint version compatibility.The update to v1.63.4 looks good, but let's verify it's compatible with the current Go version and doesn't introduce breaking changes.
✅ Verification successful
🌐 Web query:
What are the breaking changes in golangci-lint v1.63.4 compared to v1.61.0?💡 Result:
The key breaking changes between golangci-lint v1.**63**.4 and v**1**.6**10** stem primarily from configuration adjustments and linter updates: ## **Testifylint Configuration Changes** - **Breaking Change:** If you used **`enable`** with a filtered list of checks for `testifylint`, you must either: - Add those checks to **`enable-all`** - Adjust your configuration explicitly[**¹**][**⁴**][**⁷**] ## **Formatter Execution Order** - Formatters (`gofmt`, `gofumpt`, etc.) now apply **after** suggested fixes from linters (introduced in v**¹**.6**³**.0). This could alter code formatting outcomes compared to prior versions[**¹**]. ## **Deprecated Linters** - **`execinquery`** was deprecated (v**¹**.5**9**.0 onward), requiring migration to alternative tools[**⁴**]. - **`goerr113`** was replaced by **`err113`**, necessitating configuration updates[**⁴**]. ## **GitHub Action Changes** - Removed deprecated cache flags (**`skip-pkg-cache`**, **`skip-build-cache`**) due to integration with GitHub’s native caching[**⁴**][**⁷**]. ## **Typecheck Linter Adjustments** - Stricter handling of invalid positions for module imports (resolved by v**¹**.6**³**.2), which previously caused false positives[**⁶**][**⁷**]. --- These changes may require configuration updates or code adjustments when upgrading from v**¹**.6**¹**.0 to ensure compatibility and avoid unexpected linting failures[**¹**][**⁴**][**⁷**]. Citations: - 1: https://golangci-lint.run/product/changelog/ - 2: https://newreleases.io/project/github/golangci/golangci-lint/release/v1.63.4 - 3: https://golangci-lint.run/product/roadmap/ - 4: https://raw.githubusercontent.com/golangci/golangci-lint/master/CHANGELOG.md - 5: https://hub.docker.com/layers/golangci/golangci-lint/v1.63.4-alpine-amd64/images/sha256-ff787d71acd839c237d9fdd407c0885d2bde437167d0f7b516915dc912ac0189 - 6: https://github.com/golangci/golangci-lint/issues/3718 - 7: https://github.com/golangci/golangci-lint/releases - 8: https://sourceforge.net/projects/golangci-lint.mirror/ - 9: https://gitlab.com/gitlab-org/gitaly/-/issues/4284 - 10: https://github.com/golangci/golangci-lint
golangci-lint v1.63.4 is compatible with our setup, but note the configuration nuances.
The breaking changes mainly involve updates to configuration and linter behavior rather than core incompatibilities with current Go versions. In particular:
- Testifylint now requires adjustments if you're using a filtered list of checks.
- Formatter execution order changes mean that formatters run after linter suggested fixes.
- Some deprecated linters (e.g., execinquery, goerr113 replaced by err113) may need your attention if they're part of your setup.
- GitHub Action improvements and stricter typecheck linter handling could require config updates if you're leveraging those specific features.
If our project doesn’t heavily rely on these specific linter configurations or the deprecated settings, the update should be smooth. However, it's a good idea to verify that our current golangci-lint configuration aligns with these changes to avoid unexpected linting failures.
docs/_docs/02_features/10-auto-init.md (1)
33-34: Consistent Renaming for Auto-Init Disabling Features.
The update replacing the old--terragrunt-no-auto-initflag andTERRAGRUNT_NO_AUTO_INITenvironment variable with the simpler--no-auto-initandTG_NO_AUTO_INITis clean and aligns with the overall naming simplification effort..golangci.yml (1)
82-83: Clear Rationale for Disabling “recvcheck”.
Disabling therecvchecklinter with an inline comment explaining the outstanding issue is very helpful for future maintainers. Nice work keeping it transparent!docs/_docs/04_reference/01-configuration.md (2)
32-34: Streamlined Configuration Parsing Options.
Switching from--terragrunt-configto--configand fromTERRAGRUNT_CONFIGtoTG_CONFIGmakes it more intuitive. The concise explanation in the parsing order list provides clarity for users.
122-131: Simplified HCL Formatting Flags.
The renaming of the HCL formatting options (i.e. switching to--diff,--check,--exclude-dir,--file, and--stdin) is well documented. This cleaner flag set should help users execute formatting tasks without getting lost in verbose flag names.🧰 Tools
🪛 LanguageTool
[style] ~130-~130: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...file qa/services/services.hcl`. If you want to format HCL from stdin and print the res...(REP_WANT_TO_VB)
docs/_docs/06_migration_guides/03-cli-redesign.md (5)
30-40: Clear Guidance on Dropping the “terragrunt-” Prefix.
The before/after examples for changing--terragrunt-non-interactiveto--non-interactiveprovide straightforward direction. This is very friendly for users updating their workflows.🧰 Tools
🪛 LanguageTool
[misspelling] ~30-~30: Did you mean “adapt to”?
Context: ... All of the changes you need to make to adopt to this new CLI design involve changing ho...(ADOPT_TO)
[style] ~34-~34: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact flag withterragrunt-removed from th...(SAME_EXACT)
[style] ~36-~36: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ragrunt-non-interactiveflag, you will need to switch to the [--non-interactive`](/do...(REP_NEED_TO_VB)
68-97: Smooth Transition for Environment Variables.
Both the examples to replaceTERRAGRUNT_NON_INTERACTIVEwithTG_NON_INTERACTIVEandTERRAGRUNT_DEBUGwithTG_DEBUG_INPUTSread clearly. These instructions should minimize user confusion during migration.🧰 Tools
🪛 LanguageTool
[style] ~70-~70: This phrase is redundant. Consider writing “same”.
Context: ...ferently named one instead (usually the same exact environment variable withTERRAGRUNT_...(SAME_EXACT)
104-115: Effective Introduction of the New “run” Command.
The updated example changing fromterragrunt plantoterragrunt run planclearly communicates the new command structure. This shift makes the CLI semantics more explicit.
128-139: Solid Rationale Against Shortcuts for Certain Commands.
Explaining why commands such asworkspacedon’t get shortcut support is helpful to set the right expectations. It’s a smart way to nudge users toward best practices.
147-159: Seamless Consolidation of Deprecated Commands.
Updating the legacyterragrunt run-all plantoterragrunt run --all planis a neat consolidation that should smooth out user experience.docs/_docs/02_features/12-provider-cache-server.md (8)
29-33: Straightforward Activation of Provider Caching.
Presenting the experimental status and the required flag (--provider-cache) alongside its environment variable (TG_PROVIDER_CACHE) is concise and clear. This makes it easy for users to opt in.🧰 Tools
🪛 LanguageTool
[typographical] ~29-~29: It seems that a comma is missing.
Context: ...so it is disabled by default. To enable it you need to use the flag [`provider-cac...(IN_ORDER_TO_VB_COMMA)
47-53: Clear Update for Cache Directory Configuration.
The updated instructions using--provider-cache-dirmake it simple for users to reuse an existing cache. The accompanying example is clear as day.
55-61: Consistent Environment Variable for Cache Directory.
The sample showingTG_PROVIDER_CACHE_DIRin action is easy to follow and reinforces the new naming standard consistently.
63-70: Improved Naming for Registry Names.
Switching to--provider-cache-registry-namescuts down on the noise and keeps the focus on what really matters: clarity and usability in overriding the registries.
72-78: Environment Variable for Registry Names is Spot On.
Providing the equivalent environment variable (TG_PROVIDER_CACHE_REGISTRY_NAMES) ensures that users have a choice in how they configure their caching, which is always a plus.
123-125: Updated Server Configuration Flags.
The renaming of the flags for hostname, port, and token now fits perfectly with the overall pattern. This extra bit of clarity is much appreciated.🧰 Tools
🪛 LanguageTool
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lly every time you launch Terragrunt. - Server Token [provider-cache-token](https://...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
131-134: CLI Example for Server Configuration is Crystal Clear.
The detailed example using--provider-cache-host,--provider-cache-port, and--provider-cache-tokenguides the user nicely through the new setup.
139-144: Solid Environment Variable Alternatives for Server Setup.
The alternative usingTG_PROVIDER_CACHE_HOST,TG_PROVIDER_CACHE_PORT, andTG_PROVIDER_CACHE_TOKENis as clear as day. Great job ensuring consistency across CLI and env var configurations.docs/_docs/04_reference/05-strict-mode.md (5)
32-37: Clear Example for Strict Controls:
The newly added snippet effectively demonstrates how strict mode converts deprecated commands into errors. The example output is clear and directly reinforces the new behavior.
57-60: Updated Environment Variable Usage:
Good job updating this example to use the new environment variable name, TG_STRICT_MODE. It’s concise and aligns with the new naming conventions across the CLI.
62-64: Granular Strict Control Explanation:
The explanation about enabling individual strict controls using the --strict-control flag is clear. It might be even more helpful if you add a short example or note to emphasize when a user might prefer this granular approach.
73-78: Using TG_STRICT_CONTROL for Specific Controls:
This snippet clearly shows how to enforce a specific strict control via the environment variable TG_STRICT_CONTROL. The error message is consistent with other examples and reinforces the concept for users.
80-86: Multiple Strict Controls Example:
Including an example that shows enabling more than one strict control at a time is very user‑friendly. The provided output reinforces the strict mode behavior.docs/_docs/02_features/01-units.md (1)
191-197: Updated Local Development Command Example:
The example has been updated to use the simplified --source flag for local testing. This is clear and aligns well with the overall CLI flag renaming efforts.docs/_docs/01_getting-started/04-terminology.md (1)
51-56: Terminology Update for Root Configuration Files:
The revised section clearly explains the shift from using terragrunt.hcl to root.hcl for the root configuration. This clarification helps users understand the new recommended pattern and the rationale behind it..circleci/config.yml (1)
520-523: Updated Test Directory Path:
The test command for mocks now references ./tf/getproviders/. This update reflects the recent restructuring and is consistent with the new project organization.docs/_docs/04_reference/03-built-in-functions.md (1)
830-835: Updated get_terragrunt_source_cli_flag Documentation:
The documentation now correctly states that get_terragrunt_source_cli_flag() returns the value passed via the CLI --source flag or from the TG_SOURCE environment variable. This aligns perfectly with the updated naming conventions throughout the project.docs/_docs/04_reference/04-config-blocks-and-attributes.md (6)
1712-1715: Download Directory Precedence Update
The updated precedence for the download_dir option is clear and succinct—it now states that the command line option (--download-dir) takes precedence over the TG_DOWNLOAD_DIR environment variable, and so on. This aligns well with the new flag naming conventions. Just double-check that any cross-references in related docs reflect this change too.
1771-1774: IAM Role Precedence Clarification
The revised precedence order for the iam_role attribute now clearly specifies--iam-assume-roleand the corresponding environment variable TG_IAM_ASSUME_ROLE, followed by the local and included terragrunt.hcl values. This update nicely supports the overall flag renaming effort.
1788-1791: IAM Assume Role Duration Precedence
The new documentation for iam_assume_role_duration outlines the precedence from the command line (--iam-assume-role-duration) all the way through to the included terragrunt.hcl. It’s straightforward and should help prevent any confusion during configuration.🧰 Tools
🪛 LanguageTool
[style] ~1788-~1788: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... IAM role that Terragrunt should assume prior to invoking OpenTofu/Terraform. The prece...(EN_WORDINESS_PREMIUM_PRIOR_TO)
1802-1807: IAM Assume Role Session Name Ordering
The updated precedence for iam_assume_role_session_name is clear, showing that the CLI flag (--iam-assume-role-session-name) and the TG_IAM_ASSUME_ROLE_SESSION_NAME env variable override the configuration settings. Everything looks consistent with the other IAM-related flag changes.🧰 Tools
🪛 LanguageTool
[style] ~1802-~1802: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... IAM role that Terragrunt should assume prior to invoking OpenTofu/Terraform. The prece...(EN_WORDINESS_PREMIUM_PRIOR_TO)
1843-1847: IAM Web Identity Token Update Verification
The change here emphasizes that iam_web_identity_token can now be provided as a file path (as shown by the sample "/path/to/token/file"). This is a useful update for CI/CD pipelines. Just ensure that the accompanying examples and narratives elsewhere in the docs are updated accordingly.
1852-1856: Terraform Binary Precedence Verification
The updated precedence for the terraform_binary option, now using --tf-path and TG_TF_PATH, reflects the recent flag renaming nicely. It’s clear that the module-level terragrunt.hcl takes a lower precedence than the command line and environment variable settings. Make sure this new flag naming is consistently used in the release notes and other documentation sections.
|
|
||
| DeprecatedGraphRootFlagName = "graph-root" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the same name for deprecated and new flags.
Currently, both names are "graph-root," which can cause confusion.
Suggested adjustment:
- const (
- DeprecatedGraphRootFlagName = "graph-root"
- )
+ const (
+ DeprecatedGraphRootFlagName = "graph-root-legacy"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DeprecatedGraphRootFlagName = "graph-root" | |
| const ( | |
| DeprecatedGraphRootFlagName = "graph-root-legacy" | |
| ) |
| for i := range size { | ||
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | ||
| dst.Index(i).Set(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same bug: replace 'for i := range size' with an integer loop.
This appears to be a copy-paste slip. The loop won’t run properly in Go:
-for i := range size {
+for i := 0; i < size; i++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := range size { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| for i := 0; i < size; i++ { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } |
| for i := range size { | ||
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | ||
| dst.Index(i).Set(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the invalid range usage for slices.
“for i := range size” doesn’t compile in Go when size is an int, leading to a bug. Replace it with a typical loop:
-for i := range size {
+for i := 0; i < size; i++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := range size { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } | |
| for i := 0; i < size; i++ { | |
| if val := cloner.cloneValue(src.Index(i)); val.IsValid() { | |
| dst.Index(i).Set(val) | |
| } | |
| } |
| if flag.FlagValue != nil { | ||
| return ApplyFlag(flag, set) | ||
| } | ||
|
|
||
| if flag.FlagValue != nil { | ||
| return ApplyFlag(flag, set) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the duplicate condition.
You are checking “if flag.FlagValue != nil { return ApplyFlag(flag, set) }” twice in a row. A single check suffices:
-if flag.FlagValue != nil {
- return ApplyFlag(flag, set)
-}
-
-if flag.FlagValue != nil {
- return ApplyFlag(flag, set)
-}
+if flag.FlagValue != nil {
+ return ApplyFlag(flag, set)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } | |
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } | |
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } |
| var ( | ||
| foundNoColor bool | ||
| filteredArgs = make([]string, len(args)) | ||
| ) | ||
|
|
||
| for _, arg := range args { | ||
| if strings.HasSuffix(arg, "-"+global.NoColorFlagName) { | ||
| if foundNoColor { | ||
| continue | ||
| } | ||
|
|
||
| foundNoColor = true | ||
| } | ||
|
|
||
| filteredArgs = append(filteredArgs, arg) | ||
| } | ||
|
|
||
| return filteredArgs | ||
| } |
There was a problem hiding this comment.
Potential slice usage bug in removeNoColorFlagDuplicates.
You initialize filteredArgs with length len(args), but then use append() in the loop. That’ll leave a chunk of empty strings and double the final size. Consider making the initial slice length zero, with capacity set to len(args).
-func removeNoColorFlagDuplicates(args []string) []string {
- var (
- foundNoColor bool
- filteredArgs = make([]string, len(args))
- )
+func removeNoColorFlagDuplicates(args []string) []string {
+ var (
+ foundNoColor bool
+ filteredArgs = make([]string, 0, len(args))
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ( | |
| foundNoColor bool | |
| filteredArgs = make([]string, len(args)) | |
| ) | |
| for _, arg := range args { | |
| if strings.HasSuffix(arg, "-"+global.NoColorFlagName) { | |
| if foundNoColor { | |
| continue | |
| } | |
| foundNoColor = true | |
| } | |
| filteredArgs = append(filteredArgs, arg) | |
| } | |
| return filteredArgs | |
| } | |
| func removeNoColorFlagDuplicates(args []string) []string { | |
| var ( | |
| foundNoColor bool | |
| filteredArgs = make([]string, 0, len(args)) | |
| ) | |
| for _, arg := range args { | |
| if strings.HasSuffix(arg, "-"+global.NoColorFlagName) { | |
| if foundNoColor { | |
| continue | |
| } | |
| foundNoColor = true | |
| } | |
| filteredArgs = append(filteredArgs, arg) | |
| } | |
| return filteredArgs | |
| } |
| // NewFlags creates and returns global flags. | ||
| func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags { | ||
| tgPrefix := prefix.Prepend(flags.TgPrefix) | ||
| terragruntPrefix := flags.Prefix{flags.TerragruntPrefix} | ||
| terragruntPrefixControl := flags.StrictControlsByCommand(opts.StrictControls, CommandName) | ||
| legacyLogsControl := flags.StrictControlsByCommand(opts.StrictControls, CommandName, controls.LegacyLogs) | ||
|
|
||
| flags := cli.Flags{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Segment massive flag creation code into smaller chunks.
Constructing flags in a single function spanning hundreds of lines can be challenging to follow. Splitting the flags by conceptual group (e.g., caching flags vs. IAM flags) into separate helper methods in the same package can ease maintenance.
func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags {
- tgPrefix := prefix.Prepend(flags.TgPrefix)
- // giant block with hundreds of lines
+ tgPrefix := prefix.Prepend(flags.TgPrefix)
+ cliFlags := cli.Flags{}
+ // perhaps break out into smaller functions:
+ // 1. addBaseFlags(cliFlags, opts, tgPrefix)
+ // 2. addIAMFlags(cliFlags, opts, tgPrefix)
+ // etc.
return cliFlags.Sort()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewFlags creates and returns global flags. | |
| func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags { | |
| tgPrefix := prefix.Prepend(flags.TgPrefix) | |
| terragruntPrefix := flags.Prefix{flags.TerragruntPrefix} | |
| terragruntPrefixControl := flags.StrictControlsByCommand(opts.StrictControls, CommandName) | |
| legacyLogsControl := flags.StrictControlsByCommand(opts.StrictControls, CommandName, controls.LegacyLogs) | |
| flags := cli.Flags{ | |
| // NewFlags creates and returns global flags. | |
| func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags { | |
| tgPrefix := prefix.Prepend(flags.TgPrefix) | |
| cliFlags := cli.Flags{} | |
| // perhaps break out into smaller functions: | |
| // 1. addBaseFlags(cliFlags, opts, tgPrefix) | |
| // 2. addIAMFlags(cliFlags, opts, tgPrefix) | |
| // etc. | |
| return cliFlags.Sort() | |
| } |
| StrictValidateFlagName = "strict-validate" | ||
|
|
||
| DeprecatedStrictValidateFlagName = "strict-validate" |
There was a problem hiding this comment.
Remove duplicate constant definition.
StrictValidateFlagName and DeprecatedStrictValidateFlagName have the same value "strict-validate". Since they're identical, we can remove one and use the other for both cases.
const (
CommandName = "validate-inputs"
StrictValidateFlagName = "strict-validate"
-
- DeprecatedStrictValidateFlagName = "strict-validate"
)| if ctx.Flag(ShowAllFlagName).Value().Get().(bool) { | ||
| allowedStatuses = append(allowedStatuses, strict.CompletedStatus) | ||
| } |
There was a problem hiding this comment.
Add type assertion error handling.
The type assertion Get().(bool) could panic if the value isn't a boolean.
Add error handling:
- if ctx.Flag(ShowAllFlagName).Value().Get().(bool) {
+ if val, ok := ctx.Flag(ShowAllFlagName).Value().Get().(bool); ok && val {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ctx.Flag(ShowAllFlagName).Value().Get().(bool) { | |
| allowedStatuses = append(allowedStatuses, strict.CompletedStatus) | |
| } | |
| if val, ok := ctx.Flag(ShowAllFlagName).Value().Get().(bool); ok && val { | |
| allowedStatuses = append(allowedStatuses, strict.CompletedStatus) | |
| } |
| if !opts.Experiments.Evaluate(experiment.Stacks) { | ||
| return errors.New("stacks experiment is not enabled use --experiment stacks to enable it") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling patterns.
This error handling is inconsistent with the pattern used in RunGenerate. For consistency, this should also use cli.NewExitError.
Apply this change for consistency:
- return errors.New("stacks experiment is not enabled use --experiment stacks to enable it")
+ return cli.NewExitError(errors.New("stacks experiment is not enabled use --experiment stacks to enable it"), cli.ExitCodeGeneralError)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !opts.Experiments.Evaluate(experiment.Stacks) { | |
| return errors.New("stacks experiment is not enabled use --experiment stacks to enable it") | |
| if !opts.Experiments.Evaluate(experiment.Stacks) { | |
| return cli.NewExitError(errors.New("stacks experiment is not enabled use --experiment stacks to enable it"), cli.ExitCodeGeneralError) |
| @@ -121,7 +122,7 @@ func (app *App) RunContext(ctx context.Context, arguments []string) (err error) | |||
| cmd := app.newRootCommand() | |||
|
|
|||
| args := Args(parentCtx.Args().Slice()) | |||
| ctx := NewContext(parentCtx.Context, app) | |||
| ctx := NewAppContext(parentCtx.Context, app, args) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding error handling for undefined flags.
The ErrorOnUndefinedFlag is set to true, which is good for catching misconfigurations early. However, we should ensure the error messages are user-friendly.
Let's check the error handling implementation:
Also applies to: 176-176
🏁 Script executed:
#!/bin/bash
# Search for undefined flag error handling
rg -A 5 'undefined.*flag|flag.*undefined'Length of output: 1287
Undefined Flag Error Handling: Improve User-Friendliness
We've verified that while the ErrorOnUndefinedFlag is correctly enabled in internal/cli/command.go (ensuring undefined flags trigger errors), the actual error message—returned in internal/cli/generic_flag.go as:
"flag type %T is undefined"
—is rather technical. This message exposes internal type details and isn't particularly user-friendly. You might consider updating it to something clearer, for example:
"Unknown flag encountered. Please check your command options."
This tweak would provide users with more actionable feedback when they misconfigure flags. Additionally, note that internal/cli/app.go appears to treat undefined flags as arguments rather than strictly erroring out, so double-checking consistency in behavior across commands might also be worthwhile.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
internal/cli/slice_flag.go (1)
138-143: Consider adding validation for required fieldsThe struct looks good, but we might want to validate that
valuesandvalueTypeare not nil in the constructor to prevent potential panics later.func newSliceValue[T comparable](valueType FlagVariable[T], valSep string, dest *[]T, setter FlagSetterFunc[T]) *sliceValue[T] { + if dest == nil { + dest = &[]T{} + } + if valueType == nil { + panic("valueType cannot be nil") + } return &sliceValue[T]{ values: dest, valueType: valueType,internal/cli/generic_flag.go (5)
30-31: Enhance environment variables documentationThe documentation could be clearer about the order of precedence between flag values and environment variables. Let's make it more explicit.
- // The names of the env variables that are parsed and assigned to `Destination` before the flag value. + // The names of environment variables that can set this flag's value. Multiple variables are checked in order, + // with the first non-empty value taking precedence. Flag values from command line override environment variables.
32-35: Clarify Action and Setter documentationThe documentation should explain the relationship between Action and Setter, and when each is called.
- // Action is a function that is called when the flag is specified. It is executed only after all command flags have been parsed. - // Setter allows to set a value to any type by calling its `func(bool) error` function. + // Action is called after all command flags are parsed, allowing for validation or side effects. + // Example: func(ctx *Context, value T) error + + // Setter provides custom value setting logic, called immediately when the flag value is set. + // Example: func(value T) error
44-58: Improve error handling in Apply methodThe error handling could be more robust by checking for nil values and providing context.
func (flag *GenericFlag[T]) Apply(set *libflag.FlagSet) error { + if set == nil { + return errors.New("flag set cannot be nil") + } + if flag.FlagValue != nil { return ApplyFlag(flag, set) } valueType := &genericVar[T]{dest: flag.Destination} value := newGenericValue(valueType, flag.Setter) flag.FlagValue = &flagValue{ value: value, initialTextValue: value.String(), } return ApplyFlag(flag, set) }
125-135: Add validation in Set methodConsider adding input validation before setting the value.
func (flag *genericValue[T]) Set(str string) error { + if str == "" { + return errors.New("value cannot be empty") + } + if err := flag.value.Set(str); err != nil { return err } if flag.setter != nil { return flag.setter(flag.Get().(T)) } return nil }
170-173: Improve error messages for better user experienceThe error messages could be more helpful by providing valid value ranges and examples.
- return errors.New(InvalidValueError{underlyingError: err, msg: `must be one of: "0", "1", "f", "t", "false", "true"`}) + return errors.New(InvalidValueError{underlyingError: err, msg: `must be a boolean value (e.g., "true", "false", "1", "0")`}) - return errors.New(InvalidValueError{underlyingError: err, msg: "must be 32-bit integer"}) + return errors.New(InvalidValueError{underlyingError: err, msg: "must be a 32-bit integer (range: -2147483648 to 2147483647)"}) - return errors.New(InvalidValueError{underlyingError: err, msg: "must be 32-bit unsigned integer"}) + return errors.New(InvalidValueError{underlyingError: err, msg: "must be a positive 32-bit integer (range: 0 to 4294967295)"}) - return errors.New(InvalidValueError{underlyingError: err, msg: "must be 64-bit integer"}) + return errors.New(InvalidValueError{underlyingError: err, msg: "must be a 64-bit integer (range: -9223372036854775808 to 9223372036854775807)"})Also applies to: 178-181, 186-189, 194-197
internal/cli/flag.go (4)
14-16: Consider clarifying the split delimiter usage.
Currently, FlagSplitter is set to strings.Split, but there's no explicit mention of how environment variables with certain delimiters (e.g., commas, semicolons) should be handled. Clarifying the expected delimiter pattern in the documentation or variable name might help future developers.
34-36: Tidy up the docstring.
There's a minor grammatical slip in "Executed after flag have been parsed and assigned...". Consider rephrasing to "Executed after the flag has been parsed and assigned..." for clarity.-// Executed after flag have been parsed and assigned to... +// Executed after the flag has been parsed and assigned to...
57-60: Documentation mismatch.
The doc for IsArgSet still says “IsSet returns true...” rather than “IsArgSet returns true…”. Update the docstring to align with the actual function name and behavior.-// IsSet returns true if the flag was by CLI arg. +// IsArgSet returns true if the flag was set by CLI arg.
247-249: Unused helper?
SplitValue(val) always returns a single-element slice and doesn’t appear to be used. If it’s for future expansions, consider adding a TODO comment or leaving short usage notes so folks know its purpose.internal/cli/map_flag.go (3)
31-62: Nice improvements to the MapFlag struct! 👍The generic type constraints and new fields make the flag handling more flexible and type-safe. The documentation is clear and helpful.
Consider adding an example in the struct documentation showing how to use the new
Setterfield, as it's a powerful new feature that users might overlook.
125-128: Enhance GetEnvVars documentationWhile the implementation is correct, let's make the documentation more helpful.
-// GetEnvVars implements `cli.Flag` interface. +// GetEnvVars returns the list of environment variable names that can be used to set this flag's value. +// These environment variables are checked in order before the flag value from command line.
185-208: Consider a small optimization in Set methodThe implementation looks great! Here's a tiny optimization to consider:
func (flag *mapValue[K, V]) Set(str string) error { parts := flag.splitter(str, flag.valSep) if len(parts) != flatPatsCount { return errors.New(NewInvalidKeyValueError(flag.valSep, str)) } + + // Trim spaces once and reuse + keyStr := strings.TrimSpace(parts[0]) + valStr := strings.TrimSpace(parts[1]) key := flag.keyType.Clone(new(K)) - if err := key.Set(strings.TrimSpace(parts[0])); err != nil { + if err := key.Set(keyStr); err != nil { return err } val := flag.valType.Clone(new(V)) - if err := val.Set(strings.TrimSpace(parts[1])); err != nil { + if err := val.Set(valStr); err != nil { return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/commands/run/command.go(1 hunks)cli/help_test.go(1 hunks)internal/cli/bool_flag.go(1 hunks)internal/cli/flag.go(1 hunks)internal/cli/generic_flag.go(5 hunks)internal/cli/map_flag.go(7 hunks)internal/cli/slice_flag.go(6 hunks)test/integration_serial_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/help_test.go
- cli/commands/run/command.go
- test/integration_serial_test.go
- internal/cli/bool_flag.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (10)
internal/cli/slice_flag.go (5)
34-40: Nice improvements to the flag structure! 👍Good changes here:
- Renamed to
EnvVarsfor clarity (plural makes more sense)- Well-documented new
Setterfield- More consistent action function type
80-82: Consider adding error handling for value initializationThe value initialization looks cleaner, but we might want to handle potential errors from
newSliceValue. What happens if the value type initialization fails?
102-105: Clean and simple implementation! 🎯Nice straightforward update to support multiple environment variables.
166-168: The nil check is redundant hereSince we're already checking
if flag.setter != nil, we don't need additional nil handling. The code is safe as is! 👍
1-195: Overall excellent code organization! 🌟The code is well-structured, follows Go conventions, and has clear documentation. The changes make the flag handling more robust while maintaining readability.
internal/cli/flag.go (3)
223-227: Verify boolean-only flags.
TakesValue() unconditionally returns true, but boolean flags typically don’t consume a value (like --verbose). Consider adding a condition to detect IsBoolFlag() to return the correct behavior.
242-245: Confirm if subcommand scoping is always intended.
AllowedSubcommandScope() consistently returns true, which might be fine, but double-check if you want certain flags restricted to top-level commands. This might help define more specific usage boundaries.
251-269: Empty environment variables are skipped—double-check reasons.
ApplyFlag() discards any empty environment variable values. If you ever want to allow empty values (like indicating a reset), consider adjusting your logic. If not, a short inline comment clarifying the rationale might help.internal/cli/map_flag.go (2)
74-113: LGTM! Clean initialization and environment handlingThe map initialization using
make()and the newLookupEnvFuncimplementation look great. The code is more robust and handles edge cases well.
160-183: Great work on the mapValue implementation! 🎯The generic type constraints and the new
Resetmethod make the code more robust and user-friendly. The type handling withFlagVariableis elegant.
| func (flag *MapFlag[K, V]) Apply(set *libflag.FlagSet) error { | ||
| if flag.FlagValue != nil { | ||
| return ApplyFlag(flag, set) | ||
| } | ||
|
|
||
| if flag.FlagValue != nil { | ||
| return ApplyFlag(flag, set) | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove duplicate code block
There's a duplicate check for flag.FlagValue that should be removed.
func (flag *MapFlag[K, V]) Apply(set *libflag.FlagSet) error {
if flag.FlagValue != nil {
return ApplyFlag(flag, set)
}
-
- if flag.FlagValue != nil {
- return ApplyFlag(flag, set)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (flag *MapFlag[K, V]) Apply(set *libflag.FlagSet) error { | |
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } | |
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } | |
| func (flag *MapFlag[K, V]) Apply(set *libflag.FlagSet) error { | |
| if flag.FlagValue != nil { | |
| return ApplyFlag(flag, set) | |
| } | |
| // (Other code, if any, would follow here) | |
| } |
Description
exec,runcommands implementation.Relates to #3445 .
Fixes #3757.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
runcommand consolidating functionalities from deprecated commands, enhancing usability.