fix: accept comma-separated values on deployment target and scope flags - #692
fix: accept comma-separated values on deployment target and scope flags#692NickJosevski wants to merge 4 commits into
Conversation
`--deployment-target "ABC,XYZ"` was sent to the server as a single target name because the flag is a pflag StringArray, while its legacy aliases (`--target`, `--specificMachines`) are StringSlice and already split on commas. Expand comma-separated values for the environment, tenant, tenant-tag and target flags on `release deploy` and `runbook run`, so the comma form matches the repeat-the-flag form. Values that can legitimately contain a comma (--variable, --skip, package/git-resource specs) are left alone. Fixes #556 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@NickJosevski picking this up |
| func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error { | ||
| // these flags accept a comma-separated list as well as being specified multiple times | ||
| flags.Environments.Value = executionscommon.ExpandCommaSeparated(flags.Environments.Value) | ||
| flags.Tenants.Value = executionscommon.ExpandCommaSeparated(flags.Tenants.Value) |
There was a problem hiding this comment.
Blank-drop can silently flip a tenanted deploy to untenanted. ExpandCommaSeparated drops blank entries and returns nil when everything was blank, and pkg/executor/release.go:178 routes on isTenanted := len(params.Tenants) > 0 || len(params.TenantTags) > 0.
Concrete scenario (CI is exactly where this happens): --tenant "$TENANT_A,$TENANT_B" with both variables unset/empty yields "," → expands to nil → the CLI silently submits an untenanted deployment to the environment. Before this PR the literal "," (or "") was sent as a tenant name and the server rejected it. Same class of change for --exclude-deployment-target "$X" with $X empty: the exclusion list silently becomes empty instead of erroring.
Suggest erroring (or at least warning) when a flag value expands to nothing but the flag was explicitly provided, e.g. check cmd.Flags().Changed(name) && len(expanded) == 0.
| } | ||
| result := make([]string, 0, len(values)) | ||
| for _, value := range values { | ||
| for _, component := range strings.Split(value, ",") { |
There was a problem hiding this comment.
No escape hatch for names containing commas — and interactive mode now emits automation commands that re-split them. The split is unconditional, so a tenant/target/environment named e.g. Foo, Inc can no longer be passed through the primary flags at all (the PR description flags this; adding a data point on the second-order effect).
The sharper edge is the interactive echo: values chosen from a picker are backfilled into resolvedFlags and GenerateAutomationCmd emits --tenant 'Foo, Inc' (pkg/util/flag/flag.go []string case emits values verbatim, one flag per element). Pasting that "Automation Command" into CI now splits it into Foo and Inc — a hard error if those don't exist, or a deploy to the wrong tenants/targets if they do. So the interactive flow can now hand the user a re-runnable command that isn't re-runnable, with no workaround. Worth deciding on the escape-hatch question before shipping rather than after.
| } | ||
|
|
||
| func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error { | ||
| // these flags accept a comma-separated list as well as being specified multiple times |
There was a problem hiding this comment.
Altitude: the expansion lives in two run functions rather than the flag layer. Two costs:
- Same-named flags now behave differently across commands:
tenant connect --environment/-e(pkg/cmd/tenant/connect/connect.go:108) still does not split commas, so-e "dev,test"works onrelease deploybut sends the literal string ontenant connect. - Mutating
flags.X.Valueat the top of the run function creates an ordering dependency — any future code reading these flags inPreRunEor before these lines sees unsplit values, and every new command must remember to add the block.
A parse-time mechanism (a small splitting pflag.Value wrapper, or a util.StringArrayCommaSeparated(...) registration helper next to AddFlagAliasesStringSlice in pkg/util/pflagaliases.go) would give every command the behavior consistently and remove the ordering hazard.
|
|
||
| func runbookRun(cmd *cobra.Command, f factory.Factory, flags *RunFlags) error { | ||
| // these flags accept a comma-separated list as well as being specified multiple times | ||
| flags.Environments.Value = executionscommon.ExpandCommaSeparated(flags.Environments.Value) |
There was a problem hiding this comment.
Minor: this five-line block is duplicated verbatim from deployRun. If the flag set grows (the PR description already floats --runbook-tag), the two lists have to be kept in sync by hand. A tiny shared helper would collapse both call sites, e.g. executionscommon.ExpandCommaSeparatedAll(&flags.Environments.Value, &flags.Tenants.Value, ...) taking ...*[]string.
Review feedback: the five-line expansion block at the top of deployRun was duplicated verbatim in runbookRun, so any new multi-value flag has to be added to two hand-maintained lists. ExpandCommaSeparatedFlags takes the flags themselves and expands them in place, leaving one call per command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… them
Review feedback: dropping blanks let an explicitly-provided flag expand to
nothing. Because pkg/executor/release.go routes on
`len(params.Tenants) > 0 || len(params.TenantTags) > 0`, `--tenant "$A,$B"`
with both variables unset expanded to nil and the CLI silently submitted an
*untenanted* deployment to the environment. Before this branch the literal ","
was sent as a tenant name and the server rejected it. The same class of change
applied to `--exclude-deployment-target "$X"` with $X empty, where the
exclusion list quietly became empty.
A blank component always means a caller-side substitution produced nothing, so
ExpandCommaSeparated now returns an error naming the flag and quoting the
offending value. This also covers the partial case ("$A,$B" with only $B
empty), which would otherwise have silently narrowed the deployment scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: the split was unconditional, so a tenant/target/environment named e.g. "Foo, Inc" could no longer be passed through the primary flags at all. The sharper edge was the interactive echo — a value chosen from a picker is backfilled into resolvedFlags and flag.GenerateAutomationCmd emits it verbatim, so the printed "Automation Command" was not re-runnable: pasting it into CI would split "Foo, Inc" back into two names, erroring if they don't exist or deploying to the wrong tenants if they do. `\,` now means a literal comma. A backslash anywhere else is preserved verbatim, so names such as DOMAIN\host are unaffected. Interactive selections are escaped with executionscommon.EscapeCommas on the way into the automation command, so the echoed command round-trips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Fixes #556
Root cause
--deployment-targetis declared as a pflag StringArray, which does not split on commas:pkg/cmd/release/deploy/deploy.go:173—flags.StringArrayVarP(&deployFlags.DeploymentTargets.Value, ...)Its legacy aliases are registered as StringSlice, which does split on commas:
pkg/util/pflagaliases.go:39—flags.StringSlice(alias, nil, ""), used byAddFlagAliasesStringSlicepkg/cmd/release/deploy/deploy.go:78—FlagAliasSpecificMachines = "specificMachines" // octo wants a comma separated list ... but CSV also works because pflag does it for freeSo today
--specificMachines "ABC,XYZ"and--target "ABC,XYZ"both work, but the primary--deployment-target "ABC,XYZ"sends the literal stringABC,XYZto the server, producingUnable to locate deployment target(s) named 'ABC,XYZ'. The bug is an inconsistency between aflag and its own aliases, not a missing feature.
What changed
executionscommon.ExpandCommaSeparated— splits each entry on commas, trims surroundingwhitespace, drops blanks, preserves order and duplicates. Nil in, nil out.
deployRunandrunbookRunto:--environment,--tenant,--tenant-tag,--deployment-target/--run-target,--exclude-deployment-target/--exclude-run-target.Deliberately not applied to
--variable(values are arbitrary text),--skip(step names),--package/--git-resource(structured specs),--deployment-freeze-name, or--runbook-tag.The repeat-the-flag form is unchanged, so existing scripts keep working. Expansion happens before
optionsis built, so the interactive backfill intoresolvedFlagsandflag.GenerateAutomationCmdsee the already-split values;GenerateAutomationCmdemits[]stringas one
--flag 'value'per element (pkg/util/flag/flag.go:77-84), so the echoed automationcommand stays correct and re-runnable —
--deployment-target 'ABC,XYZ'in becomes--deployment-target 'ABC' --deployment-target 'XYZ'out.Test evidence
go build ./...— clean.go test ./pkg/...— 64 packagesok, 0 failures.New tests:
TestExpandCommaSeparated(pkg/executionscommon/executionscommon_test.go) — comma form,repeated form, mixed form, values containing spaces, whitespace trimming around the comma,
tenant-tag canonical values, blank entries, nil.
release deploy accepts comma-separated targets and environments; untenanted— asserts thewire request carries
SpecificMachineNames: ["first Machine", "second Machine", "third Machine"]from
--deployment-target "first Machine, second Machine" --deployment-target "third Machine".release deploy accepts comma-separated tenants and tenant tags; tenanted.runbook run accepts comma-separated environments and targets.Open questions / options
1. Scope — this one flag, or the whole multi-value execution flag set?
--deployment-target: smallest blast radius, but leaves--environment "dev,test"still broken while
--env "dev,test"works, which is the same bug wearing a different hat.--variableand--skip: consistent, but actively harmful —--variable "Note:a,b"would silently become two malformed variables.Recommendation: keep this. Four of the five (
--environment,--tenant-tag, and bothtarget flags) already accept CSV through their own legacy aliases, so this removes an
inconsistency rather than inventing new parsing.
--tenanthas no alias and is the one genuinelynew behaviour — included because splitting environments but not tenants would be arbitrary.
Happy to drop
--tenantif reviewers prefer strict "alias precedent only".--runbook-tagwas left out (no legacy alias, selects runbooks rather than deployment scope) —flagging it since it is shaped exactly like
--tenant-tagand could reasonably be included.2. Values that legitimately contain a comma.
A target/environment/tenant named
Web, Prod, or a tenant tag whose tag name contains a comma,can no longer be passed to these flags at all — there is no escape hatch. Options:
--environment/--tenant-tag/the target flags the aliases already behaved this way, so the regression surface is
--tenantplus users who were passing commas through the primary flag names.
--deployment-target 'Web\, Prod'), mirroring the escapingrelease create --packagealready does for colons. Costs a documented syntax users must learn.StringSliceinstead of a helper. Gets CSV-quoting for free(
--deployment-target '"Web, Prod",Other') viaencoding/csv, but the quoting is obscure,it changes
--helptype display fromstringArraytostrings, and it silently reinterpretsany existing value containing a quote character.
Recommendation: ship as-is, and add escaping later only if a real customer hits a comma in a
name. Worth a reviewer's call since it is technically a breaking change for such names.
🤖 Generated with Claude Code