Skip to content

fix: accept comma-separated values on deployment target and scope flags #692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
NickJosevski wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix: accept comma-separated values on deployment target and scope flags #692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d25b40c
fix: accept comma-separated values on deployment target and scope flags
NickJosevski Aug 19, 2026
8c07b92
refactor: collapse the duplicated comma-expansion block into one helper
NickJosevski Aug 31, 2026
c64ab93
fix: reject blank comma-separated values instead of silently dropping…
NickJosevski Aug 31, 2026
b77c149
fix: add a backslash escape hatch for commas in target and scope values
NickJosevski Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions pkg/cmd/release/deploy/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,18 +160,18 @@ func NewCmdDeploy(f factory.Factory) *cobra.Command {
flags := cmd.Flags()
flags.StringVarP(&deployFlags.Project.Value, deployFlags.Project.Name, "p", "", "Name or ID of the project to deploy the release from")
flags.StringVarP(&deployFlags.ReleaseVersion.Value, deployFlags.ReleaseVersion.Name, "", "", "Release version to deploy")
flags.StringArrayVarP(&deployFlags.Environments.Value, deployFlags.Environments.Name, "e", nil, "Deploy to this environment (can be specified multiple times)")
flags.StringArrayVarP(&deployFlags.Tenants.Value, deployFlags.Tenants.Name, "", nil, "Deploy to this tenant (can be specified multiple times)")
flags.StringArrayVarP(&deployFlags.TenantTags.Value, deployFlags.TenantTags.Name, "", nil, "Deploy to tenants matching this tag (can be specified multiple times). Format is 'Tag Set Name/Tag Name', such as 'Regions/South'.")
flags.StringArrayVarP(&deployFlags.Environments.Value, deployFlags.Environments.Name, "e", nil, "Deploy to this environment (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')")
flags.StringArrayVarP(&deployFlags.Tenants.Value, deployFlags.Tenants.Name, "", nil, "Deploy to this tenant (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')")
flags.StringArrayVarP(&deployFlags.TenantTags.Value, deployFlags.TenantTags.Name, "", nil, "Deploy to tenants matching this tag (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,'). Format is 'Tag Set Name/Tag Name', such as 'Regions/South'.")
flags.StringVarP(&deployFlags.DeployAt.Value, deployFlags.DeployAt.Name, "", "", "Deploy at a later time. Deploy now if omitted. TODO date formats and timezones!")
flags.StringVarP(&deployFlags.MaxQueueTime.Value, deployFlags.MaxQueueTime.Name, "", "", "Cancel the deployment if it hasn't started within this time period.")
flags.StringArrayVarP(&deployFlags.Variables.Value, deployFlags.Variables.Name, "v", nil, "Set the value for a prompted variable in the format Label:Value")
flags.BoolVarP(&deployFlags.UpdateVariables.Value, deployFlags.UpdateVariables.Name, "", false, "Overwrite the release variable snapshot by re-importing variables from the project.")
flags.StringArrayVarP(&deployFlags.ExcludedSteps.Value, deployFlags.ExcludedSteps.Name, "", nil, "Exclude specific steps from the deployment")
flags.StringVarP(&deployFlags.GuidedFailureMode.Value, deployFlags.GuidedFailureMode.Name, "", "", "Enable Guided failure mode (true/false/default)")
flags.BoolVarP(&deployFlags.ForcePackageDownload.Value, deployFlags.ForcePackageDownload.Name, "", false, "Force re-download of packages")
flags.StringArrayVarP(&deployFlags.DeploymentTargets.Value, deployFlags.DeploymentTargets.Name, "", nil, "Deploy to this target (can be specified multiple times)")
flags.StringArrayVarP(&deployFlags.ExcludeTargets.Value, deployFlags.ExcludeTargets.Name, "", nil, "Deploy to targets except for this (can be specified multiple times)")
flags.StringArrayVarP(&deployFlags.DeploymentTargets.Value, deployFlags.DeploymentTargets.Name, "", nil, "Deploy to this target (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')")
flags.StringArrayVarP(&deployFlags.ExcludeTargets.Value, deployFlags.ExcludeTargets.Name, "", nil, "Deploy to targets except for this (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')")
flags.StringArrayVarP(&deployFlags.DeploymentFreezeNames.Value, deployFlags.DeploymentFreezeNames.Name, "", nil, "Override this deployment freeze (can be specified multiple times)")
flags.StringVarP(&deployFlags.DeploymentFreezeOverrideReason.Value, deployFlags.DeploymentFreezeOverrideReason.Name, "", "", "Reason for overriding a deployment freeze")

Expand All @@ -198,6 +198,17 @@ func NewCmdDeploy(f factory.Factory) *cobra.Command {
}

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

@NickJosevski NickJosevski Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altitude: the expansion lives in two run functions rather than the flag layer. Two costs:

  1. 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 on release deploy but sends the literal string on tenant connect.
  2. Mutating flags.X.Value at the top of the run function creates an ordering dependency — any future code reading these flags in PreRunE or 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.

@NickJosevski NickJosevski Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both sub-points hold, but I've left the expansion where it is and want your call, because the obvious parse-time version silently undoes the blank-value fix from c64ab93.

Point 1 confirmed. tenant connect registers -e/--environment with StringArrayVarP (connect.go:108) and does no splitting, so -e "dev,test" is one environment name there and two on release deploy.

Point 2 is real but currently latent. The only PreRunE on either command is util.ApplyFlagAliases, and the expansion is the first statement of deployRun/runbookRun, so nothing reads unsplit values today. It is an invariant held by convention, which is your point.

Why I didn't move it. A splitting pflag.Value has to reject blanks in Set, and util.ApplyFlagAliases discards Set errors — pflagaliases.go:63 and :66 are both _ = primaryFlag.Value.Set(...). I checked both shapes with a throwaway test driving ApplyFlagAliases:

  • with a Value that errors on a blank component, --deployTo "," leaves the primary flag nil and no error escapes;
  • with the expansion where it is now, the same input arrives as []string{"", ""} and fails with --environment has a blank value; ....

So parse-time splitting reintroduces exactly the silent scope change from the blank-drop thread, on the legacy alias path (--deployTo, --env, --tag, --tenantTag, --target, --specificMachines, --exclude-target, --excludeMachines).

The version that doesn't regress is three parts:

  1. util.ApplyFlagAliases returns error. 7 call sites (buildinformation upload, package upload, release create, release deploy, release progression allow, release progression prevent, runbook run) — each is already inside a PreRunE that returns error, so it is one line each. Checked the other alias Set paths: string/stringArray Set never fail and bool aliases are fed from a bool flag's own String(), so the new splitting Value would be the only thing that can error.
  2. A registration helper next to AddFlagAliasesStringSlice wrapping a splitting/blank-rejecting pflag.Value (keeping Type() == "stringArray" so help output and completion don't change; nothing in the repo calls GetStringArray, so that's free).
  3. The split/escape logic moves out of executionscommon into pkg/util or a leaf package, because pkg/util can't import executionscommon.

I think (1) is worth doing on its own merits — swallowing Set errors during alias application is a latent bug independent of this PR. What I don't want to do unilaterally is (2)+(3) plus opting the other commands into comma splitting, under a PR scoped to the deploy/runbook scope flags — there are 11 StringArray --environment flags in pkg/cmd (these two, tenant connect, and the eight account ... create commands), and changing the rest is an observable behaviour change to commands #556 doesn't mention.

Decision I need: land #556 with the expansion in the two run functions and take (1)+(2)+(3) as a follow-up that also decides which other commands opt in — or do you want all of it here, and if so should the splitting Value go on every StringArray --environment or only on the deploy/runbook ones?

if err := executionscommon.ExpandCommaSeparatedFlags(
flags.Environments,
flags.Tenants,
flags.TenantTags,
flags.DeploymentTargets,
flags.ExcludeTargets,
); err != nil {
return err
}

outputFormat, err := cmd.Flags().GetString(constants.FlagOutputFormat)
if err != nil { // should never happen, but fallback if it does
outputFormat = constants.OutputFormatTable
Expand Down Expand Up @@ -255,15 +266,15 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error
resolvedFlags := NewDeployFlags()
resolvedFlags.Project.Value = options.ProjectName
resolvedFlags.ReleaseVersion.Value = options.ReleaseVersion
resolvedFlags.Environments.Value = options.Environments
resolvedFlags.Tenants.Value = options.Tenants
resolvedFlags.TenantTags.Value = options.TenantTags
resolvedFlags.Environments.Value = executionscommon.EscapeCommas(options.Environments)
resolvedFlags.Tenants.Value = executionscommon.EscapeCommas(options.Tenants)
resolvedFlags.TenantTags.Value = executionscommon.EscapeCommas(options.TenantTags)
resolvedFlags.DeployAt.Value = options.ScheduledStartTime
resolvedFlags.MaxQueueTime.Value = options.ScheduledExpiryTime
resolvedFlags.ExcludedSteps.Value = options.ExcludedSteps
resolvedFlags.GuidedFailureMode.Value = options.GuidedFailureMode
resolvedFlags.DeploymentTargets.Value = options.DeploymentTargets
resolvedFlags.ExcludeTargets.Value = options.ExcludeTargets
resolvedFlags.DeploymentTargets.Value = executionscommon.EscapeCommas(options.DeploymentTargets)
resolvedFlags.ExcludeTargets.Value = executionscommon.EscapeCommas(options.ExcludeTargets)
resolvedFlags.DeploymentFreezeNames.Value = options.DeploymentFreezeNames
resolvedFlags.DeploymentFreezeOverrideReason.Value = options.DeploymentFreezeOverrideReason

Expand Down
153 changes: 153 additions & 0 deletions pkg/cmd/release/deploy/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2006,6 +2006,159 @@ func TestDeployCreate_AutomationMode(t *testing.T) {
assert.Equal(t, "ServerTasks-29394\n", stdOut.String())
assert.Equal(t, "", stdErr.String())
}},

{"release deploy accepts comma-separated targets and environments; untenanted", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) {
cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) {
defer api.Close()
rootCmd.SetArgs([]string{
"release", "deploy",
"--project", fireProject.Name,
"--version", "1.0",
"--environment", "dev,test", // comma form
// mixed form; names containing spaces are preserved, whitespace around the comma is not
"--deployment-target", "first Machine, second Machine", "--deployment-target", "third Machine",
"--exclude-deployment-target", "fourthMachine,fifthMachine",
"--output-format", "basic", // not neccessary, just means we don't need the follow up HTTP requests at the end to print the web link
})
return rootCmd.ExecuteC()
})

api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject)

req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1")
requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body)
assert.Nil(t, err)

assert.Equal(t, deployments.CreateDeploymentUntenantedCommandV1{
ReleaseVersion: "1.0",
EnvironmentNames: []string{"dev", "test"},
CreateExecutionAbstractCommandV1: deployments.CreateExecutionAbstractCommandV1{
SpaceID: "Spaces-1",
ProjectIDOrName: fireProject.Name,
SpecificMachineNames: []string{"first Machine", "second Machine", "third Machine"},
ExcludedMachineNames: []string{"fourthMachine", "fifthMachine"},
},
}, requestBody)

req.RespondWith(&deployments.CreateDeploymentResponseV1{
DeploymentServerTasks: []*deployments.DeploymentServerTask{
{DeploymentID: "Deployments-203", ServerTaskID: "ServerTasks-29394"},
},
})

_, err = testutil.ReceivePair(cmdReceiver)
assert.Nil(t, err)

assert.Equal(t, "ServerTasks-29394\n", stdOut.String())
assert.Equal(t, "", stdErr.String())
}},

{"release deploy accepts comma-separated tenants and tenant tags; tenanted", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) {
cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) {
defer api.Close()
rootCmd.SetArgs([]string{
"release", "deploy",
"--project", fireProject.Name,
"--version", "1.0",
"--environment", "dev",
"--tenant", "Coke,Pepsi", // comma form
"--tenant-tag", "Region/us-east", "--tenant-tag", "Region/us-west,Region/eu", // mixed form
"--output-format", "basic",
})
return rootCmd.ExecuteC()
})

api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject)

req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1")
requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body)
assert.Nil(t, err)

assert.Equal(t, deployments.CreateDeploymentTenantedCommandV1{
ReleaseVersion: "1.0",
EnvironmentName: "dev",
Tenants: []string{"Coke", "Pepsi"},
TenantTags: []string{"Region/us-east", "Region/us-west", "Region/eu"},
CreateExecutionAbstractCommandV1: deployments.CreateExecutionAbstractCommandV1{
SpaceID: "Spaces-1",
ProjectIDOrName: fireProject.Name,
},
}, requestBody)

req.RespondWith(&deployments.CreateDeploymentResponseV1{
DeploymentServerTasks: []*deployments.DeploymentServerTask{
{DeploymentID: "Deployments-203", ServerTaskID: "ServerTasks-29394"},
},
})

_, err = testutil.ReceivePair(cmdReceiver)
assert.Nil(t, err)

assert.Equal(t, "ServerTasks-29394\n", stdOut.String())
assert.Equal(t, "", stdErr.String())
}},

{"release deploy treats a backslash-escaped comma as part of the value", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) {
cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) {
defer api.Close()
rootCmd.SetArgs([]string{
"release", "deploy",
"--project", fireProject.Name,
"--version", "1.0",
"--environment", "dev",
"--deployment-target", `Web\, Prod,Other`,
"--output-format", "basic",
})
return rootCmd.ExecuteC()
})

api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource)
api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject)

req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1")
requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body)
assert.Nil(t, err)

assert.Equal(t, []string{"Web, Prod", "Other"}, requestBody.SpecificMachineNames)

req.RespondWith(&deployments.CreateDeploymentResponseV1{
DeploymentServerTasks: []*deployments.DeploymentServerTask{
{DeploymentID: "Deployments-203", ServerTaskID: "ServerTasks-29394"},
},
})

_, err = testutil.ReceivePair(cmdReceiver)
assert.Nil(t, err)

assert.Equal(t, "ServerTasks-29394\n", stdOut.String())
assert.Equal(t, "", stdErr.String())
}},

// a --tenant that expands to nothing must not fall through to an untenanted deployment
{"release deploy rejects a blank comma-separated value rather than silently dropping it", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) {
cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) {
defer api.Close()
rootCmd.SetArgs([]string{
"release", "deploy",
"--project", fireProject.Name,
"--version", "1.0",
"--environment", "dev",
"--tenant", ",", // e.g. "$TENANT_A,$TENANT_B" where both are unset
"--output-format", "basic",
})
return rootCmd.ExecuteC()
})

_, err := testutil.ReceivePair(cmdReceiver)
assert.ErrorContains(t, err, "--tenant has a blank value")

assert.Equal(t, "", stdOut.String())
}},
}

for _, test := range tests {
Expand Down
Loading