Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors CLI argument handling with flag normalization and robust flag/value parsing, adds new IacArgs builder helpers and tests (including boolean flag edge cases), introduces minor nil-guards and capacity-constant tweaks, and updates Terraform CLI args merging behavior with lazy initialization and new subcommand-merge logic. Changes
Sequence Diagram(s)(Skipped — changes are parsing/refactor-focused and do not introduce a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- prevent false positives in flag matching by checking for dash prefix - reintroduce nil guards for TerraformCliArgs in options - improve subcommand replacement logic in InsertTerraformCliArgs - restore fixture counter cleanup in run-cmd tests
|
|
||
| if ext != ".csv" && ext != ".json" { | ||
| return nil | ||
| return fmt.Errorf("unsupported report file extension: %s (supported: .csv, .json)", ext) |
There was a problem hiding this comment.
Why is this an error now? Users should be able to use whatever extension they want, we just won't auto-detect the right report format automatically if they don't use .csv or .json.
There was a problem hiding this comment.
In the old version it is just returning out, so silently, no report format will be set
There was a problem hiding this comment.
The docs say the default is CSV if it can't work it out based on extension or flag:
https://terragrunt.gruntwork.io/docs/features/run-report/#run-report
It must have been working at some point.
| newFlags := make([]string, 0, len(a.Flags)) | ||
| target := normalizeFlag(name) | ||
|
|
||
| for i := 0; i < len(a.Flags); i++ { |
There was a problem hiding this comment.
NIT: You can use the modern range syntax here.
There was a problem hiding this comment.
Ah, maybe you run into issues if you're manually manipulating the iterator on line 318?
| "-write-out", | ||
| // valueTakingFlags contains flags that require space-separated values. | ||
| // Only flags that commonly use a space-separated format need to be listed here. | ||
| var valueTakingFlags = []string{ |
There was a problem hiding this comment.
Are we keeping this because there's no way around having knowledge of OpenTofu/Terraform CLI arguments?
There was a problem hiding this comment.
Yes, we need wha to take as "next parameter"
| @@ -991,7 +991,7 @@ func (r *Runner) SetReport(rpt *report.Report) { | |||
| // isDestroyCommand checks if the current command is a destroy operation | |||
| func isDestroyCommand(cmd string, args *clihelper.IacArgs) bool { | |||
There was a problem hiding this comment.
If we move IaCArgs out of clihelper (which is pretty bloated right now), we can put stuff like this as methods on IaCArgs.
| targetOptions.CheckDependentUnits = false | ||
| targetOptions.TerraformCommand = "output" | ||
| targetOptions.TerraformCliArgs = clihelper.NewIacArgs("output", "-json") | ||
| targetOptions.TerraformCliArgs = clihelper.NewIacArgs().SetCommand("output").AppendFlag("-json") |
|
|
||
| // Subcommands: replace or append | ||
| if len(parsed.SubCommand) > 0 { | ||
| // For "providers lock", if we insert "providers mirror", we want "providers mirror". |
There was a problem hiding this comment.
I don't really understand what this means. Why are we overwriting this?
| // - If parsed.Command matches opts command, do nothing (idempotent) | ||
| // - If parsed.Command is a known subcommand, add to SubCommand | ||
| // - Otherwise treat parsed.Command as positional argument | ||
| func (opts *TerragruntOptions) mergeCommand(parsed *clihelper.IacArgs) { |
There was a problem hiding this comment.
I'm nervous about adding another method to TerragruntOptions. I feel like we're giving more responsibility to something we should be slimming down.
Description
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.