From 266fe8c2a04007af568ac1012c52d390ab8458a9 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:42:06 +1000 Subject: [PATCH 1/4] fix: accept comma-separated values on deployment target and scope flags `--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) --- pkg/cmd/release/deploy/deploy.go | 17 +++- pkg/cmd/release/deploy/deploy_test.go | 95 +++++++++++++++++++ pkg/cmd/runbook/run/run.go | 17 +++- pkg/cmd/runbook/run/run_test.go | 48 ++++++++++ pkg/executionscommon/executionscommon.go | 23 +++++ pkg/executionscommon/executionscommon_test.go | 30 ++++++ 6 files changed, 220 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 835c523a..d598ff51 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -171,9 +171,9 @@ 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)") + flags.StringArrayVarP(&deployFlags.Tenants.Value, deployFlags.Tenants.Name, "", nil, "Deploy to this tenant (can be specified multiple times, or as a comma-separated list)") + 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). 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") @@ -182,8 +182,8 @@ func NewCmdDeploy(f factory.Factory) *cobra.Command { flags.StringVarP(&deployFlags.GuidedFailureMode.Value, deployFlags.GuidedFailureMode.Name, "", "", "Enable Guided failure mode (true/false/default)") flags.StringVarP(&deployFlags.Priority.Value, deployFlags.Priority.Name, "", "", "Jump the task queue ahead of other queued tasks (true/false/default). Requires the Priority Tasks feature, and the TaskPrioritize permission to set true.") 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)") + 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)") flags.StringArrayVarP(&deployFlags.SpecificTargetTagNames.Value, deployFlags.SpecificTargetTagNames.Name, "", nil, "Deploy to targets matching this tag (can be specified multiple times)") flags.StringArrayVarP(&deployFlags.ExcludedTargetTagNames.Value, deployFlags.ExcludedTargetTagNames.Name, "", nil, "Deploy to targets except for those matching this tag (can be specified multiple times)") flags.StringArrayVarP(&deployFlags.DeploymentFreezeNames.Value, deployFlags.DeploymentFreezeNames.Name, "", nil, "Override this deployment freeze (can be specified multiple times)") @@ -212,6 +212,13 @@ 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 + flags.Environments.Value = executionscommon.ExpandCommaSeparated(flags.Environments.Value) + flags.Tenants.Value = executionscommon.ExpandCommaSeparated(flags.Tenants.Value) + flags.TenantTags.Value = executionscommon.ExpandCommaSeparated(flags.TenantTags.Value) + flags.DeploymentTargets.Value = executionscommon.ExpandCommaSeparated(flags.DeploymentTargets.Value) + flags.ExcludeTargets.Value = executionscommon.ExpandCommaSeparated(flags.ExcludeTargets.Value) + outputFormat, err := cmd.Flags().GetString(constants.FlagOutputFormat) if err != nil { // should never happen, but fallback if it does outputFormat = constants.OutputFormatTable diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 24d8d238..1de4591d 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -2293,6 +2293,101 @@ 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()) + }}, } for _, test := range tests { diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index b0a5fa40..75d290bc 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -173,9 +173,9 @@ func NewCmdRun(f factory.Factory) *cobra.Command { flags.StringVarP(&runFlags.Project.Value, runFlags.Project.Name, "p", "", "Name or ID of the project to run the runbook from") flags.StringVarP(&runFlags.RunbookName.Value, runFlags.RunbookName.Name, "n", "", "Name of the runbook to run") flags.StringArrayVarP(&runFlags.RunbookTags.Value, runFlags.RunbookTags.Name, "", nil, "Run all runbooks matching this tag (can be specified multiple times). Format is 'Tag Set Name/Tag Name'. Mutually exclusive with --name.") - flags.StringArrayVarP(&runFlags.Environments.Value, runFlags.Environments.Name, "e", nil, "Run in this environment (can be specified multiple times)") - flags.StringArrayVarP(&runFlags.Tenants.Value, runFlags.Tenants.Name, "", nil, "Run for this tenant (can be specified multiple times)") - flags.StringArrayVarP(&runFlags.TenantTags.Value, runFlags.TenantTags.Name, "", nil, "Run for tenants matching this tag (can be specified multiple times). Format is 'Tag Set Name/Tag Name', such as 'Regions/South'.") + flags.StringArrayVarP(&runFlags.Environments.Value, runFlags.Environments.Name, "e", nil, "Run in this environment (can be specified multiple times, or as a comma-separated list)") + flags.StringArrayVarP(&runFlags.Tenants.Value, runFlags.Tenants.Name, "", nil, "Run for this tenant (can be specified multiple times, or as a comma-separated list)") + flags.StringArrayVarP(&runFlags.TenantTags.Value, runFlags.TenantTags.Name, "", nil, "Run for tenants matching this tag (can be specified multiple times, or as a comma-separated list). Format is 'Tag Set Name/Tag Name', such as 'Regions/South'.") flags.StringVarP(&runFlags.RunAt.Value, runFlags.RunAt.Name, "", "", "Run at a later time. Run now if omitted. TODO date formats and timezones!") flags.StringVarP(&runFlags.MaxQueueTime.Value, runFlags.MaxQueueTime.Name, "", "", "Cancel a scheduled run if it hasn't started within this time period.") flags.StringArrayVarP(&runFlags.Variables.Value, runFlags.Variables.Name, "v", nil, "Set the value for a prompted variable in the format Label:Value") @@ -184,8 +184,8 @@ func NewCmdRun(f factory.Factory) *cobra.Command { flags.StringVarP(&runFlags.GuidedFailureMode.Value, runFlags.GuidedFailureMode.Name, "", "", "Enable Guided failure mode (true/false/default)") flags.StringVarP(&runFlags.Priority.Value, runFlags.Priority.Name, "", "", "Jump the task queue ahead of other queued tasks (true/false/default). Requires the Priority Tasks feature. For runbook runs, 'default' is the same as 'false'.") flags.BoolVarP(&runFlags.ForcePackageDownload.Value, runFlags.ForcePackageDownload.Name, "", false, "Force re-download of packages") - flags.StringArrayVarP(&runFlags.RunTargets.Value, runFlags.RunTargets.Name, "", nil, "Run on this target (can be specified multiple times)") - flags.StringArrayVarP(&runFlags.ExcludeTargets.Value, runFlags.ExcludeTargets.Name, "", nil, "Run on targets except for this (can be specified multiple times)") + flags.StringArrayVarP(&runFlags.RunTargets.Value, runFlags.RunTargets.Name, "", nil, "Run on this target (can be specified multiple times, or as a comma-separated list)") + flags.StringArrayVarP(&runFlags.ExcludeTargets.Value, runFlags.ExcludeTargets.Name, "", nil, "Run on targets except for this (can be specified multiple times, or as a comma-separated list)") flags.StringArrayVarP(&runFlags.SpecificTargetTagNames.Value, runFlags.SpecificTargetTagNames.Name, "", nil, "Run on targets matching this tag (can be specified multiple times)") flags.StringArrayVarP(&runFlags.ExcludedTargetTagNames.Value, runFlags.ExcludedTargetTagNames.Name, "", nil, "Run on targets except for those matching this tag (can be specified multiple times)") flags.StringVarP(&runFlags.GitRef.Value, runFlags.GitRef.Name, "", "", "Git Reference e.g. refs/heads/main. Only relevant for config-as-code projects where runbooks are stored in Git.") @@ -215,6 +215,13 @@ func NewCmdRun(f factory.Factory) *cobra.Command { } 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) + flags.Tenants.Value = executionscommon.ExpandCommaSeparated(flags.Tenants.Value) + flags.TenantTags.Value = executionscommon.ExpandCommaSeparated(flags.TenantTags.Value) + flags.RunTargets.Value = executionscommon.ExpandCommaSeparated(flags.RunTargets.Value) + flags.ExcludeTargets.Value = executionscommon.ExpandCommaSeparated(flags.ExcludeTargets.Value) + if flags.RunbookName.Value != "" && len(flags.RunbookTags.Value) > 0 { return errors.New("--name and --runbook-tag are mutually exclusive. Please specify either a runbook name or runbook tags, not both") } diff --git a/pkg/cmd/runbook/run/run_test.go b/pkg/cmd/runbook/run/run_test.go index a3d50e83..e69b087c 100644 --- a/pkg/cmd/runbook/run/run_test.go +++ b/pkg/cmd/runbook/run/run_test.go @@ -344,6 +344,54 @@ func TestRunbookRun_AutomationMode(t *testing.T) { assert.Contains(t, stdOut.String(), "ServerTasks-29394\n") assert.Equal(t, "", stdErr.String()) }}, + + {"runbook run accepts comma-separated environments and targets", 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{ + "runbook", "run", + "--project", "Fire Project", + "--runbook", "Provision Database", + "--environment", "dev,test", // comma form + // mixed form; names containing spaces are preserved, whitespace around the comma is not + "--run-target", "first Machine, second Machine", "--run-target", "third Machine", + "--exclude-run-target", "fourthMachine,fifthMachine", + "--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/Fire Project").RespondWithJSON(fixtures.AsServerResponse(fireProject)) + + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") + requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) + assert.Nil(t, err) + + assert.Equal(t, runbooks.RunbookRunCommandV1{ + RunbookName: "Provision Database", + 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(&runbooks.RunbookRunResponseV1{ + RunbookRunServerTasks: []*runbooks.RunbookRunServerTask{ + {RunbookRunID: "RunbookRun-203", ServerTaskID: "ServerTasks-29394"}, + }, + }) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + + assert.Contains(t, stdOut.String(), "ServerTasks-29394\n") + assert.Equal(t, "", stdErr.String()) + }}, } for _, test := range tests { diff --git a/pkg/executionscommon/executionscommon.go b/pkg/executionscommon/executionscommon.go index 91af36fa..405187e2 100644 --- a/pkg/executionscommon/executionscommon.go +++ b/pkg/executionscommon/executionscommon.go @@ -301,6 +301,29 @@ func AskVariableSpecificPrompt(asker question.Asker, message string, variableTyp } } +// ExpandCommaSeparated splits each entry on commas so `--flag "A,B"` behaves the same as +// `--flag A --flag B`. Whitespace around each entry is trimmed and blank entries are dropped. +// Only apply this to flags whose values cannot legitimately contain a comma; notably NOT to +// --variable, --skip or the package/git-resource specs. +func ExpandCommaSeparated(values []string) []string { + if len(values) == 0 { + return values + } + result := make([]string, 0, len(values)) + for _, value := range values { + for _, component := range strings.Split(value, ",") { + component = strings.TrimSpace(component) + if component != "" { + result = append(result, component) + } + } + } + if len(result) == 0 { + return nil + } + return result +} + func ParseVariableStringArray(variables []string) (map[string]string, error) { result := make(map[string]string, len(variables)) for _, v := range variables { diff --git a/pkg/executionscommon/executionscommon_test.go b/pkg/executionscommon/executionscommon_test.go index 72604be2..26db6a16 100644 --- a/pkg/executionscommon/executionscommon_test.go +++ b/pkg/executionscommon/executionscommon_test.go @@ -412,3 +412,33 @@ func TestToVariableStringArray(t *testing.T) { }) } } + +func TestExpandCommaSeparated(t *testing.T) { + tests := []struct { + name string + input []string + expect []string + }{ + {name: "nil stays nil", input: nil, expect: nil}, + {name: "single value", input: []string{"ABC"}, expect: []string{"ABC"}}, + + {name: "comma form", input: []string{"ABC,XYZ"}, expect: []string{"ABC", "XYZ"}}, + {name: "repeated form", input: []string{"ABC", "XYZ"}, expect: []string{"ABC", "XYZ"}}, + {name: "mixed form", input: []string{"ABC,XYZ", "DEF"}, expect: []string{"ABC", "XYZ", "DEF"}}, + + {name: "preserves spaces within values", input: []string{"Web Server 01,Web Server 02"}, expect: []string{"Web Server 01", "Web Server 02"}}, + {name: "trims spaces around values", input: []string{" ABC ,\tXYZ "}, expect: []string{"ABC", "XYZ"}}, + + {name: "preserves order and duplicates", input: []string{"ABC,ABC"}, expect: []string{"ABC", "ABC"}}, + {name: "tenant tags", input: []string{"Regions/us-east,Regions/us-west"}, expect: []string{"Regions/us-east", "Regions/us-west"}}, + + {name: "drops blank entries", input: []string{"ABC,,XYZ"}, expect: []string{"ABC", "XYZ"}}, + {name: "all blank entries returns nil", input: []string{"", " , "}, expect: nil}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expect, executionscommon.ExpandCommaSeparated(test.input)) + }) + } +} From cf471125bae94c8844558ec53e9a4afce4033c4e Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:10:17 +1000 Subject: [PATCH 2/4] refactor: collapse the duplicated comma-expansion block into one helper 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) --- pkg/cmd/release/deploy/deploy.go | 12 +++++++----- pkg/cmd/runbook/run/run.go | 12 +++++++----- pkg/executionscommon/executionscommon.go | 9 +++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index d598ff51..6240b551 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -213,11 +213,13 @@ 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 - flags.Environments.Value = executionscommon.ExpandCommaSeparated(flags.Environments.Value) - flags.Tenants.Value = executionscommon.ExpandCommaSeparated(flags.Tenants.Value) - flags.TenantTags.Value = executionscommon.ExpandCommaSeparated(flags.TenantTags.Value) - flags.DeploymentTargets.Value = executionscommon.ExpandCommaSeparated(flags.DeploymentTargets.Value) - flags.ExcludeTargets.Value = executionscommon.ExpandCommaSeparated(flags.ExcludeTargets.Value) + executionscommon.ExpandCommaSeparatedFlags( + flags.Environments, + flags.Tenants, + flags.TenantTags, + flags.DeploymentTargets, + flags.ExcludeTargets, + ) outputFormat, err := cmd.Flags().GetString(constants.FlagOutputFormat) if err != nil { // should never happen, but fallback if it does diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index 75d290bc..8e6c01ad 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -216,11 +216,13 @@ func NewCmdRun(f factory.Factory) *cobra.Command { 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) - flags.Tenants.Value = executionscommon.ExpandCommaSeparated(flags.Tenants.Value) - flags.TenantTags.Value = executionscommon.ExpandCommaSeparated(flags.TenantTags.Value) - flags.RunTargets.Value = executionscommon.ExpandCommaSeparated(flags.RunTargets.Value) - flags.ExcludeTargets.Value = executionscommon.ExpandCommaSeparated(flags.ExcludeTargets.Value) + executionscommon.ExpandCommaSeparatedFlags( + flags.Environments, + flags.Tenants, + flags.TenantTags, + flags.RunTargets, + flags.ExcludeTargets, + ) if flags.RunbookName.Value != "" && len(flags.RunbookTags.Value) > 0 { return errors.New("--name and --runbook-tag are mutually exclusive. Please specify either a runbook name or runbook tags, not both") diff --git a/pkg/executionscommon/executionscommon.go b/pkg/executionscommon/executionscommon.go index 405187e2..530bcf54 100644 --- a/pkg/executionscommon/executionscommon.go +++ b/pkg/executionscommon/executionscommon.go @@ -10,6 +10,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/question" "github.com/OctopusDeploy/cli/pkg/surveyext" "github.com/OctopusDeploy/cli/pkg/util" + "github.com/OctopusDeploy/cli/pkg/util/flag" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" @@ -324,6 +325,14 @@ func ExpandCommaSeparated(values []string) []string { return result } +// ExpandCommaSeparatedFlags applies ExpandCommaSeparated in place to each of the given flags, +// so callers don't have to keep a hand-maintained list of assignments in sync. +func ExpandCommaSeparatedFlags(flags ...*flag.Flag[[]string]) { + for _, f := range flags { + f.Value = ExpandCommaSeparated(f.Value) + } +} + func ParseVariableStringArray(variables []string) (map[string]string, error) { result := make(map[string]string, len(variables)) for _, v := range variables { From 3174a9a10326fcac8fe4ebd7495d58335312041f Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:11:25 +1000 Subject: [PATCH 3/4] fix: reject blank comma-separated values instead of silently dropping 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) --- pkg/cmd/release/deploy/deploy.go | 6 ++- pkg/cmd/release/deploy/deploy_test.go | 21 +++++++++ pkg/cmd/runbook/run/run.go | 6 ++- pkg/executionscommon/executionscommon.go | 31 ++++++++----- pkg/executionscommon/executionscommon_test.go | 46 +++++++++++++++++-- 5 files changed, 92 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 6240b551..940278f2 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -213,13 +213,15 @@ 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 - executionscommon.ExpandCommaSeparatedFlags( + 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 diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 1de4591d..bbe38eb4 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -2388,6 +2388,27 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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 { diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index 8e6c01ad..662010ac 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -216,13 +216,15 @@ func NewCmdRun(f factory.Factory) *cobra.Command { 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 - executionscommon.ExpandCommaSeparatedFlags( + if err := executionscommon.ExpandCommaSeparatedFlags( flags.Environments, flags.Tenants, flags.TenantTags, flags.RunTargets, flags.ExcludeTargets, - ) + ); err != nil { + return err + } if flags.RunbookName.Value != "" && len(flags.RunbookTags.Value) > 0 { return errors.New("--name and --runbook-tag are mutually exclusive. Please specify either a runbook name or runbook tags, not both") diff --git a/pkg/executionscommon/executionscommon.go b/pkg/executionscommon/executionscommon.go index 530bcf54..1eed473c 100644 --- a/pkg/executionscommon/executionscommon.go +++ b/pkg/executionscommon/executionscommon.go @@ -303,34 +303,43 @@ func AskVariableSpecificPrompt(asker question.Asker, message string, variableTyp } // ExpandCommaSeparated splits each entry on commas so `--flag "A,B"` behaves the same as -// `--flag A --flag B`. Whitespace around each entry is trimmed and blank entries are dropped. +// `--flag A --flag B`. Whitespace around each entry is trimmed. +// +// Blank entries are rejected rather than silently dropped. A value such as "," or "A,,B" +// almost always means a caller-side variable substitution produced nothing, and quietly +// dropping it would change the scope of the deployment: an empty --tenant list, for example, +// turns a tenanted deployment into an untenanted one rather than failing. +// // Only apply this to flags whose values cannot legitimately contain a comma; notably NOT to // --variable, --skip or the package/git-resource specs. -func ExpandCommaSeparated(values []string) []string { +func ExpandCommaSeparated(flagName string, values []string) ([]string, error) { if len(values) == 0 { - return values + return values, nil } result := make([]string, 0, len(values)) for _, value := range values { for _, component := range strings.Split(value, ",") { component = strings.TrimSpace(component) - if component != "" { - result = append(result, component) + if component == "" { + return nil, fmt.Errorf("--%s has a blank value; check for an empty variable or a stray comma in %q", flagName, value) } + result = append(result, component) } } - if len(result) == 0 { - return nil - } - return result + return result, nil } // ExpandCommaSeparatedFlags applies ExpandCommaSeparated in place to each of the given flags, // so callers don't have to keep a hand-maintained list of assignments in sync. -func ExpandCommaSeparatedFlags(flags ...*flag.Flag[[]string]) { +func ExpandCommaSeparatedFlags(flags ...*flag.Flag[[]string]) error { for _, f := range flags { - f.Value = ExpandCommaSeparated(f.Value) + expanded, err := ExpandCommaSeparated(f.Name, f.Value) + if err != nil { + return err + } + f.Value = expanded } + return nil } func ParseVariableStringArray(variables []string) (map[string]string, error) { diff --git a/pkg/executionscommon/executionscommon_test.go b/pkg/executionscommon/executionscommon_test.go index 26db6a16..0942b9c4 100644 --- a/pkg/executionscommon/executionscommon_test.go +++ b/pkg/executionscommon/executionscommon_test.go @@ -8,6 +8,7 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/OctopusDeploy/cli/pkg/executionscommon" + "github.com/OctopusDeploy/cli/pkg/util/flag" "github.com/OctopusDeploy/cli/test/fixtures" "github.com/OctopusDeploy/cli/test/testutil" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" @@ -431,14 +432,53 @@ func TestExpandCommaSeparated(t *testing.T) { {name: "preserves order and duplicates", input: []string{"ABC,ABC"}, expect: []string{"ABC", "ABC"}}, {name: "tenant tags", input: []string{"Regions/us-east,Regions/us-west"}, expect: []string{"Regions/us-east", "Regions/us-west"}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := executionscommon.ExpandCommaSeparated("environment", test.input) + assert.NoError(t, err) + assert.Equal(t, test.expect, result) + }) + } +} - {name: "drops blank entries", input: []string{"ABC,,XYZ"}, expect: []string{"ABC", "XYZ"}}, - {name: "all blank entries returns nil", input: []string{"", " , "}, expect: nil}, +// a blank component almost always means a caller-side variable expanded to nothing; dropping it +// silently would narrow the scope of a deployment, or flip a tenanted deploy to untenanted +func TestExpandCommaSeparated_RejectsBlankValues(t *testing.T) { + tests := []struct { + name string + input []string + }{ + {name: "empty string", input: []string{""}}, + {name: "lone comma", input: []string{","}}, + {name: "whitespace only", input: []string{" , "}}, + {name: "blank in the middle", input: []string{"ABC,,XYZ"}}, + {name: "trailing comma", input: []string{"ABC,"}}, + {name: "blank alongside a good repeat", input: []string{"ABC", ""}}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expect, executionscommon.ExpandCommaSeparated(test.input)) + result, err := executionscommon.ExpandCommaSeparated("tenant", test.input) + assert.Nil(t, result) + assert.ErrorContains(t, err, "--tenant has a blank value") }) } } + +func TestExpandCommaSeparatedFlags(t *testing.T) { + environments := flag.New[[]string]("environment", false) + environments.Value = []string{"dev,test"} + tenants := flag.New[[]string]("tenant", false) + tenants.Value = []string{"Tenant A", "Tenant B,Tenant C"} + + assert.NoError(t, executionscommon.ExpandCommaSeparatedFlags(environments, tenants)) + assert.Equal(t, []string{"dev", "test"}, environments.Value) + assert.Equal(t, []string{"Tenant A", "Tenant B", "Tenant C"}, tenants.Value) + + bad := flag.New[[]string]("deployment-target", false) + bad.Value = []string{"ABC,"} + err := executionscommon.ExpandCommaSeparatedFlags(environments, bad) + assert.ErrorContains(t, err, "--deployment-target has a blank value") +} From 788222a52c020eb7711298c77e113c3584e4ea2f Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:12:06 +1000 Subject: [PATCH 4/4] fix: add a backslash escape hatch for commas in target and scope values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- pkg/cmd/release/deploy/deploy.go | 20 ++++---- pkg/cmd/release/deploy/deploy_test.go | 37 +++++++++++++++ pkg/cmd/runbook/run/run.go | 30 ++++++------ pkg/cmd/runbook/run/run_by_tag.go | 6 +-- pkg/executionscommon/executionscommon.go | 46 +++++++++++++++++-- pkg/executionscommon/executionscommon_test.go | 21 ++++++++- 6 files changed, 127 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 940278f2..d83d9918 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -171,9 +171,9 @@ 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, or as a comma-separated list)") - flags.StringArrayVarP(&deployFlags.Tenants.Value, deployFlags.Tenants.Name, "", nil, "Deploy to this tenant (can be specified multiple times, or as a comma-separated list)") - 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). 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") @@ -182,8 +182,8 @@ func NewCmdDeploy(f factory.Factory) *cobra.Command { flags.StringVarP(&deployFlags.GuidedFailureMode.Value, deployFlags.GuidedFailureMode.Name, "", "", "Enable Guided failure mode (true/false/default)") flags.StringVarP(&deployFlags.Priority.Value, deployFlags.Priority.Name, "", "", "Jump the task queue ahead of other queued tasks (true/false/default). Requires the Priority Tasks feature, and the TaskPrioritize permission to set true.") 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, or as a comma-separated list)") - 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)") + 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.SpecificTargetTagNames.Value, deployFlags.SpecificTargetTagNames.Name, "", nil, "Deploy to targets matching this tag (can be specified multiple times)") flags.StringArrayVarP(&deployFlags.ExcludedTargetTagNames.Value, deployFlags.ExcludedTargetTagNames.Name, "", nil, "Deploy to targets except for those matching this tag (can be specified multiple times)") flags.StringArrayVarP(&deployFlags.DeploymentFreezeNames.Value, deployFlags.DeploymentFreezeNames.Name, "", nil, "Override this deployment freeze (can be specified multiple times)") @@ -287,16 +287,16 @@ 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 = executionscommon.EscapeCommas(options.DeploymentTargets) + resolvedFlags.ExcludeTargets.Value = executionscommon.EscapeCommas(options.ExcludeTargets) resolvedFlags.Priority.Value = options.Priority - resolvedFlags.DeploymentTargets.Value = options.DeploymentTargets - resolvedFlags.ExcludeTargets.Value = options.ExcludeTargets resolvedFlags.SpecificTargetTagNames.Value = options.SpecificTargetTagNames resolvedFlags.ExcludedTargetTagNames.Value = options.ExcludedTargetTagNames resolvedFlags.DeploymentFreezeNames.Value = options.DeploymentFreezeNames diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index bbe38eb4..3a0ff56e 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -2389,6 +2389,43 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) { diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index 662010ac..06cab41b 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -173,9 +173,9 @@ func NewCmdRun(f factory.Factory) *cobra.Command { flags.StringVarP(&runFlags.Project.Value, runFlags.Project.Name, "p", "", "Name or ID of the project to run the runbook from") flags.StringVarP(&runFlags.RunbookName.Value, runFlags.RunbookName.Name, "n", "", "Name of the runbook to run") flags.StringArrayVarP(&runFlags.RunbookTags.Value, runFlags.RunbookTags.Name, "", nil, "Run all runbooks matching this tag (can be specified multiple times). Format is 'Tag Set Name/Tag Name'. Mutually exclusive with --name.") - flags.StringArrayVarP(&runFlags.Environments.Value, runFlags.Environments.Name, "e", nil, "Run in this environment (can be specified multiple times, or as a comma-separated list)") - flags.StringArrayVarP(&runFlags.Tenants.Value, runFlags.Tenants.Name, "", nil, "Run for this tenant (can be specified multiple times, or as a comma-separated list)") - flags.StringArrayVarP(&runFlags.TenantTags.Value, runFlags.TenantTags.Name, "", nil, "Run for tenants matching this tag (can be specified multiple times, or as a comma-separated list). Format is 'Tag Set Name/Tag Name', such as 'Regions/South'.") + flags.StringArrayVarP(&runFlags.Environments.Value, runFlags.Environments.Name, "e", nil, "Run in this environment (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')") + flags.StringArrayVarP(&runFlags.Tenants.Value, runFlags.Tenants.Name, "", nil, "Run for this tenant (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')") + flags.StringArrayVarP(&runFlags.TenantTags.Value, runFlags.TenantTags.Name, "", nil, "Run for 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(&runFlags.RunAt.Value, runFlags.RunAt.Name, "", "", "Run at a later time. Run now if omitted. TODO date formats and timezones!") flags.StringVarP(&runFlags.MaxQueueTime.Value, runFlags.MaxQueueTime.Name, "", "", "Cancel a scheduled run if it hasn't started within this time period.") flags.StringArrayVarP(&runFlags.Variables.Value, runFlags.Variables.Name, "v", nil, "Set the value for a prompted variable in the format Label:Value") @@ -184,8 +184,8 @@ func NewCmdRun(f factory.Factory) *cobra.Command { flags.StringVarP(&runFlags.GuidedFailureMode.Value, runFlags.GuidedFailureMode.Name, "", "", "Enable Guided failure mode (true/false/default)") flags.StringVarP(&runFlags.Priority.Value, runFlags.Priority.Name, "", "", "Jump the task queue ahead of other queued tasks (true/false/default). Requires the Priority Tasks feature. For runbook runs, 'default' is the same as 'false'.") flags.BoolVarP(&runFlags.ForcePackageDownload.Value, runFlags.ForcePackageDownload.Name, "", false, "Force re-download of packages") - flags.StringArrayVarP(&runFlags.RunTargets.Value, runFlags.RunTargets.Name, "", nil, "Run on this target (can be specified multiple times, or as a comma-separated list)") - flags.StringArrayVarP(&runFlags.ExcludeTargets.Value, runFlags.ExcludeTargets.Name, "", nil, "Run on targets except for this (can be specified multiple times, or as a comma-separated list)") + flags.StringArrayVarP(&runFlags.RunTargets.Value, runFlags.RunTargets.Name, "", nil, "Run on this target (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')") + flags.StringArrayVarP(&runFlags.ExcludeTargets.Value, runFlags.ExcludeTargets.Name, "", nil, "Run on targets except for this (can be specified multiple times, or as a comma-separated list; escape a comma inside a value as '\\,')") flags.StringArrayVarP(&runFlags.SpecificTargetTagNames.Value, runFlags.SpecificTargetTagNames.Name, "", nil, "Run on targets matching this tag (can be specified multiple times)") flags.StringArrayVarP(&runFlags.ExcludedTargetTagNames.Value, runFlags.ExcludedTargetTagNames.Name, "", nil, "Run on targets except for those matching this tag (can be specified multiple times)") flags.StringVarP(&runFlags.GitRef.Value, runFlags.GitRef.Name, "", "", "Git Reference e.g. refs/heads/main. Only relevant for config-as-code projects where runbooks are stored in Git.") @@ -355,16 +355,16 @@ func runDbRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopu resolvedFlags := NewRunFlags() resolvedFlags.Project.Value = options.ProjectName resolvedFlags.RunbookName.Value = options.RunbookName - 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.RunAt.Value = options.ScheduledStartTime resolvedFlags.MaxQueueTime.Value = options.ScheduledExpiryTime resolvedFlags.ExcludedSteps.Value = options.ExcludedSteps resolvedFlags.GuidedFailureMode.Value = options.GuidedFailureMode + resolvedFlags.RunTargets.Value = executionscommon.EscapeCommas(options.RunTargets) + resolvedFlags.ExcludeTargets.Value = executionscommon.EscapeCommas(options.ExcludeTargets) resolvedFlags.Priority.Value = options.Priority - resolvedFlags.RunTargets.Value = options.RunTargets - resolvedFlags.ExcludeTargets.Value = options.ExcludeTargets resolvedFlags.SpecificTargetTagNames.Value = options.SpecificTargetTagNames resolvedFlags.ExcludedTargetTagNames.Value = options.ExcludedTargetTagNames @@ -498,16 +498,16 @@ func runGitRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octop resolvedFlags := NewRunFlags() resolvedFlags.Project.Value = options.ProjectName resolvedFlags.RunbookName.Value = options.RunbookName - 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.RunAt.Value = options.ScheduledStartTime resolvedFlags.MaxQueueTime.Value = options.ScheduledExpiryTime resolvedFlags.ExcludedSteps.Value = options.ExcludedSteps resolvedFlags.GuidedFailureMode.Value = options.GuidedFailureMode + resolvedFlags.RunTargets.Value = executionscommon.EscapeCommas(options.RunTargets) + resolvedFlags.ExcludeTargets.Value = executionscommon.EscapeCommas(options.ExcludeTargets) resolvedFlags.Priority.Value = options.Priority - resolvedFlags.RunTargets.Value = options.RunTargets - resolvedFlags.ExcludeTargets.Value = options.ExcludeTargets resolvedFlags.SpecificTargetTagNames.Value = options.SpecificTargetTagNames resolvedFlags.ExcludedTargetTagNames.Value = options.ExcludedTargetTagNames resolvedFlags.GitRef.Value = options.GitReference diff --git a/pkg/cmd/runbook/run/run_by_tag.go b/pkg/cmd/runbook/run/run_by_tag.go index d281bff5..71b784ef 100644 --- a/pkg/cmd/runbook/run/run_by_tag.go +++ b/pkg/cmd/runbook/run/run_by_tag.go @@ -321,9 +321,9 @@ func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, oc resolvedFlags := NewRunFlags() resolvedFlags.Project.Value = flags.Project.Value resolvedFlags.RunbookTags.Value = flags.RunbookTags.Value - resolvedFlags.Environments.Value = flags.Environments.Value - resolvedFlags.Tenants.Value = flags.Tenants.Value - resolvedFlags.TenantTags.Value = flags.TenantTags.Value + resolvedFlags.Environments.Value = executionscommon.EscapeCommas(flags.Environments.Value) + resolvedFlags.Tenants.Value = executionscommon.EscapeCommas(flags.Tenants.Value) + resolvedFlags.TenantTags.Value = executionscommon.EscapeCommas(flags.TenantTags.Value) spaceName := "" if s := f.GetCurrentSpace(); s != nil { diff --git a/pkg/executionscommon/executionscommon.go b/pkg/executionscommon/executionscommon.go index 1eed473c..026d3d2d 100644 --- a/pkg/executionscommon/executionscommon.go +++ b/pkg/executionscommon/executionscommon.go @@ -305,23 +305,27 @@ func AskVariableSpecificPrompt(asker question.Asker, message string, variableTyp // ExpandCommaSeparated splits each entry on commas so `--flag "A,B"` behaves the same as // `--flag A --flag B`. Whitespace around each entry is trimmed. // +// A comma that is part of a value can be escaped with a backslash, so +// `--deployment-target 'Web\, Prod'` yields the single value `Web, Prod`. A backslash in any +// other position is left alone, so target names such as `DOMAIN\host` are unaffected. +// // Blank entries are rejected rather than silently dropped. A value such as "," or "A,,B" // almost always means a caller-side variable substitution produced nothing, and quietly // dropping it would change the scope of the deployment: an empty --tenant list, for example, // turns a tenanted deployment into an untenanted one rather than failing. // -// Only apply this to flags whose values cannot legitimately contain a comma; notably NOT to -// --variable, --skip or the package/git-resource specs. +// Only apply this to flags whose values cannot legitimately contain an unescaped comma; +// notably NOT to --variable, --skip or the package/git-resource specs. func ExpandCommaSeparated(flagName string, values []string) ([]string, error) { if len(values) == 0 { return values, nil } result := make([]string, 0, len(values)) for _, value := range values { - for _, component := range strings.Split(value, ",") { + for _, component := range splitOnUnescapedCommas(value) { component = strings.TrimSpace(component) if component == "" { - return nil, fmt.Errorf("--%s has a blank value; check for an empty variable or a stray comma in %q", flagName, value) + return nil, fmt.Errorf("--%s has a blank value; check for an empty variable or a stray comma in %q. Use '\\,' to include a comma in a value", flagName, value) } result = append(result, component) } @@ -329,6 +333,26 @@ func ExpandCommaSeparated(flagName string, values []string) ([]string, error) { return result, nil } +// splitOnUnescapedCommas splits on commas, treating `\,` as an escaped literal comma. +// Any other backslash is preserved verbatim. +func splitOnUnescapedCommas(value string) []string { + var result []string + var current strings.Builder + for i := 0; i < len(value); i++ { + switch { + case value[i] == '\\' && i+1 < len(value) && value[i+1] == ',': + current.WriteByte(',') + i++ + case value[i] == ',': + result = append(result, current.String()) + current.Reset() + default: + current.WriteByte(value[i]) + } + } + return append(result, current.String()) +} + // ExpandCommaSeparatedFlags applies ExpandCommaSeparated in place to each of the given flags, // so callers don't have to keep a hand-maintained list of assignments in sync. func ExpandCommaSeparatedFlags(flags ...*flag.Flag[[]string]) error { @@ -342,6 +366,20 @@ func ExpandCommaSeparatedFlags(flags ...*flag.Flag[[]string]) error { return nil } +// EscapeCommas escapes any comma within each value so that the result survives a round trip +// back through ExpandCommaSeparated. Used when echoing user selections into the generated +// automation command, which emits values verbatim. +func EscapeCommas(values []string) []string { + if len(values) == 0 { + return values + } + result := make([]string, 0, len(values)) + for _, value := range values { + result = append(result, strings.ReplaceAll(value, ",", "\\,")) + } + return result +} + func ParseVariableStringArray(variables []string) (map[string]string, error) { result := make(map[string]string, len(variables)) for _, v := range variables { diff --git a/pkg/executionscommon/executionscommon_test.go b/pkg/executionscommon/executionscommon_test.go index 0942b9c4..701cf942 100644 --- a/pkg/executionscommon/executionscommon_test.go +++ b/pkg/executionscommon/executionscommon_test.go @@ -432,6 +432,11 @@ func TestExpandCommaSeparated(t *testing.T) { {name: "preserves order and duplicates", input: []string{"ABC,ABC"}, expect: []string{"ABC", "ABC"}}, {name: "tenant tags", input: []string{"Regions/us-east,Regions/us-west"}, expect: []string{"Regions/us-east", "Regions/us-west"}}, + + {name: "escaped comma is a literal comma", input: []string{`Web\, Prod`}, expect: []string{"Web, Prod"}}, + {name: "escaped and unescaped commas mix", input: []string{`Web\, Prod,Other`}, expect: []string{"Web, Prod", "Other"}}, + {name: "backslash not before a comma is preserved", input: []string{`DOMAIN\host,Other`}, expect: []string{`DOMAIN\host`, "Other"}}, + {name: "trailing backslash is preserved", input: []string{`ABC\`}, expect: []string{`ABC\`}}, } for _, test := range tests { @@ -444,7 +449,7 @@ func TestExpandCommaSeparated(t *testing.T) { } // a blank component almost always means a caller-side variable expanded to nothing; dropping it -// silently would narrow the scope of a deployment, or flip a tenanted deploy to untenanted +// silently would narrow the scope of a deployment (or flip a tenanted deploy to untenanted) func TestExpandCommaSeparated_RejectsBlankValues(t *testing.T) { tests := []struct { name string @@ -482,3 +487,17 @@ func TestExpandCommaSeparatedFlags(t *testing.T) { err := executionscommon.ExpandCommaSeparatedFlags(environments, bad) assert.ErrorContains(t, err, "--deployment-target has a blank value") } + +// values chosen interactively are echoed back as an automation command verbatim, so any comma +// inside them has to be escaped or the replayed command would split it back apart +func TestEscapeCommas_RoundTripsThroughExpand(t *testing.T) { + assert.Nil(t, executionscommon.EscapeCommas(nil)) + + input := []string{"Web, Prod", "Plain", `Already\, Escaped`} + escaped := executionscommon.EscapeCommas(input) + assert.Equal(t, []string{`Web\, Prod`, "Plain", `Already\\, Escaped`}, escaped) + + expanded, err := executionscommon.ExpandCommaSeparated("deployment-target", escaped) + assert.NoError(t, err) + assert.Equal(t, []string{"Web, Prod", "Plain", `Already\, Escaped`}, expanded) +}