Skip to content

Conversation

yhakbar
Copy link
Collaborator

@yhakbar yhakbar commented Sep 26, 2025

Description

Fixes #4853 in the runner-pool implementation.

TODOs

Read the Gruntwork contribution guidelines.

  • I authored this code entirely myself
  • I am submitting code based on open source software (e.g. MIT, MPL-2.0, Apache)]
  • I am adding or upgrading a dependency or adapted code and confirm it has a compatible open source license
  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

Summary by CodeRabbit

  • New Features

    • find and list now support shell-like parsing of configured commands, allowing you to specify an executable with arguments (e.g., quoted/space-separated values).
    • Consistent handling of command and arguments across discovery, improving behavior for multi-argument inputs.
    • Clearer error messages when command parsing fails.
  • Chores

    • Added a lightweight dependency to enable shell-style argument parsing.

Copy link

vercel bot commented Sep 26, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
terragrunt-docs Ready Ready Preview Comment Sep 26, 2025 3:24pm

Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
CLI command parsing (QueueConstructAs)
cli/commands/find/find.go, cli/commands/list/list.go
Parse QueueConstructAs using shellwords into Cmd (first token) and Args (rest). Replace prior raw-string usage. Enable parse-exclude when set. Add error handling for parse failures. Update discovery context construction accordingly.
Runner/discovery context wiring
internal/runner/runnerpool/builder.go
Pass discovery WithOptions(opts...). Build discovery context with Cmd and Args from Terraform CLI args (First/Tail) instead of a single command string. Signature formatting only; no exported API change.
Dependencies
go.mod
Add indirect dependency github.com/mattn/go-shellwords.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • denis256
  • wakeful

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description references the correct issue but lacks a detailed summary of the changes introduced, leaves placeholder sections under Description, Release Notes, and Migration Guide unfilled, and therefore does not adhere to the required template completeness. Please add a descriptive summary of the code changes under the Description section, fill in the Release Notes draft with the actual one-line change summary, and provide concrete migration instructions if the changes are backward incompatible.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change—updating the discovery context so that plan -destroy and apply -destroy commands function correctly—which aligns directly with the modifications in CLI parsing and context construction without extraneous details.
Linked Issues Check ✅ Passed The code changes implement parsing of the QueueConstructAs string into a command and its arguments and update the discovery context accordingly, directly addressing the need to support plan -destroy and apply -destroy while respecting dependencies as described in issue #4853.
Out of Scope Changes Check ✅ Passed All modifications are focused on enhancing CLI command parsing and discovery context construction to support destroy flags, and even the added go-shellwords dependency and formatting changes relate to that goal, so no out-of-scope changes are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/fixing-discovery-context

Comment @coderabbitai help to get the list of available commands and usage tips.

@yhakbar yhakbar force-pushed the fix/fixing-discovery-context branch from ad834fc to b598243 Compare September 26, 2025 15:23
@yhakbar yhakbar marked this pull request as ready for review September 30, 2025 13:04
@yhakbar yhakbar requested a review from denis256 as a code owner September 30, 2025 13:04
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 612a592 and b598243.

⛔ 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)

Comment on lines +50 to 66
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,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).

Comment on lines +46 to 62
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,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.

@yhakbar yhakbar merged commit 09a6c2c into main Sep 30, 2025
97 of 99 checks passed
@yhakbar yhakbar deleted the fix/fixing-discovery-context branch September 30, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't run "terragrunt run --all --out-dir /tmp/tfplan plan -destroy"
2 participants