fix: use StringArrayVar for --key flag to preserve special characters#206
Open
Raymondhou0917 wants to merge 1 commit intozeabur:mainfrom
Open
fix: use StringArrayVar for --key flag to preserve special characters#206Raymondhou0917 wants to merge 1 commit intozeabur:mainfrom
Raymondhou0917 wants to merge 1 commit intozeabur:mainfrom
Conversation
…zeabur#201) Cobra's StringToStringVarP uses an internal CSV parser that mangles values containing ${}, commas, and other special characters. This causes `--key 'DATABASE_URL=${POSTGRESQL.POSTGRES_CONNECTION_STRING}'` to silently truncate or empty the value. Switch to StringArrayVarP and manually split each entry on the first `=` only, preserving arbitrary characters in values. Applied to both `variable create` and `variable update` commands. Fixes zeabur#201 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
internal/cmd/variable/update/update.go (1)
109-119: Consider extractingparseRawKeysto a shared variable helper.This parser is duplicated in
internal/cmd/variable/create/create.goandinternal/cmd/variable/update/update.go. A shared helper would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/variable/update/update.go` around lines 109 - 119, The parseRawKeys function is duplicated; extract it into a single shared helper (e.g., export as ParseRawKeys or keep parseRawKeys in a new helper file) and replace both copies with calls to that helper. Specifically, move the logic from parseRawKeys into a new helper file in the same package, keep the same signature (map[string]string, error) and name (or export as ParseRawKeys), update callers in update.go and create.go to call the shared helper, and adjust imports/visibility as needed so both command implementations use the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmd/variable/create/create.go`:
- Line 40: The interactive prompt path currently ignores the supplied --key/-k
flags (opts.rawKeys) causing prompts even when flags exist; change the logic in
the create command so that before invoking interactive prompts (check
f.Interactive), you parse opts.rawKeys into the expected map/list (the same
parsing done in the non-interactive branch around the current parsing block that
uses opts.rawKeys -> opts.Pairs or similar) and, if any keys were provided,
bypass the prompt and use the parsed values; ensure you reference and reuse the
existing parsing routine or replicate its behavior so opts.rawKeys is honored
and prompts are skipped when non-empty.
In `@internal/cmd/variable/update/update.go`:
- Line 40: runUpdateVariable currently ignores opts.rawKeys in the interactive
path; update runUpdateVariable to check opts.rawKeys (the flag bound via
cmd.Flags().StringArrayVarP) and, when non-empty, skip the interactive prompt
and call the existing parseKeyPairs (or equivalent) to populate the variable
values instead of prompting. Ensure you reference opts.rawKeys,
runUpdateVariable, and parseKeyPairs (or the function used at 122-129 in the
non-interactive path) so the non-interactive parsing logic is reused for both
modes.
---
Nitpick comments:
In `@internal/cmd/variable/update/update.go`:
- Around line 109-119: The parseRawKeys function is duplicated; extract it into
a single shared helper (e.g., export as ParseRawKeys or keep parseRawKeys in a
new helper file) and replace both copies with calls to that helper.
Specifically, move the logic from parseRawKeys into a new helper file in the
same package, keep the same signature (map[string]string, error) and name (or
export as ParseRawKeys), update callers in update.go and create.go to call the
shared helper, and adjust imports/visibility as needed so both command
implementations use the single implementation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 681dd2e6-e038-4aca-adc8-93b8d656a485
📒 Files selected for processing (2)
internal/cmd/variable/create/create.gointernal/cmd/variable/update/update.go
| util.AddEnvOfServiceParam(cmd, &opts.environmentID) | ||
| cmd.Flags().BoolVarP(&opts.skipConfirm, "yes", "y", false, "Skip confirmation") | ||
| cmd.Flags().StringToStringVarP(&opts.keys, "key", "k", nil, "Key value pair of the variable") | ||
| cmd.Flags().StringArrayVarP(&opts.rawKeys, "key", "k", nil, "Key value pair of the variable (KEY=VALUE)") |
There was a problem hiding this comment.
--key should bypass prompts in interactive sessions, but currently doesn’t.
Line 40 captures raw --key values, yet Line 102-109 parses them only in the non-interactive path. With f.Interactive=true, users are still forced into prompts even when flags are supplied.
Suggested fix
func runCreateVariable(f *cmdutil.Factory, opts *Options) error {
+ // If --key is provided, execute flag-driven flow directly.
+ if len(opts.rawKeys) > 0 {
+ return runCreateVariableNonInteractive(f, opts)
+ }
if f.Interactive {
return runCreateVariableInteractive(f, opts)
} else {
return runCreateVariableNonInteractive(f, opts)
}
}As per coding guidelines: "Commands should support both interactive and non-interactive modes; if a flag is provided, skip the interactive prompt".
Also applies to: 102-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/variable/create/create.go` at line 40, The interactive prompt
path currently ignores the supplied --key/-k flags (opts.rawKeys) causing
prompts even when flags exist; change the logic in the create command so that
before invoking interactive prompts (check f.Interactive), you parse
opts.rawKeys into the expected map/list (the same parsing done in the
non-interactive branch around the current parsing block that uses opts.rawKeys
-> opts.Pairs or similar) and, if any keys were provided, bypass the prompt and
use the parsed values; ensure you reference and reuse the existing parsing
routine or replicate its behavior so opts.rawKeys is honored and prompts are
skipped when non-empty.
| util.AddEnvOfServiceParam(cmd, &opts.environmentID) | ||
| cmd.Flags().BoolVarP(&opts.skipConfirm, "yes", "y", false, "Skip confirmation") | ||
| cmd.Flags().StringToStringVarP(&opts.keys, "key", "k", nil, "Key value pair of the variable") | ||
| cmd.Flags().StringArrayVarP(&opts.rawKeys, "key", "k", nil, "Key value pair of the variable (KEY=VALUE)") |
There was a problem hiding this comment.
--key input is ignored when running interactively.
Line 40 accepts --key, but Line 122-129 only parses it in the non-interactive path. In an interactive terminal, runUpdateVariable still prompts instead of honoring provided flags, which breaks expected flag-driven execution.
Suggested fix
func runUpdateVariable(f *cmdutil.Factory, opts *Options) error {
+ // If --key is provided, honor flags and skip interactive prompts.
+ if len(opts.rawKeys) > 0 {
+ return runUpdateVariableNonInteractive(f, opts)
+ }
if f.Interactive {
opts.keys = make(map[string]string)
return runUpdateVariableInteractive(f, opts)
}
return runUpdateVariableNonInteractive(f, opts)
}As per coding guidelines: "Commands should support both interactive and non-interactive modes; if a flag is provided, skip the interactive prompt".
Also applies to: 122-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/variable/update/update.go` at line 40, runUpdateVariable
currently ignores opts.rawKeys in the interactive path; update runUpdateVariable
to check opts.rawKeys (the flag bound via cmd.Flags().StringArrayVarP) and, when
non-empty, skip the interactive prompt and call the existing parseKeyPairs (or
equivalent) to populate the variable values instead of prompting. Ensure you
reference opts.rawKeys, runUpdateVariable, and parseKeyPairs (or the function
used at 122-129 in the non-interactive path) so the non-interactive parsing
logic is reused for both modes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
variable createandvariable updatefromStringToStringVarPtoStringArrayVarP--keyentry on the first=only viastrings.SplitN(raw, "=", 2)${}, commas, and other special charactersProblem
--key 'DATABASE_URL=${POSTGRESQL.POSTGRES_CONNECTION_STRING}'silently truncates or empties the value because Cobra'sStringToStringVaruses an internal CSV parser that cannot handle these characters.Changes
internal/cmd/variable/create/create.go— addedrawKeys []stringfield,parseRawKeys()helperinternal/cmd/variable/update/update.go— same changesBackward Compatibility
Users can still use
--key KEY=VALUEformat exactly as before. Values containing=signs (e.g., connection strings) are preserved because only the first=is used as the delimiter.Fixes #201
Summary by CodeRabbit
createandupdatecommands now validate--key/-kinput more strictly, rejecting empty keys and invalid KEY=VALUE formats with clear error messages.