feat: exec, run, info strict commands implementation, flags renaming.
#3723
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (25)
main.go (2)
24-28: Make the error message more helpfulWhile the error handling is solid, we could make it more user-friendly by including the valid log levels in the error message.
Here's a suggested improvement:
if err := global.NewLogLevelFlag(opts, nil).Apply(new(flag.FlagSet)); err != nil { - opts.Logger.Error(err.Error()) + opts.Logger.Errorf("Invalid log level in TG_LOG_LEVEL environment variable: %v. Valid levels are: TRACE, DEBUG, INFO, WARN, ERROR", err) os.Exit(1) }
69-73: Add documentation and improve formattingThe
setupContextfunction looks good but could use some love:
- Let's add documentation to explain its purpose
- Let's address the formatting suggestion from the linter
Here's how we can improve it:
+// setupContext creates and configures the application context with exit code tracking +// and logging capabilities. func setupContext(opts *options.TerragruntOptions, exitCode *tf.DetailedExitCode) context.Context { ctx := context.Background() ctx = tf.ContextWithDetailedExitCode(ctx, exitCode) + return log.ContextWithLogger(ctx, opts.Logger) }🧰 Tools
🪛 golangci-lint (1.62.2)
72-72: return statements should not be cuddled if block has more than two lines
(wsl)
internal/strict/view/plaintext/render.go (2)
1-11: Add a package-level comment for clarity.A brief comment explaining the purpose of this package (e.g., rendering controls in plaintext format) could help maintainers quickly understand its role.
44-64: Avoid panics in the initial template parse.Using
template.Mustwill panic ifParsefails. Consider handling this error more gracefully, especially if there's a possibility of dynamic or user-provided template strings.internal/cli/context.go (2)
7-15: Consider renaming the struct to avoid overshadowing the standard library's context.
This type is namedContextbut it embeds the standard library'scontext.Context. It's not necessarily wrong, but it might cause confusion in usage or code reading. Renaming could clarify usage and reduce potential mix-ups around method calls oncontext.Contextvs. the localContexttype.
40-45: Watch out for overshadowing WithValue.
By providingWithValue(key, val), calls might mask or confuse standard library usage in certain scenarios. This can be valid, but consider clarifying it through docstrings or a renaming strategy for maximum clarity.pkg/log/formatter.go (1)
9-37: Consider splitting the interface to reduce complexity.
TheFormatterinterface does a lot: toggling color, output, relative paths, custom format, etc. Breaking it into smaller interfaces can make testing and maintenance simpler in the long run.internal/strict/controls/control.go (1)
100-129: Double-check subcontrol evaluation ordering.When
ctrl.Enabledis false, we skip returningctrl.Errorbut eventually invokectrl.Subcontrols.Evaluate(ctx). This can lead to warnings across subcontrols if they're individually enabled. Ensure this order aligns with your intended flow, or consider adjusting the sequence to evaluate subcontrols first if you want consistent error/warning aggregation.internal/cli/flag.go (1)
108-114: Include specific environment variable in the error message.Right now, if the same env var is set multiple times, the error reads “setting the env var multiple times” without mentioning which env var. It might help to incorporate the variable name for faster troubleshooting.
- return errors.Errorf("setting the env var multiple times") + return errors.Errorf("setting the env var %s multiple times", flag.valueName)cli/flags/global/flags.go (1)
196-229: Check combined usage ofstrict-modeandstrict-controlflags.Both flags independently enable strict features. If users specify both, their combined effect might need clarifying. Consider logging a single notice summarizing the net effect of enabling both.
internal/cli/exit_code.go (1)
12-13: Consider adding validation for exit code rangeWhile using
bytetype implicitly limits values to 0-255, consider adding explicit validation when setting exit codes to catch potential overflows early.internal/strict/controls/deprecated_command.go (2)
9-12: Consider using an enum for command categoriesWhile string constants work, an enumerated type might provide better type safety and prevent typos in category names.
14-22: Great user-friendly error messages! 🎯The error and warning messages are clear and actionable. They follow a consistent format and provide helpful guidance to users.
However, consider adding a link to documentation about deprecated commands, similar to what's done in the experiments error messages.
internal/experiment/errors.go (1)
18-20: Consider more descriptive error messageThe current message could be more helpful. Consider including context about what caused the invalid experiment name.
- return "allowed experiment(s): " + strings.Join(err.allowedNames, ", ") + return "Invalid experiment name. Allowed experiment(s) are: " + strings.Join(err.allowedNames, ", ")internal/strict/status.go (2)
15-19: Consider using constants for status namesThe status names in the map could be defined as constants to ensure consistency across the codebase.
41-45: Consider moving ANSI colors to a separate packageThese color constants might be useful in other parts of the codebase. Consider moving them to a shared
internal/colorspackage.internal/strict/category.go (3)
9-10: Consider using a more descriptive type name.The comment mentions "DeprecatedFlag Category" but the type name is just "Categories". This could be confusing.
-// Categories is multiple of DeprecatedFlag Category. -type Categories []*Category +// Categories represents a collection of DeprecatedFlag categories. +type Categories []*Category
54-60: Improve the Less implementation for better readability.The current implementation is a bit hard to follow. Let's make it more explicit.
-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 -} +func (categories Categories) Less(i, j int) bool { + // Handle empty names: empty names should come last + if categories[i].Name == "" { + return false + } + if categories[j].Name == "" { + return true + } + // Normal lexicographical comparison + return categories[i].Name < categories[j].Name +}🧰 Tools
🪛 golangci-lint (1.62.2)
59-59: return statements should not be cuddled if block has more than two lines
(wsl)
74-80: Consider adding validation for the Category struct.The Category struct could benefit from validation to ensure the Name field isn't empty when creating new categories.
+// NewCategory creates a new Category with validation +func NewCategory(name string, hidden bool) (*Category, error) { + if name == "" { + return nil, fmt.Errorf("category name cannot be empty") + } + return &Category{ + Name: name, + Hidden: hidden, + }, nil +}cli/flags/prefix.go (1)
11-12: Add deprecation notice details.The comment marks TerragruntPrefix as deprecated but doesn't explain why or what to use instead.
- // TerragruntPrefix is an environment variable deprecated prefix. + // TerragruntPrefix is a deprecated environment variable prefix. + // Deprecated: Use TgPrefix instead for consistency with the new naming convention.pkg/log/format/formatter.go (2)
19-20: Consider using a more descriptive field name for output control.The field name
disabledOutputis a bit ambiguous. Consider a more specific name that better describes its purpose.- disabledOutput bool + suppressOutput bool
34-36: Add logging for disabled output state.When output is disabled, it might be helpful to log this state change at debug level for troubleshooting.
if formatter.placeholders == nil || formatter.disabledOutput { + if formatter.disabledOutput { + // Log at debug level that output is disabled + entry.Debug("Log output is currently disabled") + } return nil, nil }cli/commands/run/command.go (1)
94-113: Enhance command validation error messages.The error messages could be more helpful by suggesting similar commands when an invalid command is provided.
+func findSimilarCommands(cmd string, validCmds []string) []string { + var similar []string + for _, valid := range validCmds { + if strings.HasPrefix(valid, cmd) || + levenshtein.Distance(cmd, valid) <= 2 { + similar = append(similar, valid) + } + } + return similar +} func validateCommand(opts *options.TerragruntOptions) error { if opts.DisableCommandValidation || collections.ListContainsElement(tf.CommandNames, opts.TerraformCommand) { return nil } + similar := findSimilarCommands(opts.TerraformCommand, tf.CommandNames) var errMsg string + var suggestions string + if len(similar) > 0 { + suggestions = fmt.Sprintf("\n\nDid you mean one of these?\n %s", + strings.Join(similar, "\n ")) + }internal/cli/args.go (1)
84-94: Consider pre-allocating the result slice.While the
Removemethod works correctly, we could optimize it by pre-allocating the result slice with a capacity oflen(args)-1since we know we're removing exactly one element.- result := make([]string, 0, len(args)) + result := make([]string, 0, len(args)-1)🧰 Tools
🪛 golangci-lint (1.62.2)
87-87: ranges should only be cuddled with assignments used in the iteration
(wsl)
cli/app.go (1)
327-339: Consider adding a comment explaining the flag propagation.While the code is correct, it would be helpful to add a comment explaining why we need to propagate the
-no-colorflag to Terraform.// Since Terragrunt and Terraform have the same `-no-color` flag, // if a user specifies `-no-color` for Terragrunt, we should propagate it to Terraform as well. + // This ensures consistent color output behavior between Terragrunt and Terraform. if opts.Logger.Formatter().DisabledColors() { args = append(args, tf.FlagNameNoColor) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
cli/app.go(9 hunks)cli/commands/exec/action.go(1 hunks)cli/commands/exec/options.go(1 hunks)cli/commands/hclfmt/action.go(3 hunks)cli/commands/info/strict/command.go(1 hunks)cli/commands/run/command.go(1 hunks)cli/commands/run/file_copy_getter.go(2 hunks)cli/commands/run/target.go(4 hunks)cli/commands/shortcuts.go(1 hunks)cli/commands/stack/action.go(3 hunks)cli/flags/flag_opts.go(1 hunks)cli/flags/global/flags.go(1 hunks)cli/flags/prefix.go(1 hunks)config/config.go(6 hunks)internal/cache/context.go(1 hunks)internal/cli/args.go(4 hunks)internal/cli/context.go(1 hunks)internal/cli/exit_code.go(1 hunks)internal/cli/flag.go(1 hunks)internal/cli/funcs.go(1 hunks)internal/cli/map_flag.go(7 hunks)internal/experiment/errors.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/controls/control.go(1 hunks)internal/strict/controls/deprecated_command.go(1 hunks)internal/strict/controls/deprecated_env_var.go(1 hunks)internal/strict/status.go(1 hunks)internal/strict/view/plaintext/render.go(1 hunks)internal/strict/view/writer.go(1 hunks)main.go(3 hunks)pkg/log/format/formatter.go(5 hunks)pkg/log/formatter.go(1 hunks)test/fixtures/exec-cmd/script.sh(1 hunks)test/integration_windows_test.go(1 hunks)tf/detailed_exitcode.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- cli/commands/exec/options.go
- internal/cli/funcs.go
- cli/commands/run/target.go
- tf/detailed_exitcode.go
- internal/cache/context.go
- test/integration_windows_test.go
- cli/commands/hclfmt/action.go
- cli/commands/run/file_copy_getter.go
- cli/commands/exec/action.go
- internal/strict/view/writer.go
- cli/commands/shortcuts.go
- cli/commands/stack/action.go
- cli/flags/flag_opts.go
- cli/commands/info/strict/command.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
main.go
72-72: return statements should not be cuddled if block has more than two lines
(wsl)
internal/cli/args.go
87-87: ranges should only be cuddled with assignments used in the iteration
(wsl)
internal/strict/category.go
59-59: return statements should not be cuddled if block has more than two lines
(wsl)
🪛 Shellcheck (0.10.0)
test/fixtures/exec-cmd/script.sh
[warning] 9-9: TF_VAR_foo is referenced but not assigned.
(SC2154)
[warning] 15-15: TF_VAR_bar is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (34)
main.go (2)
4-15: Clean package organization! 👍The switch from
shell.DetailedExitCodetotf.DetailedExitCodeand the new imports align nicely with the CLI redesign goals.Also applies to: 20-20
48-50: Smart error handling! 🎯Great improvement on the error stack trace handling:
- Only logs stack traces when they exist
- Uses the appropriate TRACE level for detailed error information
internal/strict/view/plaintext/render.go (4)
19-19: Great use of interface compliance check.Verifying
Renderimplementsview.Renderat compile time is a neat way to catch mismatches early.
23-25: Constructor looks good.Straightforward constructor returning a pointer to
Render. Nicely done.
66-79: Formatting approach is sound.Using tabwriter for alignment is a simple, effective choice. No issues here.
81-89: Good encapsulation of template building & execution.It's clean to separate the template creation from its final execution—the structure boosts readability.
internal/cli/context.go (1)
17-23: Looks neat!
The constructor approach is straightforward and keeps everything readable. Thumbs up, no issues spotted here.pkg/log/formatter.go (1)
51-63: Well-structured error handling.
The early check for a nil log entry and returning an error prevents unexpected panics or downstream issues. This is a good defensive step.internal/strict/controls/deprecated_env_var.go (2)
44-51: Clear error and warning messages.
The approach of returning compelling guidance when environment variables are deprecated is carefully done. Users can better understand what changed and how to proceed.
57-95: Confirm behavior for multiple environment variables.
IfnewFlagcan have multiple environment variables, this code references just the first one. That might be fine, but it could be worth verifying that the remaining vars aren't needed or used differently.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Searching references to newFlag.GetEnvVars() to confirm usage patterns and ensure consistent handling across the codebase. rg -A 5 'newFlag\.GetEnvVars\(\)'Length of output: 1210
Verification of Multiple Environment Variables Handling
The search output shows that the code consistently accesses only the first environment variable returned by
newFlag.GetEnvVars(). For instance, it usesutil.FirstElement(util.RemoveEmptyElements(newFlag.GetEnvVars()))when assigningnewNameand similarly indexes the slice in the conditional block. There isn't any evidence in the codebase suggesting that the remaining environment variables are handled differently or need separate processing. This indicates that the current behavior, while only referencing the first element, is intentional and consistent.internal/strict/controls/control.go (1)
42-44: Consider concurrency safety for shared fields.
OnceWarnnicely prevents repeated warnings, but other fields (likeEnabledorError) could also be accessed from multiple goroutines. If you anticipate concurrent evaluations, consider wrapping updates to shared fields in thread-safe operations or clarifying single-thread usage in doc comments.internal/cli/flag.go (2)
32-33: Review usage of MapFlagSetterFunc.
MapFlagSetterFunc[K any, V any]is defined but isn't visibly used in this file. If it’s still needed, consider adding references or usage examples. Otherwise, removing it may keep the code lean and focused.
251-270: Validate multiple environment variables usage.The
ApplyFlagfunction loops over all environment variables for a flag. WhenMultipleSet()is true, setting the same env var multiple times could be unexpected. Confirm that’s the intended behavior, or warn users to unify multiple environment values into one.cli/flags/global/flags.go (2)
49-60: Deprecated flags mirror current flags.The deprecated flag names here match the new flags (e.g.,
DeprecatedLogLevelFlagName = "log-level"). This provides no real migration path and may confuse users. Consider renaming them to distinguish from active flags.
304-318: Add a safeguard against infinite loop in nested commands.As noted previously, this loop breaks only when
cmd == nil. If there’s a cyclic subcommand reference, it might never fail and keep iterating. Add a counter break to matchmaxIterations.internal/cli/exit_code.go (2)
4-10: Nice work on the exit code constants! 👍The exit codes follow standard Unix conventions, and the use of
iotafor automatic enumeration is a clean approach. The comments clearly explain each code's purpose.
15-20: Well-designed error interface extensionThe
ExitCoderinterface nicely extends the standard error interface while maintaining compatibility with Go's error wrapping conventions throughUnwrap().internal/experiment/errors.go (1)
38-40: Excellent user guidance in error message! 🌟Great job including both the error context and a link to documentation. This helps users understand and resolve the issue quickly.
internal/strict/status.go (1)
47-59: Nice color-coded status output! 🎨The colored output improves readability and user experience. Good job handling the default case too!
cli/commands/run/command.go (1)
27-28: Remove shortcut examples until fully implemented.Based on the past review comments, we should be careful about advertising shortcuts that aren't fully supported yet.
- "# 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",internal/cli/args.go (4)
9-12: LGTM! Good choice for the constant name and value.The
BuiltinCmdSepconstant with value"--"follows the standard Unix/Linux convention for separating command options from positional arguments.
29-38: LGTM! Clean and efficient implementation ofSplit.The function efficiently splits the slice at the first occurrence of the separator, returning both parts. The early return with
nilfor the second part when no separator is found is a nice touch.
40-44: LGTM! Smart use ofslices.Clonefor safe copying.The
WithoutBuiltinCmdSepmethod safely handles the command separator by usingslices.Cloneto avoid modifying the original slice.
142-157: LGTM! Well-structured command name extraction.The
CommandNameNmethod is a clean implementation that finds the nth non-flag argument. This refactoring improves code reuse as it's used by bothCommandNameandSubCommandName.internal/cli/map_flag.go (2)
85-93: LGTM! Clean environment variable lookup implementation.The environment variable lookup is properly encapsulated in a function that handles both the lookup and splitting of values.
95-99: LGTM! Good use of generics for type safety.The code properly uses generic type variables for key and value types, making the implementation type-safe while remaining flexible.
cli/app.go (2)
145-177: LGTM! Well-structured global flags handling.The
GlobalFlagsfunction provides a clean way to handle backward compatibility for moved flags while preventing duplicates.
179-201: LGTM! Smart handling of duplicate no-color flags.The
removeNoColorFlagDuplicatesfunction efficiently handles the edge case where both--no-colorand--terragrunt-no-colorare present, preventing the "setting the flag multiple times" error.config/config.go (2)
42-44: LGTM! Good addition of recommended config name constant.The
RecommendedParentConfigNameconstant with value"root.hcl"helps standardize the naming convention for parent config files.
792-794: LGTM! Improved error message for file info retrieval.The error message now clearly indicates that the failure occurred during file info retrieval, making debugging easier.
test/fixtures/exec-cmd/script.sh (4)
1-1: Shebang line is correctly set.The script properly uses
#!/usr/bin/env bash, which dynamically determines the path to the Bash interpreter. This follows the previous suggestion by yhakbar.
7-7: Echo statement outputs command arguments and working directory.The echo command clearly prints the first two arguments along with the present working directory (
$PWD), which aids in debugging and verification.
9-13: Verification of TF_VAR_foo value works as intended.The conditional check confirms that
TF_VAR_foois set to "FOO" and, if not, provides an informative error message including the current value (or "not set" if undefined) before exiting. Note that Shellcheck warnings (SC2154: TF_VAR_foo is referenced but not assigned) can be safely ignored here since this variable is expected to be set externally.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 9-9: TF_VAR_foo is referenced but not assigned.
(SC2154)
15-19: Verification of TF_VAR_bar value is clear and effective.Similar to the previous block, this segment checks that
TF_VAR_barequals "BAR". The error message and exit mechanism ensure users understand what variable is misconfigured. The static analysis warnings regarding unassigned variables are acceptable in this context.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 15-15: TF_VAR_bar is referenced but not assigned.
(SC2154)
| // EnvVar returns a string that is the concatenation of the slice values with the given `name`, | ||
| // using underscores as separators, replacing dashes with underscores, converting to uppercase. | ||
| func (prefix Prefix) EnvVar(name string) string { | ||
| if name == "" { | ||
| return "" | ||
| } | ||
|
|
||
| name = strings.Join(append(prefix, name), "_") | ||
|
|
||
| return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for environment variable names.
The EnvVar method should validate that the input name follows environment variable naming conventions.
func (prefix Prefix) EnvVar(name string) string {
if name == "" {
return ""
}
+
+ // Validate environment variable name
+ if strings.ContainsAny(name, "$.{}\\ ") {
+ return ""
+ }
name = strings.Join(append(prefix, name), "_")
return strings.ToUpper(strings.ReplaceAll(name, "-", "_"))
}📝 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.
| // EnvVar returns a string that is the concatenation of the slice values with the given `name`, | |
| // using underscores as separators, replacing dashes with underscores, converting to uppercase. | |
| func (prefix Prefix) EnvVar(name string) string { | |
| if name == "" { | |
| return "" | |
| } | |
| name = strings.Join(append(prefix, name), "_") | |
| return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) | |
| } | |
| // EnvVar returns a string that is the concatenation of the slice values with the given `name`, | |
| // using underscores as separators, replacing dashes with underscores, converting to uppercase. | |
| func (prefix Prefix) EnvVar(name string) string { | |
| if name == "" { | |
| return "" | |
| } | |
| // Validate environment variable name | |
| if strings.ContainsAny(name, "$.{}\\ ") { | |
| return "" | |
| } | |
| name = strings.Join(append(prefix, name), "_") | |
| return strings.ToUpper(strings.ReplaceAll(name, "-", "_")) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
cli/commands/stack/action.go (1)
63-75: Consider consistent error handling in switch casesWhile we've improved the experiment check error handling, the error handling in the switch cases could be more consistent.
Consider updating the error handling in the switch cases to match the new pattern:
- return errors.New(err) + return cli.NewExitError(err, cli.ExitCodeGeneralError)This would make the error handling consistent throughout the function.
internal/cli/args.go (1)
84-95: Consider using a more efficient filter pattern.While the current implementation works well, we could make it more idiomatic and potentially more efficient.
-func (args Args) Remove(name string) Args { - result := make([]string, 0, len(args)) - - for _, arg := range args { - if arg != name { - result = append(result, arg) - } - } - - return result -} +func (args Args) Remove(name string) Args { + return Args(slices.DeleteFunc(slices.Clone(args), func(s string) bool { + return s == name + })) +}main.go (2)
24-28: Consider enhancing the error message for log level setup.While the new flag-based approach is great, we could make the error message more helpful by including the valid log levels.
Here's a suggested improvement:
- opts.Logger.Error(err.Error()) + opts.Logger.Errorf("Failed to set log level: %v. Valid levels are: TRACE, DEBUG, INFO, WARN, ERROR", err)
69-74: Add documentation for context enhancements.The
setupContextfunction is well-structured, but let's make it even clearer with some documentation.Consider adding comments like this:
func setupContext(opts *options.TerragruntOptions, exitCode *tf.DetailedExitCode) context.Context { + // Start with a base context ctx := context.Background() + // Add support for detailed exit codes ctx = tf.ContextWithDetailedExitCode(ctx, exitCode) + // Add logger support for consistent logging across the application return log.ContextWithLogger(ctx, opts.Logger) }test/integration_strict_test.go (1)
31-31: Great improvements to error messaging! 🎯The changes look good! The error messages are clearer, and moving to a generic "deprecated-commands" control is more maintainable than having individual controls for each deprecated command.
Consider adding a comment explaining the "deprecated-commands" control to help future maintainers understand its purpose and scope. Something like:
name: "plan-all with plan-all strict control", + // Using deprecated-commands control to handle all deprecated command validations controls: []string{"deprecated-commands"},Also applies to: 36-37, 39-39
docs/_docs/04_reference/06-experiments.md (1)
140-158: Stabilization Criteria for CLI Redesign
The checklist under the "Criteria for stabilization" forcli-redesignis comprehensive and covers various new commands and flag updates. As development progresses, consider updating this list to mirror actual improvements. In the long term, grouping related items could enhance readability if the list expands further.cli/commands/stack/command.go (2)
10-23: Consider grouping related constantsThe constants could be more organized by grouping them into related categories:
const ( // CommandName stack command name. CommandName = "stack" + + // Flag names OutputFormatFlagName = "format" JSONFormatFlagName = "json" RawFormatFlagName = "raw" + // Subcommands generate = "generate" run = "run" output = "output" + // Output formats rawOutputFormat = "raw" jsonOutputFormat = "json" )
26-28: Document the prefix parameter's purposeThe new
prefixparameter could use some documentation to explain its role in flag handling.// NewFlags builds the flags for stack. +// prefix: Used to prepend environment variable names with a consistent prefix. +// The prefix helps avoid naming conflicts and maintains consistency across the CLI. func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cli/commands/run/command.go(1 hunks)cli/commands/stack/action.go(2 hunks)cli/commands/stack/command.go(3 hunks)docs/_docs/04_reference/06-experiments.md(3 hunks)internal/cli/args.go(4 hunks)internal/strict/category.go(1 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)internal/strict/view/plaintext/render.go(1 hunks)main.go(3 hunks)options/options.go(12 hunks)test/integration_strict_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/strict/controls/deprecated_flag_name.go
- cli/commands/run/command.go
- internal/strict/view/plaintext/render.go
- internal/strict/category.go
- options/options.go
🧰 Additional context used
🧠 Learnings (1)
cli/commands/stack/action.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (23)
cli/commands/stack/action.go (4)
8-8: LGTM! Clean import addition.The new CLI package import supports the improved error handling pattern.
24-25: Nice improvement in experiment checking and error handling! 👍The changes make two solid improvements:
- Using
Evaluate()instead of direct map access is more maintainable- Consistent error handling with
cli.NewExitErrorprovides better error reporting
33-34: Maintaining consistency with RunGenerate - good job!The same improvements in experiment checking and error handling are applied here, keeping the codebase consistent.
48-49: Experiment check updated consistently 👍The same improvements in experiment checking and error handling are applied here.
internal/cli/args.go (4)
9-12: LGTM! Clear and purposeful constants.The constants are well-named and their values are appropriate for their intended use.
29-38: Improve method documentation.The documentation should be more descriptive about what the method does and its return values.
-// Split splits `args` into two slices separated by `sep`. +// Split splits `args` into two slices at the first occurrence of `sep`. +// Returns (args before sep, args after sep). If sep is not found, +// returns (all args, nil).
40-44: LGTM! Safe handling of slice operations.Good use of
slices.Cloneto prevent modifying the original slice when appending.
143-170: LGTM! Great refactoring to reduce duplication.The introduction of
CommandNameNas a helper method is a nice improvement that:
- Reduces code duplication
- Makes the code more maintainable
- Keeps the logic clear and focused
main.go (2)
4-15: Nice package organization! 👍The move to
tf.DetailedExitCodeand the reorganization of imports aligns well with the PR's goal of better CLI structure.Also applies to: 20-20
47-50: Smart error handling improvement! 🎯Great job on making the error output cleaner by only showing the stack trace when it's not empty. This makes the output much more user-friendly.
test/integration_strict_test.go (4)
6-6: Nice package reorganization! 👍Moving from the
strictpackage to the centralizederrorspackage makes error handling more consistent across the codebase.
46-46: Smart move with the error handling! 🧠Using
ErrorContainsand a simpler error message makes the test more resilient to minor message changes. This should reduce the maintenance burden that was mentioned in the previous reviews.Also applies to: 68-68
95-95: Clear and helpful error messages! 📝The anti-pattern messaging is much more informative now, and the comment explaining why we can't test strict mode is super helpful - it directly addresses the confusion from previous reviews.
Also applies to: 101-101, 103-103
111-111: Document the CLI redesign experiment 📚The CLI flag simplification looks good, but the
--experiment cli-redesignflag needs some context.Could you add a comment explaining:
- What the CLI redesign experiment does?
- When it will become the default behavior?
- If there are any known limitations?
This will help other developers understand the transition period and what to expect.
Also applies to: 120-120
docs/_docs/04_reference/06-experiments.md (5)
68-71: Consistent Addition of New Experimental Feature
The new bullet point for[cli-redesign](#cli-redesign)fits nicely with the existing list. Just verify that the anchor link directs readers correctly to the section below.
72-92: Symlinks Experiment Section Formatting
The updated headers for thesymlinkssection—including "What it does", "How to provide feedback", and "Criteria for stabilization"—are well structured and clear. This formatting will help users quickly locate the details they seek.🧰 Tools
🪛 LanguageTool
[grammar] ~90-~90: The operating system from Apple is written “macOS”.
Context: ...cannot be supported). - [ ] Ensure that MacOS integration tests still work. See [#361...(MAC_OS)
[grammar] ~91-~91: The operating system from Apple is written “macOS”.
Context: ...616). - [ ] Add integration tests for MacOS in CI. ###stacksSupport for Terra...(MAC_OS)
93-118: Updated Stacks Section
The revisions to thestacksexperiment section, especially the headers for its subsections, are consistent with the other experiments. Please double-check that the checkboxes accurately represent the current progress and intended stabilization criteria.
119-131: Introduction of CLI Redesign Section
The newcli-redesignsection is a great addition that outlines important new functionalities like therunandexeccommands. Ensure that the description and provided details align with the actual implementation and related RFC (issue #3445).
132-139: Feedback Guidelines for CLI Redesign
The "How to provide feedback" section forcli-redesignis clear and directs users to the appropriate discussion forum. It might be beneficial to briefly mention how user feedback will be used to iterate on these features in future releases.cli/commands/stack/command.go (4)
4-8: Clean import organization! 👍The imports are well-organized and the new
flagspackage is properly integrated.
90-90: Nice simplification! 👍The direct use of
cli.ShowCommandHelpis cleaner than the previous implementation.
29-52: Verify environment variable generationThe environment variable generation looks good, but let's verify the naming convention is consistent across commands.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other NewFlags implementations to verify prefix usage consistency ast-grep --pattern 'func NewFlags(opts *options.TerragruntOptions, prefix flags.Prefix) cli.Flags { $$$ tgPrefix := prefix.Prepend($$$) $$$ }'Length of output: 62086
Environment Variable Naming Consistency Confirmed
The consistency check shows that all commands (including the stack command) use the same logic for generating environment variables. Each implementation creates a
tgPrefixby callingprefix.Prepend(flags.TgPrefix)and then usestgPrefix.EnvVars(FlagName)to set the environment variables. No discrepancies were found across the commands.
58-61: Verify the impact of ErrorOnUndefinedFlagThe change from
DisallowUndefinedFlagstoErrorOnUndefinedFlaglooks good, but let's verify the error messaging is user-friendly.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other commands using ErrorOnUndefinedFlag to verify consistency rg "ErrorOnUndefinedFlag:\s*true" -A 2 -B 2Length of output: 4800
Error messaging for undefined flags is consistent and user-friendly.
The use of
ErrorOnUndefinedFlag: trueappears uniformly across multiple commands (as seen in files likecli/help_test.go,cli/commands/catalog/command.go, etc.), which indicates that when an undefined flag is provided, users will receive a consistent and clear error message. Given that this flag leverages the underlying CLI library's error handling, it should deliver a user-friendly notification. No anomalies or user confusion were evident from our repository search.
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
New Features
Bug Fixes
Documentation
Tests