Conversation
- 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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR relocates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/iacargs/iacargs.go`:
- Around line 362-366: IsDestroyCommand currently checks
a.Contains("-"+CommandNameDestroy), which fails for forms like -destroy=true;
update IsDestroyCommand to use the flag-normalizing helper HasFlag with the flag
name (e.g., HasFlag(CommandNameDestroy) or HasFlag("destroy")) instead of
Contains so that variants like -destroy=true are recognized; reference
IsDestroyCommand, Contains, HasFlag, and CommandNameDestroy when making this
change.
In `@internal/runner/runnerpool/runner.go`:
- Around line 334-336: The call to
terragruntOptions.TerraformCliArgs.IsDestroyCommand can panic if
TerraformCliArgs is nil; update the check around IsDestroyCommand in the code
that currently calls applyPreventDestroyExclusions(l, units) so it first
verifies terragruntOptions != nil && terragruntOptions.TerraformCliArgs != nil
before calling IsDestroyCommand. Locate the use of terragruntOptions and the
IsDestroyCommand invocation and short-circuit to skip
applyPreventDestroyExclusions when TerraformCliArgs is nil to match existing
defensive checks in InsertTerraformCliArgs/AppendTerraformCliArgs.
| // IsDestroyCommand returns true if this represents a destroy operation. | ||
| // It checks both the command name and the -destroy flag. | ||
| func (a *IacArgs) IsDestroyCommand(cmd string) bool { | ||
| return cmd == CommandNameDestroy || a.Contains("-"+CommandNameDestroy) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the IacArgs implementation
cat -n internal/iacargs/iacargs.go | head -100Repository: gruntwork-io/terragrunt
Length of output: 3391
🏁 Script executed:
# Search for HasFlag and Contains implementations
rg -A 10 "func.*HasFlag" internal/iacargs/Repository: gruntwork-io/terragrunt
Length of output: 2503
🏁 Script executed:
# Check if there are other destroy-related methods
rg -i "destroy" internal/iacargs/iacargs.goRepository: gruntwork-io/terragrunt
Length of output: 562
🌐 Web query:
Terraform CLI destroy command flag formats accepted (-destroy, -destroy=true, --destroy)
💡 Result:
-
Supported (Terraform CLI “destroy mode” flag):
terraform apply -destroy/terraform plan -destroy(enables destroy mode). Theterraform destroycommand is effectivelyterraform apply -destroy. [1][2]terraform apply -destroy=true/terraform plan -destroy=true(explicit boolean form). Terraform’s CLI supports boolean flags with an explicit=true|falseform (e.g.,-refresh=false), and-destroyis a boolean planning-mode switch. [2][3]
-
Not supported / not documented:
--destroy(Terraform’s official docs consistently show single-dash long options like-destroy, not GNU-style--destroy). [1][2]
Sources:
[1] Terraform destroy command docs (alias for terraform apply -destroy) — HashiCorp Developer docs.
[2] Terraform plan command docs (activate destroy mode using -destroy) — HashiCorp Developer docs.
[3] Terraform plan command docs (example boolean option format -refresh=false) — HashiCorp Developer docs.
🏁 Script executed:
rg -A 10 "func.*Contains" internal/iacargs/iacargs.goRepository: gruntwork-io/terragrunt
Length of output: 467
🏁 Script executed:
# Also check the test file for Contains to understand its behavior
rg -B 3 -A 10 "Contains" internal/iacargs/iacargs_test.goRepository: gruntwork-io/terragrunt
Length of output: 993
Destroy detection misses -destroy=true variant
IsDestroyCommand uses Contains which performs exact matching on the Flags list. When -destroy=true is passed, the exact token -destroy is not present, so the check fails and bypasses destroy protections. Use HasFlag("destroy") which normalizes and extracts flag names, properly handling -destroy=true format.
🛠️ Suggested fix
func (a *IacArgs) IsDestroyCommand(cmd string) bool {
- return cmd == CommandNameDestroy || a.Contains("-"+CommandNameDestroy)
+ if cmd == "" {
+ cmd = a.Command
+ }
+ return cmd == CommandNameDestroy || a.HasFlag(CommandNameDestroy)
}📝 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.
| // IsDestroyCommand returns true if this represents a destroy operation. | |
| // It checks both the command name and the -destroy flag. | |
| func (a *IacArgs) IsDestroyCommand(cmd string) bool { | |
| return cmd == CommandNameDestroy || a.Contains("-"+CommandNameDestroy) | |
| } | |
| // IsDestroyCommand returns true if this represents a destroy operation. | |
| // It checks both the command name and the -destroy flag. | |
| func (a *IacArgs) IsDestroyCommand(cmd string) bool { | |
| if cmd == "" { | |
| cmd = a.Command | |
| } | |
| return cmd == CommandNameDestroy || a.HasFlag(CommandNameDestroy) | |
| } |
🤖 Prompt for AI Agents
In `@internal/iacargs/iacargs.go` around lines 362 - 366, IsDestroyCommand
currently checks a.Contains("-"+CommandNameDestroy), which fails for forms like
-destroy=true; update IsDestroyCommand to use the flag-normalizing helper
HasFlag with the flag name (e.g., HasFlag(CommandNameDestroy) or
HasFlag("destroy")) instead of Contains so that variants like -destroy=true are
recognized; reference IsDestroyCommand, Contains, HasFlag, and
CommandNameDestroy when making this change.
| if terragruntOptions.TerraformCliArgs.IsDestroyCommand(terragruntOptions.TerraformCommand) { | ||
| applyPreventDestroyExclusions(l, units) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the struct definition and initialization of TerraformCliArgs
rg -n --type=go -A 5 'type.*Options struct' | head -100Repository: gruntwork-io/terragrunt
Length of output: 5228
🏁 Script executed:
#!/bin/bash
# Look for TerraformCliArgs initialization patterns
rg -n --type=go 'TerraformCliArgs.*=' | head -40Repository: gruntwork-io/terragrunt
Length of output: 2525
🏁 Script executed:
#!/bin/bash
# Check the specific file and surrounding context
cat -n internal/runner/runnerpool/runner.go | sed -n '320,350p'Repository: gruntwork-io/terragrunt
Length of output: 1484
🏁 Script executed:
#!/bin/bash
# Search for nil checks on TerraformCliArgs
rg -n --type=go 'TerraformCliArgs.*!=' | head -20Repository: gruntwork-io/terragrunt
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Get the function signature and more context around lines 334-336
sed -n '300,340p' internal/runner/runnerpool/runner.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 1732
🏁 Script executed:
#!/bin/bash
# Find the function definition that contains line 334
sed -n '1,334p' internal/runner/runnerpool/runner.go | tac | head -100 | tac | grep -n "^func"Repository: gruntwork-io/terragrunt
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Get more complete function context
sed -n '280,365p' internal/runner/runnerpool/runner.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 3228
🏁 Script executed:
#!/bin/bash
# Get the full function signature of NewRunnerPoolStack
sed -n '237,280p' internal/runner/runnerpool/runner.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 1834
🏁 Script executed:
#!/bin/bash
# Look for where NewRunnerPoolStack is called
rg -n 'NewRunnerPoolStack' --type=go -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 1175
🏁 Script executed:
#!/bin/bash
# Check TerraformCliArgs field definition in TerragruntOptions struct
sed -n '99,200p' pkg/options/options.go | grep -A 150 'type TerragruntOptions struct' | head -120Repository: gruntwork-io/terragrunt
Length of output: 4569
🏁 Script executed:
#!/bin/bash
# Check the builder_helpers.go context where NewRunnerPoolStack is called
sed -n '195,210p' internal/runner/runnerpool/builder_helpers.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 540
🏁 Script executed:
#!/bin/bash
# Find where tgOpts comes from in builder_helpers.go
sed -n '1,202p' internal/runner/runnerpool/builder_helpers.go | tail -60 | head -50 | cat -nRepository: gruntwork-io/terragrunt
Length of output: 1708
🏁 Script executed:
#!/bin/bash
# Search for the full context of the call to NewRunnerPoolStack in builder_helpers
rg -n -B 10 'NewRunnerPoolStack' internal/runner/runnerpool/builder_helpers.go | head -40Repository: gruntwork-io/terragrunt
Length of output: 481
🏁 Script executed:
#!/bin/bash
# Check if there are other places where TerraformCliArgs is dereferenced without nil check
rg -n 'TerraformCliArgs\.' --type=go | grep -v '!=' | grep -v '==' | head -30Repository: gruntwork-io/terragrunt
Length of output: 2766
🏁 Script executed:
#!/bin/bash
# Look for the main entry point to understand how TerragruntOptions is initialized
rg -n 'func main' --type=go -A 30Repository: gruntwork-io/terragrunt
Length of output: 4467
🏁 Script executed:
#!/bin/bash
# Check if there's initialization code in the CLI that creates/initializes options
rg -n 'TerragruntOptions\s*{' --type=go -A 5 | head -60Repository: gruntwork-io/terragrunt
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Search for where NewRunnerPoolStack is called from and trace back to see if options are initialized
rg -n 'createRunner\|NewRunnerPoolStack' --type=go -B 5 | head -80Repository: gruntwork-io/terragrunt
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Find the NewTerragruntOptions function to see how TerraformCliArgs is initialized
rg -n 'func NewTerragruntOptions' --type=go -A 30Repository: gruntwork-io/terragrunt
Length of output: 5901
🏁 Script executed:
#!/bin/bash
# Check pkg/options/options.go to see the initialization of TerraformCliArgs
sed -n '700,850p' pkg/options/options.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 4768
🏁 Script executed:
#!/bin/bash
# Find NewTerragruntOptions initialization function
rg -n 'NewTerragruntOptions' --type=go | head -5Repository: gruntwork-io/terragrunt
Length of output: 449
🏁 Script executed:
#!/bin/bash
# Check test files to see if TerragruntOptions is ever created without initialization
rg -n 'TerragruntOptions\{' --type=go -A 3 | head -40Repository: gruntwork-io/terragrunt
Length of output: 2472
🏁 Script executed:
#!/bin/bash
# Look for any test files that might construct TerragruntOptions directly
rg -n '&options\.TerragruntOptions' --type=go -A 2 | head -40Repository: gruntwork-io/terragrunt
Length of output: 2463
🏁 Script executed:
#!/bin/bash
# Check if IsDestroyCommand is ever called elsewhere with similar pattern
rg -n 'IsDestroyCommand' --type=go -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 2321
🏁 Script executed:
#!/bin/bash
# Check if there are specific callers of NewRunnerPoolStack and trace back the source of terragruntOptions
rg -n 'createRunner' --type=go -B 20 | grep -A 20 'func 'Repository: gruntwork-io/terragrunt
Length of output: 1578
🏁 Script executed:
#!/bin/bash
# Find all callers of createRunner to understand the flow
rg -n 'createRunner\(' --type=go -B 5 -A 2 | head -60Repository: gruntwork-io/terragrunt
Length of output: 1189
🏁 Script executed:
#!/bin/bash
# Check if there's a nil guard on line 665 where TerraformCliArgs is also dereferenced
sed -n '660,670p' internal/runner/runnerpool/runner.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Check what happens in the test that calls NewRunnerPoolStack
sed -n '30,50p' internal/runner/runnerpool/runner_test.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Check if there are any other paths where TerraformCliArgs might not be initialized
rg -n 'TerraformCliArgs.*nil' --type=go -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Verify Build function to ensure it always properly initializes terragruntOptions
sed -n '11,30p' internal/runner/runnerpool/builder.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 768
Add nil guard for TerraformCliArgs to prevent potential panic.
While TerraformCliArgs is initialized by the standard constructors in production code paths, defensive nil checks already exist elsewhere in the codebase (InsertTerraformCliArgs, AppendTerraformCliArgs). This is good practice given the possibility of direct struct construction without proper initialization.
🔧 Suggested guard
- if terragruntOptions.TerraformCliArgs.IsDestroyCommand(terragruntOptions.TerraformCommand) {
+ isDestroy := terragruntOptions.TerraformCommand == tf.CommandNameDestroy
+ if terragruntOptions.TerraformCliArgs != nil {
+ isDestroy = terragruntOptions.TerraformCliArgs.IsDestroyCommand(terragruntOptions.TerraformCommand)
+ }
+ if isDestroy {
applyPreventDestroyExclusions(l, units)
}🤖 Prompt for AI Agents
In `@internal/runner/runnerpool/runner.go` around lines 334 - 336, The call to
terragruntOptions.TerraformCliArgs.IsDestroyCommand can panic if
TerraformCliArgs is nil; update the check around IsDestroyCommand in the code
that currently calls applyPreventDestroyExclusions(l, units) so it first
verifies terragruntOptions != nil && terragruntOptions.TerraformCliArgs != nil
before calling IsDestroyCommand. Locate the use of terragruntOptions and the
IsDestroyCommand invocation and short-circuit to skip
applyPreventDestroyExclusions when TerraformCliArgs is nil to match existing
defensive checks in InsertTerraformCliArgs/AppendTerraformCliArgs.
2 participants
Description
clihelperpackageTODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.