-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Properly sets discovery context so that plan -destroy
and apply -destroy
work right
#4887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds shell-like parsing of QueueConstructAs and Terraform CLI args into Cmd and Args in find/list commands and runner pool builder. Updates discovery context to use parsed Cmd/Args, enables parse-exclude when QueueConstructAs is set, wires discovery WithOptions. Adds go-shellwords as an indirect dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (find/list)
participant Parser as Shellwords Parser
participant Builder as RunnerPool Builder
participant Disc as Discovery
User->>CLI: terragrunt ... <QueueConstructAs>
alt QueueConstructAs set
CLI->>Parser: Parse QueueConstructAs
Parser-->>CLI: Cmd + Args or Error
CLI-->>User: Error (on parse fail)
end
CLI->>Builder: Build(..., Cmd, Args, opts)
Builder->>Disc: Configure WithOptions + Context(Cmd,Args)
Disc-->>Builder: Discovery plan
Builder-->>CLI: Runner
CLI-->>User: Execute with parsed Cmd/Args
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
…ly -destroy` work right
ad834fc
to
b598243
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (4)
cli/commands/find/find.go
(2 hunks)cli/commands/list/list.go
(2 hunks)go.mod
(1 hunks)internal/runner/runnerpool/builder.go
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
cli/commands/find/find.go
internal/runner/runnerpool/builder.go
cli/commands/list/list.go
🧬 Code graph analysis (3)
cli/commands/find/find.go (1)
internal/discovery/discovery.go (1)
DiscoveryContext
(93-96)
internal/runner/runnerpool/builder.go (3)
options/options.go (1)
TerragruntOptions
(100-332)internal/discovery/discovery.go (1)
DiscoveryContext
(93-96)internal/cli/args.go (1)
Args
(27-27)
cli/commands/list/list.go (1)
internal/discovery/discovery.go (1)
DiscoveryContext
(93-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
- GitHub Check: integration_tests / Test (Provider Cache Server with Latest Terraform)
parser := shellwords.NewParser() | ||
|
||
args, err := parser.Parse(opts.QueueConstructAs) | ||
if err != nil { | ||
return errors.New(err) | ||
} | ||
|
||
cmd := args[0] | ||
|
||
if len(args) > 1 { | ||
args = args[1:] | ||
} | ||
|
||
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | ||
Cmd: opts.QueueConstructAs, | ||
Cmd: cmd, | ||
Args: args, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle empty parse results and remove the command from Args.
If shellwords.Parse
returns no tokens (e.g., the user passed only whitespace), args[0]
panics. And when the user supplies exactly one token, we keep it in args
, so the command shows up twice (in Cmd
and Args
). Add an explicit len(args) == 0
check and always slice to args = args[1:]
.
Apply this diff:
args, err := parser.Parse(opts.QueueConstructAs)
if err != nil {
return errors.New(err)
}
- cmd := args[0]
-
- if len(args) > 1 {
- args = args[1:]
- }
+ if len(args) == 0 {
+ return errors.New("queue construct command is empty after parsing")
+ }
+
+ cmd := args[0]
+ args = args[1:]
📝 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.
parser := shellwords.NewParser() | |
args, err := parser.Parse(opts.QueueConstructAs) | |
if err != nil { | |
return errors.New(err) | |
} | |
cmd := args[0] | |
if len(args) > 1 { | |
args = args[1:] | |
} | |
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | |
Cmd: opts.QueueConstructAs, | |
Cmd: cmd, | |
Args: args, | |
}) | |
parser := shellwords.NewParser() | |
args, err := parser.Parse(opts.QueueConstructAs) | |
if err != nil { | |
return errors.New(err) | |
} | |
if len(args) == 0 { | |
return errors.New("queue construct command is empty after parsing") | |
} | |
cmd := args[0] | |
- if len(args) > 1 { | |
- args = args[1:] | |
args = args[1:] | |
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | |
Cmd: cmd, | |
Args: args, | |
}) |
🤖 Prompt for AI Agents
In cli/commands/find/find.go around lines 50 to 66, the code assumes
parser.Parse returned at least one token which can panic and also leaves the
single token in args resulting in duplicate command; after parsing, check if
len(args) == 0 and return a clear error if so, then set cmd := args[0] and
always drop the command from args with args = args[1:] (no conditional), and
preserve the existing error handling for Parse (wrap/return the parse error
appropriately).
parser := shellwords.NewParser() | ||
|
||
args, err := parser.Parse(opts.QueueConstructAs) | ||
if err != nil { | ||
return errors.New(err) | ||
} | ||
|
||
cmd := args[0] | ||
|
||
if len(args) > 1 { | ||
args = args[1:] | ||
} | ||
|
||
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | ||
Cmd: opts.QueueConstructAs, | ||
Cmd: cmd, | ||
Args: args, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against empty parse results and strip the Cmd from Args.
parser.Parse
can return an empty slice (e.g., user passes only whitespace), so args[0]
will panic. And when the input is a single token (e.g., "plan"
), args
still contains that token because we only slice when len(args) > 1
, causing the command to appear in both Cmd
and Args
. Please bail out if len(args) == 0
, and always set args = args[1:]
so the command isn’t duplicated.
Apply this diff:
args, err := parser.Parse(opts.QueueConstructAs)
if err != nil {
return errors.New(err)
}
- cmd := args[0]
-
- if len(args) > 1 {
- args = args[1:]
- }
+ if len(args) == 0 {
+ return errors.New("queue construct command is empty after parsing")
+ }
+
+ cmd := args[0]
+ args = args[1:]
📝 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.
parser := shellwords.NewParser() | |
args, err := parser.Parse(opts.QueueConstructAs) | |
if err != nil { | |
return errors.New(err) | |
} | |
cmd := args[0] | |
if len(args) > 1 { | |
args = args[1:] | |
} | |
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | |
Cmd: opts.QueueConstructAs, | |
Cmd: cmd, | |
Args: args, | |
}) | |
parser := shellwords.NewParser() | |
args, err := parser.Parse(opts.QueueConstructAs) | |
if err != nil { | |
return errors.New(err) | |
} | |
if len(args) == 0 { | |
return errors.New("queue construct command is empty after parsing") | |
} | |
cmd := args[0] | |
args = args[1:] | |
d = d.WithDiscoveryContext(&discovery.DiscoveryContext{ | |
Cmd: cmd, | |
Args: args, | |
}) |
🤖 Prompt for AI Agents
In cli/commands/list/list.go around lines 46 to 62, guard against parser.Parse
returning an empty slice and ensure the command token is removed from Args:
after parsing, if len(args) == 0 return a descriptive error (e.g., "empty
command" or wrap the parse error), then set cmd := args[0] and unconditionally
set args = args[1:] so the command is not duplicated in Args before creating the
DiscoveryContext.
Description
Fixes #4853 in the
runner-pool
implementation.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Chores