fix(config): Fix flag parsing with space-separated value#6
fix(config): Fix flag parsing with space-separated value#6adamwg merged 2 commits intocrossplane:mainfrom
Conversation
While reviewing/testing crossplane#3 I noticed that the space-separated `--config /path/to/config.yaml` ignores the config. Reproducer: ```bash # Create a new config file go run ./cmd/crossplane --config=/tmp/xpcfg.yaml config set features.enableAlpha true # Correctly prints the full content of the config go run ./cmd/crossplane --config=/tmp/xpcfg.yaml config view # (before the fix) prints only "version: 1" ignoring the config go run ./cmd/crossplane --config /tmp/xpcfg.yaml config view ``` The problem is that `strings.TrimPrefix` returns the original string unchanged when the prefix doesn't match, so the `v != ""` check can not distinguish "prefix matched and value extracted" from "prefix not matched". For `--config /path` this makes `configFlag` return "--config" as the path instead of falling through to read the next argument. Switching to `strings.CutPrefix`, which returns an ok bool that makes the distinction clear. Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
📝 WalkthroughWalkthroughThe ChangesConfig Flag Parsing Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (1)
cmd/crossplane/main.go (1)
139-142: ⚡ Quick winOptional: tighten the
HasPrefixguard to avoid a potential false positive.
HasPrefix(a, "--config")would also match a future flag like--config-diror--configs. If such a flag appeared before--configin argv andCutPrefixreturnedfalse,args[i+1]would be silently consumed as the config path. The function would work correctly today since no such flags exist, but it might be worth narrowing the guard for resilience.Could you clarify whether restricting the early-continue filter to exactly
--configand--config=...was considered? Something like:🛡️ Proposed tightening of the HasPrefix guard
- if !strings.HasPrefix(a, "--config") { + if a != "--config" && !strings.HasPrefix(a, "--config=") { continue }This would make the logic self-consistent with the
CutPrefixcheck on the next line and eliminate the reliance on "no other--config*flags exist today."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/main.go` around lines 139 - 142, The loop's early-continue uses strings.HasPrefix(a, "--config") which could falsely match flags like --config-dir; change the guard to only allow the exact flag or the equals form by checking a == "--config" OR strings.HasPrefix(a, "--config=") (so the branch matches either --config <value> or --config=<value>), keeping the existing strings.CutPrefix usage for the equals case and still using args[i+1] for the space-separated form; update the condition in the for-loop (the locals "a", "args", and the strings.HasPrefix check) accordingly to avoid consuming args[i+1] for other --config* flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/crossplane/main.go`:
- Around line 139-142: The loop's early-continue uses strings.HasPrefix(a,
"--config") which could falsely match flags like --config-dir; change the guard
to only allow the exact flag or the equals form by checking a == "--config" OR
strings.HasPrefix(a, "--config=") (so the branch matches either --config <value>
or --config=<value>), keeping the existing strings.CutPrefix usage for the
equals case and still using args[i+1] for the space-separated form; update the
condition in the for-loop (the locals "a", "args", and the strings.HasPrefix
check) accordingly to avoid consuming args[i+1] for other --config* flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7842535-e472-432b-a41d-09fbf241c87b
📒 Files selected for processing (1)
cmd/crossplane/main.go
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
Good catch, thanks for fixing this. Might be worth adding a test as well.
Adds a table-driven test for configFlag The `EmptyEquals` case revealed that `--config=` silently fell back to the default config path, while a trailing `--config` errored cleanly. Treat both empty cases the same way and error out, so the user gets immediate feedback either way. Signed-off-by: Theo Chatzimichos <tampakrap@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/main.go (1)
140-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten
--configdetection to avoid false positivesNice fix overall. One remaining edge case: Line 140 matches any arg that starts with
--config(e.g.--configPath), and then Line 152 may incorrectly consume the next token as a config path. Could we restrict matching to only--configor--config=<value>?Suggested patch
- if !strings.HasPrefix(a, "--config") { + if a != "--config" && !strings.HasPrefix(a, "--config=") { continue } if v, ok := strings.CutPrefix(a, "--config="); ok { if v == "" { return "", errors.New("flag --config requires a value") } return v, nil }As per coding guidelines
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/main.go` around lines 140 - 152, The loop currently treats any argument starting with "--config" as the flag which causes false positives (e.g. "--configPath"); update the detection so it only accepts either the exact token "--config" or the prefixed form "--config=<value>" by replacing the strings.HasPrefix(a, "--config") check with explicit checks: first try strings.CutPrefix(a, "--config=") as already done, then if that fails only proceed when a == "--config" (and then consume args[i+1] after validating it exists and is non-empty); ensure you do not treat other long flags that begin with "--config" as matches in the loop that contains the existing CutPrefix and next-token consumption logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/main.go`:
- Around line 144-147: This change makes the flag parsing for "--config=" (the
strings.CutPrefix(a, "--config=") branch that checks v == "") strictly require a
value and removes the previous empty-fallback behavior; update the PR to either
add the 'breaking-change' label to reflect the user-visible behavior change or
add a clear note in the release notes/changelog and cmd/* documentation
explaining why the previous fallback was intentionally removed and why this is
not considered a breaking change (also confirm the identical change at the other
occurrence handling "--config=").
---
Outside diff comments:
In `@cmd/crossplane/main.go`:
- Around line 140-152: The loop currently treats any argument starting with
"--config" as the flag which causes false positives (e.g. "--configPath");
update the detection so it only accepts either the exact token "--config" or the
prefixed form "--config=<value>" by replacing the strings.HasPrefix(a,
"--config") check with explicit checks: first try strings.CutPrefix(a,
"--config=") as already done, then if that fails only proceed when a ==
"--config" (and then consume args[i+1] after validating it exists and is
non-empty); ensure you do not treat other long flags that begin with "--config"
as matches in the loop that contains the existing CutPrefix and next-token
consumption logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a3fe597-db1e-4492-8872-a131f8c4d316
📒 Files selected for processing (2)
cmd/crossplane/main.gocmd/crossplane/main_test.go
| if v, ok := strings.CutPrefix(a, "--config="); ok { | ||
| if v == "" { | ||
| return "", errors.New("flag --config requires a value") | ||
| } |
There was a problem hiding this comment.
Please confirm breaking-change labeling for --config= behavior change
Thanks for adding explicit validation. Since this removes prior behavior (--config= no longer falls back), can you confirm whether this PR should carry a breaking-change label (or document why this is exempt)?
As per coding guidelines Do not remove behavior in apis/** or cmd/** without labeling as 'breaking-change'.
Also applies to: 155-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/main.go` around lines 144 - 147, This change makes the flag
parsing for "--config=" (the strings.CutPrefix(a, "--config=") branch that
checks v == "") strictly require a value and removes the previous empty-fallback
behavior; update the PR to either add the 'breaking-change' label to reflect the
user-visible behavior change or add a clear note in the release notes/changelog
and cmd/* documentation explaining why the previous fallback was intentionally
removed and why this is not considered a breaking change (also confirm the
identical change at the other occurrence handling "--config=").
2 participants
Description of your changes
While reviewing/testing #3 I noticed that the space-separated
--config /path/to/config.yamlignores the config. Reproducer:The problem is that
strings.TrimPrefixreturns the original string unchanged when the prefix doesn't match, so thev != ""check can not distinguish "prefix matched and value extracted" from "prefix not matched". For--config /paththis makesconfigFlagreturn "--config" as the path instead of falling through to read the next argument.Switching to
strings.CutPrefix, which returns an ok bool that makes the distinction clear.I have:
./nix.sh flake checkto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.