Fix double-hyphen (--) parsing in CLI commands#1990
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
📝 WalkthroughWalkthroughAdds POSIX "--" end-of-options handling to the compatibility flag translator so args after "--" are passed through untouched; adds unit and integration tests reproducing issue Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
📝 WalkthroughWalkthroughThis PR resolves issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/exec/workflow_utils_test.go`:
- Around line 1610-1625: The test re-implements the stack insertion loop from
workflow_utils.go (the block that checks for "--" and inserts "-s" with
tt.workflowStack into args), which can drift if production logic changes;
extract that insertion into a single helper (e.g., InsertWorkflowStack or
ApplyWorkflowStack) in workflow_utils.go and call it from both the production
code and workflow_utils_test.go instead of duplicating the loop, and also fix
the two inline comments in workflow_utils_test.go to end with periods to satisfy
godot.
In `@tests/cli_double_hyphen_test.go`:
- Around line 49-95: The workspace assertion is brittle: Terraform sometimes
emits `Workspace "nonprod" doesn't exist.` instead of `Switched to workspace
"nonprod"`, causing CI flakes; update the test that checks combinedOutput (the
block that uses tt.stderrContains and combinedOutput := stdout.String() +
stderr.String()) to accept either message (e.g., assert that combinedOutput
contains tt.stderrContains OR contains `Workspace "<stack>" doesn't exist.`), or
broaden the expectation by checking for either substring using tt.expectedStack
(from tests entries) so the assertion is resilient to both `Switched to
workspace "<stack>"` and `Workspace "<stack>" doesn't exist.`.
🧹 Nitpick comments (1)
tests/cli_double_hyphen_test.go (1)
32-35: Prefert.Setenv/t.Cleanupfor env isolation.Using
os.Unsetenvcan leak state across tests. Prefert.Setenv(set empty if lookup semantics allow) or wrap an unset helper witht.Cleanupto restore prior values. Based on learnings, prefert.Setenvfor env isolation in tests.Also applies to: 121-124, 172-175
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/exec/workflow_utils_test.go`:
- Around line 1620-1624: The two inline comments in the test block that
manipulate args (the comment "Insert before the \"--\"" and the comment "Append
at the end") need trailing periods to satisfy godot linting; update those
comments to "Insert before the \"--\"." and "Append at the end." respectively in
the code that appends tt.workflowStack to args (refer to variables args, idx and
tt.workflowStack in this test helper block).
In `@tests/cli_double_hyphen_test.go`:
- Around line 41-69: The test cases in tests/cli_double_hyphen_test.go pass
unknown flags after the "--" which can make Terraform fail before emitting the
workspace switch message; update the three affected test entries (the ones with
name values "stack before double-hyphen is parsed correctly", "short stack flag
before double-hyphen", and "stack=value syntax before double-hyphen") to use
valid Terraform plan flags after the "--" (for example "-input=false",
"-lock=false", or "-no-color") in their args slices so the command advances far
enough to produce the "Switched to workspace \"nonprod\"" output while still
verifying pass-through of arguments.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1990 +/- ##
=======================================
Coverage 74.92% 74.93%
=======================================
Files 777 777
Lines 71302 71307 +5
=======================================
+ Hits 53423 53433 +10
+ Misses 14399 14395 -4
+ Partials 3480 3479 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
These changes were released in v1.204.1-rc.2. |
2 participants
what
--(POSIX end-of-options marker) were incorrectly parsed by Cobra/pflag--followed by terraform flags like-consolidate-warnings=falseendOfOptionsMarkerto replace repeated"--"string literal (linting fix)why
atmos terraform plan vpc --stack nonprod -- -consolidate-warnings=falsewere failing with corrupted stack valuesolidate-warnings=falseinstead ofnonprodbecause pflag was incorrectly parsing the-consolidate-warnings=falseargument after----should be passed through to the subprocess without being parsed by the CLIreferences
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.