fix: Correct displaying deprecated flag warnings #3880
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor the Terragrunt CLI by restructuring command and flag handling. Command initialization now uses a consolidated Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (6)
cli/flags/flags_test.go (1)
18-59: Great coverage of negative boolean flags.
The table-driven test is nice and thorough! It ensures the standard boolean vs. negative boolean scenarios are all accounted for. However, consider adding one more test scenario to confirm behavior when flags start with a certain default and get toggled by user input. It can help flush out edge cases.cli/flags/flag.go (1)
37-48: Clarify flipping logic in the doc comment.
The new negative-flag logic is a nifty solution, but it’ll be clearer if you explicitly describe that “negative” flags invert the original boolean value before deciding whether the flag takes a value. It might prevent future confusion for contributors. Functionally, this looks correct and matches the tests.cli/commands/commands.go (1)
49-49: Document magic numbers used for category ordering.The magic numbers (10, 20, 30, 40) used for category ordering are clear but would benefit from a comment explaining the increment pattern.
- Order: 10, //nolint: mnd + Order: 10, // Categories are ordered in increments of 10 for easy insertion of new categoriesAlso applies to: 59-59, 78-78, 85-85
internal/cli/bool_flag.go (1)
34-35: Add usage example for negative flag.While the comment explains what the field does, adding a usage example would make it clearer.
// If set to true, then the assigned flag value will be inverted + // Example: With Negative=true, --flag=true becomes false, and --flag=false becomes true Negative boolinternal/cli/flag.go (1)
68-69: Document interface method with example.The new IsNegativeBoolFlag method would benefit from a more detailed comment with an example.
- // IsNegativeBoolFlag returns true if the boolean has an inverted value. + // IsNegativeBoolFlag returns true if the boolean flag's value should be inverted. + // Example: For a flag with Negative=true, when set to true it returns false, and vice versa. IsNegativeBoolFlag() boolcli/flags/global/flags.go (1)
80-112: Great implementation of flag management with room for improvement.The function effectively collects flags from various commands while preventing duplicates. However, consider extracting the command list initialization to a separate function for better maintainability.
Here's a suggested refactor:
func NewFlagsWithDeprecatedMovedFlags(opts *options.TerragruntOptions) cli.Flags { globalFlags := NewFlags(opts, nil) - - commands := cli.Commands{ - runCmd.NewCommand(opts), // run - runall.NewCommand(opts), // runAction-all - terragruntinfo.NewCommand(opts), // terragrunt-info - validateinputs.NewCommand(opts), // validate-inputs - graphdependencies.NewCommand(opts), // graph-dependencies - renderjson.NewCommand(opts), // render-json - awsproviderpatch.NewCommand(opts), // aws-provider-patch - outputmodulegroups.NewCommand(opts), // output-module-groups - graph.NewCommand(opts), // graph - } + commands := initializeCommands(opts) var seen []string for _, cmd := range commands { for _, flag := range cmd.Flags { flagName := util.FirstElement(util.RemoveEmptyElements(flag.Names())) if slices.Contains(seen, flagName) { continue } seen = append(seen, flagName) globalFlags = append(globalFlags, flags.NewMovedFlag(flag, cmd.Name, flags.StrictControlsByMovedGlobalFlags(opts.StrictControls, cmd.Name))) } } return globalFlags } + +func initializeCommands(opts *options.TerragruntOptions) cli.Commands { + return cli.Commands{ + runCmd.NewCommand(opts), // run + runall.NewCommand(opts), // runAction-all + terragruntinfo.NewCommand(opts), // terragrunt-info + validateinputs.NewCommand(opts), // validate-inputs + graphdependencies.NewCommand(opts), // graph-dependencies + renderjson.NewCommand(opts), // render-json + awsproviderpatch.NewCommand(opts), // aws-provider-patch + outputmodulegroups.NewCommand(opts), // output-module-groups + graph.NewCommand(opts), // graph + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/app.go(1 hunks)cli/app_test.go(1 hunks)cli/commands/commands.go(1 hunks)cli/flags/flag.go(1 hunks)cli/flags/flags_test.go(1 hunks)cli/flags/global/flags.go(3 hunks)internal/cli/bool_flag.go(1 hunks)internal/cli/flag.go(3 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)
🔇 Additional comments (7)
cli/app.go (1)
59-60: Double-check deprecated flags coverage.
Pretty neat approach withNewFlagsWithDeprecatedMovedFlags(opts). Just be sure there's enough unit or integration testing ensuring these moved flags truly behave seamlessly for users upgrading from older Terragrunt versions. Likewise,commands.New(opts).WrapAction(WrapWithTelemetry(opts))looks solid, but keep an eye on whether telemetry is triggered even when deprecated flags are used.Would you like me to propose an additional test snippet?
cli/flags/flags_test.go (1)
14-16: Helper function clarity is spot-on.
mockDestValueis a clean way to create destinations for tests, especially when working with generics. It's straightforward, easy to read, and does the job well.internal/strict/controls/deprecated_flag_name.go (1)
43-44:❓ Verification inconclusive
LGTM! Message format improvements look good.
The updated error and warning messages are now more flexible, correctly handling flags that might not use the
--prefix. This is a good improvement for better user experience.Let's ensure all test cases are updated to match the new format:
🏁 Script executed:
#!/bin/bash # Search for test cases that might still expect the old format with double hyphens rg -A 2 "Use `--%s` instead" "**/*_test.go"Length of output: 195
LGTM! The updated message formats clearly improve clarity and flexibility.
The new messages handle cases (such as flags lacking the
--prefix) more intuitively. However, the automated search for test cases reflecting the old format didn’t produce conclusive results. Please manually verify that all tests validating flag deprecation messages have been updated accordingly.cli/commands/commands.go (1)
31-36: Great job organizing commands into clear categories!The command categorization improves help output readability and maintainability.
internal/cli/flag.go (1)
232-233: Improved clarity in TakesValue documentation.The updated comment is more precise about the flag's behavior.
cli/flags/global/flags.go (1)
9-17: LGTM! Well-organized imports for command modules.The new imports are logically grouped and follow a clear pattern for command organization.
cli/app_test.go (1)
506-506: LGTM! Consistent with command handling refactor.The change from
cli.TerragruntCommands(opts)tocommands.New(opts)aligns with the new modular command structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cli/commands/commands.go (1)
31-36: Nice categorization of commands! Consider adding category descriptions.The command categories are well-organized, but adding brief descriptions for each category in the constants' documentation would help users and developers understand their purpose better.
const ( + // MainCommandsCategoryName represents primary Terragrunt operations like run, run-all MainCommandsCategoryName = "Main commands" + // CatalogCommandsCategoryName represents commands for managing Terragrunt catalogs CatalogCommandsCategoryName = "Catalog commands" + // ConfigurationCommandsCategoryName represents commands for managing Terragrunt configurations ConfigurationCommandsCategoryName = "Configuration commands" + // ShortcutsCommandsCategoryName represents OpenTofu-specific shortcut commands ShortcutsCommandsCategoryName = "OpenTofu shortcuts" )cli/flags/global/flags.go (1)
95-115: Consider adding error handling for duplicate flag names across commands.While the code prevents duplicate flags using
slices.Contains, it silently skips them. Consider logging a warning when duplicates are found to help with debugging.if slices.Contains(seen, flagName) { + opts.Logger.Debugf("Skipping duplicate flag %q from command %q", flagName, cmd.Name) continue }internal/cli/bool_flag.go (1)
135-145: Consider adding a nil check for safety.The implementation looks good, but we should add a safety check before dereferencing
val.genericVar.destto prevent potential panics.Here's a safer version:
func (val *boolVar) Set(str string) error { if err := val.genericVar.Set(str); err != nil { return err } + if val.genericVar.dest == nil { + return fmt.Errorf("destination pointer is nil") + } + if val.negative { *val.genericVar.dest = !*val.genericVar.dest } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/commands/commands.go(1 hunks)cli/flags/global/flags.go(3 hunks)internal/cli/bool_flag.go(2 hunks)internal/cli/flag.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (6)
cli/commands/commands.go (1)
40-97: Well-structured command organization with room for growth!The command organization using increments of 10 for category ordering is a smart choice, making it easy to insert new categories in the future. The code is clean and maintainable.
internal/cli/flag.go (2)
68-70: Good documentation for negative boolean flags.The comment clearly explains the behavior of negative boolean flags with a practical example.
233-237:❓ Verification inconclusive
Verify TakesValue behavior with negative boolean flags.
The
TakesValuemethod always returns true, which might not be correct for negative boolean flags. Consider testing this with various flag configurations.
🏁 Script executed:
#!/bin/bash # Search for tests related to negative boolean flags rg -A 5 "negative.*bool.*flag" --type goLength of output: 42
Verify Negative Boolean Flag Behavior in
TakesValue
Our automated search didn't find any tests specifically covering negative boolean flags. SinceTakesValue()always returns true, please manually confirm whether this behavior is appropriate for negative boolean flags or if adjustments (and additional tests) are needed.cli/flags/global/flags.go (1)
80-92: Well-organized command initialization with clear comments.The command initialization is clean and each command's purpose is clearly indicated in the comments.
internal/cli/bool_flag.go (2)
34-36: Nice documentation! 👍The comment for the
Negativefield clearly explains its purpose and includes a helpful example that makes it easy to understand how the flag inversion works.
47-54:✅ Verification successful
Let's verify the negative flag propagation.
The changes look good, but let's make sure all callers are aware of this new behavior.
Let's check how this negative flag is being used across the codebase:
🏁 Script executed:
#!/bin/bash # Search for BoolFlag struct initialization with Negative field rg "BoolFlag\s*{[^}]*Negative:\s*true" -A 5Length of output: 173
I'll generate a new verification script using a corrected regex pattern (escaping the curly brace) to ensure we accurately search for instances where the
Negativefield is set to true in aBoolFlaginitialization. This will help us verify whether the negative flag propagation is effectively communicated across the codebase.Once this script is executed, please share the output so we can determine if any further action is required.
🏁 Script executed:
#!/bin/bash # Corrected regex search: escaping the curly brace to find BoolFlag initializations with Negative set to true rg "BoolFlag\s*\{[^}]*Negative:\s*true" -A 5Length of output: 899
Negative flag propagation verified.
I reviewed the updated implementation and confirmed that the negative flag is correctly passed along as seen in the tests from bothcli/flags/flags_test.goandinternal/cli/bool_flag_test.go. Since there’s clear evidence from the test cases that all callers are aware of and utilize this new behavior, the changes are solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/commands/commands.go (2)
1-28: Add a bit more context to the package comment.The package comment is accurate but could be more helpful. Consider expanding it to briefly mention the command categories and their purpose.
-// Package commands represents CLI commands. +// Package commands represents CLI commands organized into categories like main operations, +// catalog management, configuration handling, and OpenTofu shortcuts.
54-54: Consider defining constants for category order numbers.Instead of using magic numbers with
//nolint: mndcomments, consider defining these as package constants. This would make the ordering scheme more maintainable and self-documenting.const ( MainCommandsCategoryName = "Main commands" CatalogCommandsCategoryName = "Catalog commands" ConfigurationCommandsCategoryName = "Configuration commands" ShortcutsCommandsCategoryName = "OpenTofu shortcuts" + // Category order constants - using increments of 10 for easy insertion + MainCommandsOrder = 10 + CatalogCommandsOrder = 20 + ConfigurationCommandsOrder = 30 + ShortcutsCommandsOrder = 40 )Also applies to: 64-64, 83-83, 90-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/commands/commands.go(1 hunks)
⏰ 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 (2)
cli/commands/commands.go (2)
30-40: Nice work on the command categories!The constants are well-named and documented, making it clear what each category represents.
42-101: Great job on the command organization!The
Newfunction is well-structured with:
- Clear separation of command groups
- Logical categorization
- Flexible ordering system
- Clean command merging
The modular approach will make it easier to maintain and extend the CLI in the future.
|
I switched to draft because I remembered I still needed to make a few small improvements. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
cli/commands/version/command.go (1)
24-26: Consider removing unused parameter.The
TerragruntOptionsparameter is marked as unused with_. Since it's not needed, we could simplify the function signature.-func Action(ctx *cli.Context, _ *options.TerragruntOptions) error { +func Action(ctx *cli.Context) error {cli/commands/help/command.go (2)
45-45: Consider moving magic number to a package-level constant.The value
1000would be more maintainable as a named constant with a descriptive name.+const maxCommandDepth = 1000 // Maximum depth of nested subcommands -const maxIterations = 1000
47-58: Simplify command lookup logic.The current implementation could be more straightforward using a while loop with an explicit condition.
-for range maxIterations { +for i := 0; i < maxCommandDepth && args.Len() > 0; i++ { cmdName := args.CommandName() cmd := cmds.Get(cmdName) if cmd == nil { break } args = args.Remove(cmdName) cmds = cmd.Subcommands ctx = ctx.NewCommandContext(cmd, args) }cli/flags/flag.go (1)
37-50: Add examples for boolean flag behavior.The boolean flag handling logic, especially for negative flags, could benefit from documentation examples.
Add examples to the comment:
// TakesValue implements `cli.Flag` interface. -// It returns `true` for all flags except boolean ones that are `false` or `true` inverted. +// It returns `true` for all flags except boolean ones that are `false` or `true` inverted. +// Examples: +// --flag -> takes value if not boolean or if boolean is true +// --no-flag -> takes value if boolean is false (negative flag)cli/commands/commands.go (1)
31-40: Consider documenting the order increment strategy 📝The command categories are well-organized with order increments of 10 (e.g., 10, 20, 30, 40). While this is a smart approach for future insertions, it would be helpful to add a comment explaining this design choice.
Add a comment like this above the first category:
// Command category names. const ( + // Categories use order increments of 10 to allow easy insertion of new categories between existing ones // MainCommandsCategoryName represents primary Terragrunt operations like run, run-all. MainCommandsCategoryName = "Main commands"Also applies to: 52-55
cli/flags/flag_test.go (1)
63-65: Make test cases more descriptive 🔍While using numeric indices works, giving each test case a descriptive name would make failures easier to understand.
Instead of using numeric indices, consider naming your test cases:
- t.Run(fmt.Sprintf("testCase-%d", i), func(t *testing.T) { + t.Run(fmt.Sprintf("%s-%s", testCase.flag.Names()[0], + testCase.expected ? "takes_value" : "no_value"), func(t *testing.T) {cli/flags/deprecated_flag.go (1)
128-134: Fix typo in parameter name 🔤There's a small typo in the parameter name
strcitControls(should bestrictControls). Otherwise, the nil check is a great addition for robustness!-func registerStrictControls(strcitControls strict.Controls, +func registerStrictControls(strictControls strict.Controls,cli/flags/flag_opts.go (2)
41-48: Potential memory leak in flag name handling.The code creates a new slice for flag names but doesn't clean up the old one. While Go has garbage collection, it's better to be explicit about resource management.
Consider using a single allocation:
- flag := &DeprecatedFlag{ - Flag: deprecatedFlag, - names: slices.Clone(deprecatedNames), - } + flag := &DeprecatedFlag{ + Flag: deprecatedFlag, + names: make([]string, len(deprecatedNames)), + } + copy(flag.names, deprecatedNames)
54-60: Improve error handling for nil controls.The current implementation silently returns when
regControlsFnis nil. While this works, it might hide potential issues.Consider adding a debug log to make the behavior more transparent:
if regControlsFn == nil { + opts.Logger.Debug("No control registration function provided, skipping controls setup") return }cli/flags/global/flags.go (1)
73-106: Well-structured solution for handling moved flags! 👍The new function elegantly solves the duplicate warnings issue. The comments explaining the problem and solution are particularly helpful.
However, let's make the code more robust:
Consider pre-allocating the
seenslice to reduce allocations:- var seen []string + seen := make([]string, 0, len(commands)*2) // Rough estimate of capacity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cli/app.go(2 hunks)cli/commands/commands.go(1 hunks)cli/commands/help/command.go(2 hunks)cli/commands/run/flags.go(0 hunks)cli/commands/version/command.go(1 hunks)cli/flags/deprecated_flag.go(3 hunks)cli/flags/flag.go(3 hunks)cli/flags/flag_opts.go(5 hunks)cli/flags/flag_test.go(1 hunks)cli/flags/global/flags.go(4 hunks)internal/cli/help.go(1 hunks)internal/experiment/experiment.go(1 hunks)internal/strict/category.go(1 hunks)internal/strict/control.go(1 hunks)internal/strict/controls/deprecated_env_var.go(2 hunks)internal/strict/controls/deprecated_flag_name.go(3 hunks)internal/strict/status.go(1 hunks)main.go(1 hunks)tf/getproviders/lock.go(1 hunks)
💤 Files with no reviewable changes (1)
- cli/commands/run/flags.go
✅ Files skipped from review due to trivial changes (5)
- internal/strict/status.go
- internal/strict/control.go
- internal/strict/category.go
- internal/experiment/experiment.go
- tf/getproviders/lock.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (13)
internal/cli/help.go (1)
6-6:✅ Verification successful
Nice upgrade to standard library slices! 👍
Good move switching from the experimental package to the standard library version. Just make sure your Go version is ≥ 1.21 where this became standard.
Let's check the Go version requirement in go.mod:
🏁 Script executed:
#!/bin/bash grep -r "^go " go.modLength of output: 31
Great upgrade to standard library slices! 👍
Good move replacing the experimental package with the standard library version. I confirmed that your
go.modspecifies Go 1.23, which is safely above the 1.21 requirement. Everything checks out and the change is approved!internal/strict/controls/deprecated_env_var.go (2)
6-6: Consistent import update 👍Matches the move to standard library slices in help.go. Good consistency!
65-65:❓ Verification inconclusive
Fixed the environment variable check logic
The condition now correctly checks if the value is in the deprecated flag's environment variables, which makes more sense for deprecation handling. This should fix any false positives in the deprecation warnings.
Let's verify this change doesn't break existing tests:
🏁 Script executed:
#!/bin/bash # Look for related tests rg -l "TestDeprecatedEnvVar|test.*deprecated.*env" --type goLength of output: 62
Environment Variable Check Logic Update – Please Verify Test Coverage
The update in
internal/strict/controls/deprecated_env_var.gonow properly checks if the environment variable (valueName) exists within the deprecated flag’s environment variables. This should mitigate false positives in deprecation warnings. However, our initial search for tests (usingrg -l "TestDeprecatedEnvVar|test.*deprecated.*env") returned no results, so it’s unclear if there’s automated test coverage for this behavior.
- Location:
internal/strict/controls/deprecated_env_var.go, line 65- Updated snippet:
if valueName == "" || !ctrl.deprecatedFlag.Value().IsEnvSet() || !slices.Contains(ctrl.deprecatedFlag.GetEnvVars(), valueName) {- Action needed: Please perform a manual verification of the test suite to confirm that this change does not break any existing tests. If tests for deprecated environment variable functionality are missing, consider adding them to ensure ongoing reliability.
main.go (1)
24-27: LGTM! Clean environment variable parsing.The change to use
ParseEnvVarsfor log level configuration is cleaner and follows the principle of configuration through environment variables.cli/flags/flag.go (1)
114-119: LGTM! Clean environment variable parsing implementation.The new
ParseEnvVarsmethod provides a clean way to handle environment variable configuration.internal/strict/controls/deprecated_flag_name.go (2)
44-45: Nice cleanup of warning messages! 👍The message format has been simplified by removing the redundant
--prefix from the second placeholder, making the output cleaner and more consistent.
83-83: Good use of standard library! 🎯Switching to
slices.Containsmakes the code more idiomatic and potentially more efficient. It's a great example of leveraging Go's standard library features.cli/flags/deprecated_flag.go (1)
76-78: Smart enhancement to control registration! 💡The addition of the boolean check before assigning controls is a great defensive programming practice. It ensures controls are only set when registration is successful.
cli/flags/flag_opts.go (1)
6-7: Nice upgrade to standard library! 🎉Good move switching to the standard library's
slicespackage. This reduces external dependencies and future-proofs the code.cli/flags/global/flags.go (1)
312-313:❓ Verification inconclusive
Verify path handling for cross-platform compatibility.
The path handling logic might need additional testing on Windows systems.
🏁 Script executed:
#!/bin/bash # Check for Windows-specific path handling rg -g '*.go' -l 'filepath\.(Join|ToSlash|FromSlash)' | while read -r file; do echo "=== $file ===" rg 'filepath\.(Join|ToSlash|FromSlash)' "$file" doneLength of output: 40797
Action: Double-check Windows Path Handling
I've reviewed our usage of path functions across the codebase. The standard library calls like
filepath.Join,filepath.ToSlash, andfilepath.FromSlashare used consistently—which is a good sign for cross-platform compatibility. In particular, the call incli/flags/global/flags.go(lines 312-313) that delegates tohelp.Action(ctx, opts)appears to be in line with our overall approach. We also have Windows-specific tests (e.g., intest/integration_windows_test.go) ensuring that our path metrics behave properly.Please make sure that any additional internal path manipulations—especially those within helper methods like
help.Action—are thoroughly tested on Windows as well.cli/app.go (3)
60-61: Clean command initialization! 🎯Great job simplifying the command initialization. The code is now more maintainable and easier to understand.
73-85: Excellent signal handling implementation!The graceful shutdown implementation is well-thought-out. The carriage return before printing the shutdown message is a nice touch for better UX.
165-166:❓ Verification inconclusive
Address the TODO comment about context linting.
There's a TODO comment about a context-related linter warning that should be addressed.
Let's check what the linter is complaining about:
🏁 Script executed:
#!/bin/bash # Run golangci-lint to check the specific context-related issue golangci-lint run --no-config --disable-all --enable=contextcheck ./...Length of output: 155
Hey there,
The lint check for the context warning timed out before giving a clear verdict. This means we’re not yet 100% sure if the context-related issue is real or if the
//nolint:contextchecksuppression is fine as-is.Next Steps:
- Re-run Linter: Please run the above script to see if any context lint warnings actually appear.
- Evaluate Warning: Based on the output, decide whether you need to fix any genuine context issues or if the suppression is acceptable.
- Update TODO: If there’s nothing to fix, you can remove the TODO comment; otherwise, address the warning as needed.
Once you’ve gathered the extended lint output, update this review accordingly.
🏁 Script executed:
#!/bin/bash # Re-run golangci-lint with an increased timeout to properly verify the context lint warning. golangci-lint run --no-config --disable-all --enable=contextcheck --timeout=5m ./...Length of output: 86
Context Linting Check – Please Verify Manually
After re-running golangci-lint with an extended timeout, no context lint warnings were reported. This suggests that the
//nolint:contextcheckdirective is suppressing a warning that isn’t currently emerging. Given the previous timeout and the ambiguity of the linter output, please manually verify in your environment that no context issues arise. If everything checks out, you can safely remove the TODO comment. Otherwise, update the comment to clearly explain why the suppression remains necessary.
- Action: Manually re-run the linter in your environment to confirm no context warnings.
- Next Step: If no warnings occur, clear the TODO comment to keep the code clean.
| if deprecatedFlagValue := deprecatedFlag.Value(); deprecatedFlagValue.IsSet() && deprecatedFlag.newValueFn != nil { | ||
| newValue := deprecatedFlag.newValueFn(deprecatedFlagValue) | ||
| new.Flag.Value().Set(newValue) //nolint:errcheck | ||
| newFlag.Flag.Value().Getter(deprecatedFlagValue.GetName()).Set(newValue) //nolint:errcheck |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential error in Value() method.
The error from Set() is being suppressed with //nolint:errcheck. Consider handling this error explicitly.
- newFlag.Flag.Value().Getter(deprecatedFlagValue.GetName()).Set(newValue) //nolint:errcheck
+ if err := newFlag.Flag.Value().Getter(deprecatedFlagValue.GetName()).Set(newValue); err != nil {
+ return cli.NewFlagValue(deprecatedFlagValue.GetName(), 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.
| newFlag.Flag.Value().Getter(deprecatedFlagValue.GetName()).Set(newValue) //nolint:errcheck | |
| if err := newFlag.Flag.Value().Getter(deprecatedFlagValue.GetName()).Set(newValue); err != nil { | |
| return cli.NewFlagValue(deprecatedFlagValue.GetName(), err) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cli/commands/help/command.go (2)
34-39: Consider adding a constant for the debug environment variable.The environment variable name
CLI_TEMPLATE_ERROR_DEBUGappears to be a magic string. Let's make it more maintainable.const ( CommandName = "help" + // DebugEnvVar is used by urfave/cli for template error debugging + DebugEnvVar = "CLI_TEMPLATE_ERROR_DEBUG" ) func Action(ctx *cli.Context, opts *options.TerragruntOptions) error { if opts.Logger.Level() >= log.DebugLevel { - if err := os.Setenv("CLI_TEMPLATE_ERROR_DEBUG", "1"); err != nil { + if err := os.Setenv(DebugEnvVar, "1"); err != nil {
45-58: Consider adding early return for performance.The loop could be simplified with an early return when a command is found, avoiding unnecessary iterations.
for i := 0; i < maxCommandDepth && args.Len() > 0; i++ { cmdName := args.CommandName() cmd := cmds.Get(cmdName) if cmd == nil { break } + if len(cmd.Subcommands) == 0 { + ctx = ctx.NewCommandContext(cmd, args) + return cli.NewExitError(cli.ShowCommandHelp(ctx), cli.ExitCodeGeneralError) + } args = args.Remove(cmdName) cmds = cmd.Subcommands ctx = ctx.NewCommandContext(cmd, args) }cli/flags/global/flags.go (2)
73-106: Great job handling duplicate flag warnings!The function effectively prevents duplicate warnings by tracking seen flags. However, consider using a map for better lookup performance.
- var seen []string + seen := make(map[string]struct{}) for _, cmd := range commands { for _, flag := range cmd.Flags { flagName := util.FirstElement(util.RemoveEmptyElements(flag.Names())) - if slices.Contains(seen, flagName) { + if _, exists := seen[flagName]; exists { continue } - seen = append(seen, flagName) + seen[flagName] = struct{}{}
276-303: Nice backward compatibility handling for log levels!The function maintains compatibility with pre-v0.67.0 behavior while providing clear logging levels. However, consider extracting the removed levels to constants.
+const ( + PanicLevel = "panic" + FatalLevel = "fatal" +) func NewLogLevelFlag(opts *options.TerragruntOptions, prefix flags.Prefix) *flags.Flag { // ... removedLevels := []string{ - "panic", - "fatal", + PanicLevel, + FatalLevel, }cli/flags/deprecated_flag.go (1)
128-134: Consider simplifying the control flowThe nil check is great for safety! However, we could make the code even cleaner by returning early for the success case too.
Here's a suggestion that maintains the same logic but with clearer flow:
func registerStrictControls(strictControls strict.Controls, flagNameControl, envVarControl strict.Control, flagNamesCategory, envVarsCategory string, controlNames ...string) bool { if strictControls == nil { return false } + if flagNameControl == nil && envVarControl == nil { + return true + } if flagNameControl != nil { strictControls.FilterByNames(append( controlNames, controls.TerragruntPrefixFlags, controls.DeprecatedFlags, )...).AddSubcontrolsToCategory(flagNamesCategory, flagNameControl) } if envVarControl != nil { strictControls.FilterByNames(append( controlNames, controls.TerragruntPrefixEnvVars, controls.DeprecatedEnvVars, )...).AddSubcontrolsToCategory(envVarsCategory, envVarControl) } return true }Also applies to: 152-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/commands/help/command.go(2 hunks)cli/commands/version/command.go(1 hunks)cli/flags/deprecated_flag.go(2 hunks)cli/flags/global/flags.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/commands/version/command.go
⏰ 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 (4)
cli/commands/help/command.go (1)
17-26: LGTM! Clean and focused command setup.The
NewCommandfunction is well-structured and follows the single responsibility principle by delegating the actual help action to a separate function.cli/flags/global/flags.go (1)
305-324: LGTM! Clean help and version flag implementation.The code nicely delegates to dedicated command packages, improving modularity and maintainability.
cli/flags/deprecated_flag.go (2)
76-78: Nice defensive programming improvement! 👍The conditional assignment of controls based on the registration result prevents potential issues when registration fails. This makes the code more robust and safer.
98-98:✅ Verification successful
Verify all implementations of RegisterStrictControlsFunc
The return type change from
voidtoboolis a breaking change. Let's make sure we haven't missed any implementations.
🏁 Script executed:
#!/bin/bash # Search for all implementations of RegisterStrictControlsFunc to verify they return bool rg --type go "func.*\(.*strict\.Control,\s*strict\.Control\)" -A 1Length of output: 68
Let's go a bit deeper to ensure that we haven’t overlooked any instances of implementations or assignments of
RegisterStrictControlsFunc. The previous script didn’t return any output, so we should widen our net. Please run the following script to search for all references toRegisterStrictControlsFuncin the codebase:Once we have the output from this command, we can verify that all implementations properly reflect the updated return type. Let me know the results so that we can confirm everything is in order.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search the entire repository for occurrences of RegisterStrictControlsFunc along with some context rg --type go "RegisterStrictControlsFunc" -A 2 -nLength of output: 3999
All implementations of
RegisterStrictControlsFuncreturn the updated bool as expected.The search results show that every reference—whether creating a new instance via helper functions or passing it around—consistently uses the updated signature in files like
cli/flags/deprecated_flag.goandcli/flags/flag_opts.go. No outdated usage was found.Please give a final manual check to ensure that any indirect usages or integrations align with this change.
|
|
||
| // Command category names. | ||
| const ( | ||
| // MainCommandsCategoryName represents primary Terragrunt operations like run, run-all. |
There was a problem hiding this comment.
| // MainCommandsCategoryName represents primary Terragrunt operations like run, run-all. | |
| // MainCommandsCategoryName represents primary Terragrunt operations like run, exec. |
NIT: We'll forget to update this later.
cli/flags/flag.go
Outdated
| @@ -34,36 +34,41 @@ func NewMovedFlag(deprecatedFlag cli.Flag, newCommandName string, regControlsFn | |||
| } | |||
|
|
|||
| // TakesValue implements `cli.Flag` interface. | |||
| func (new *Flag) TakesValue() bool { | |||
| if new.Flag.Value() == nil { | |||
| // It returns `true` for all flags except boolean ones that are `false` or `true` inverted. | |||
There was a problem hiding this comment.
| // It returns `true` for all flags except boolean ones that are `false` or `true` inverted. | |
| // TakesValue returns `true` for all flags except boolean ones that are `false` or `true` inverted. |
NIT
There was a problem hiding this comment.
I also don't really understand what this doc string means.
There was a problem hiding this comment.
I specified TakesValue at the first line.
There was a problem hiding this comment.
// TakesValue implements `github.com/urfave/cli.DocGenerationFlag` required to generate help.
// TakesValue returns `true` for all flags except boolean ones that are `false` or `true` inverted.
| // }, "run", nil)) | ||
| // | ||
| // Log output: | ||
| // WARN The `--no-auto-init` global flag is moved to `run` command and will be removed from the global flags in a future version. Use `run --no-auto-init` instead. |
There was a problem hiding this comment.
| // WARN The `--no-auto-init` global flag is moved to `run` command and will be removed from the global flags in a future version. Use `run --no-auto-init` instead. | |
| // WARN The global `--no-auto-init` flag has moved to the `run` command and will not be accessible as a global flag in a future version of Terragrunt. Use `run --no-auto-init` instead. |
Make sure to update the actual warning too.
There was a problem hiding this comment.
We need to change it here, since I only demonstrated the log output.
cli/flags/global/flags.go
Outdated
| // and each time we call `commands.New` we define them again (deprecated flags), | ||
| // we need to clear `Strict Controls` to avoid them being displayed and causing duplicate warnings, for example, log output: | ||
| // | ||
| // WARN The `--terragrunt-no-auto-init` global flag is moved to `run` command and will be removed from the global flags in a future version. Use `--run --no-auto-init=false` instead. |
There was a problem hiding this comment.
See my note above about updating this text.
cli/flags/global/flags.go
Outdated
| // we need to clear `Strict Controls` to avoid them being displayed and causing duplicate warnings, for example, log output: | ||
| // | ||
| // WARN The `--terragrunt-no-auto-init` global flag is moved to `run` command and will be removed from the global flags in a future version. Use `--run --no-auto-init=false` instead. | ||
| // WARN The `--terragrunt-no-auto-init` flag is deprecated and will be removed in a future version. Use `--no-auto-init=false` instead. |
There was a problem hiding this comment.
| // WARN The `--terragrunt-no-auto-init` flag is deprecated and will be removed in a future version. Use `--no-auto-init=false` instead. | |
| // WARN The `--terragrunt-no-auto-init` flag is deprecated and will be removed in a future version of Terragrunt. Use `--no-auto-init` instead. |
Note the second part of the change: users should set the value to --no-auto-init, not --no-auto-init=false. Even if it works, it's a double-negative and that's confusing.
There was a problem hiding this comment.
Regarding =false, yes I fixed it in this PR. I just took this log when running on the main branch. Fixed comment.
Regarding of Terragrunt, We need to change it here first.
| ErrorFmt: "The `--%s` global flag is no longer supported. Use `--%s` instead.", | ||
| WarningFmt: "The `--%s` global flag is moved to `" + commandName + "` command and will be removed from the global flags in a future version. Use `--%s` instead.", | ||
| ErrorFmt: "The `--%s` global flag is no longer supported. Use `%s` instead.", | ||
| WarningFmt: "The `--%s` global flag is moved to `" + commandName + "` command and will be removed from the global flags in a future version. Use `%s` instead.", |
There was a problem hiding this comment.
| WarningFmt: "The `--%s` global flag is moved to `" + commandName + "` command and will be removed from the global flags in a future version. Use `%s` instead.", | |
| WarningFmt: "The global `--%s` flag has moved to the `" + commandName + "` command and will not be accessible as a global flag in a future version of Terragrunt. Use `%s` instead.", |
These initially confused me. It would help to add a comment above this that the second placeholder has both the command and the flag, and that's why it doesn't have the dashes.
Description
Fixes #3878.
Fixes #3872.
Got rid of similar warning messages for moved global flags:
Reduced to:
Summary by CodeRabbit
New Features
Refactor
Tests