fix(config): Fix flag parsing with space-separated value#6
fix(config): Fix flag parsing with space-separated value#6tampakrap wants to merge 1 commit 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🎯 2 (Simple) | ⏱️ ~8 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ 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.
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.