From 954e06ed8bdeb59836f0f688a45fdea2e3583a76 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:44:08 +1000 Subject: [PATCH 01/26] fix: report missing package versions instead of a server null reference `release create --no-prompt` sends the create request straight to the server without resolving package versions first. When a package has no version in its feed the server raises a null reference exception, which surfaces as "Octopus API error: Object reference not set to an instance of an object. []". On a 5xx failure the CLI now repeats the package version resolution the server does, and reports the packages, steps and feeds that have no version available. Where it can't identify a specific package, an unhandled server error now carries a hint about the likely causes. Fixes #426 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create.go | 93 +++++++++++- pkg/cmd/release/create/create_test.go | 207 ++++++++++++++++++++++++++ pkg/packages/packages.go | 101 ++++++++++--- 3 files changed, 382 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index b4b50caa..2f1d835b 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -28,6 +28,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/util/flag" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/feeds" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" @@ -318,7 +319,7 @@ func createRun(cmd *cobra.Command, f factory.Factory, flags *CreateFlags) error executor.NewTask(executor.TaskTypeCreateRelease, options), }) if err != nil { - return err + return DiagnoseCreateReleaseFailure(octopus, options, err) } if options.Response != nil { @@ -420,6 +421,96 @@ func BuildPackageVersionBaselineForChannel(octopus *octopusApiClient.Client, dep return result, nil } +// serverNullReferenceMessage is what an Octopus Server sends back when it hits an unhandled +// null reference exception; it carries no information about what actually went wrong. +const serverNullReferenceMessage = "Object reference not set to an instance of an object" + +// DiagnoseCreateReleaseFailure replaces an opaque server-side failure with an actionable message where +// it can. The server raises a null reference exception, surfaced as a bare 500, when it can't select a +// version for a package; see https://github.com/OctopusDeploy/cli/issues/426 +func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease, cause error) error { + var apiError *core.APIError + if !errors.As(cause, &apiError) || apiError.StatusCode < 500 { + return cause + } + + // diagnosis is best-effort; if any part of it fails we must not mask the original failure + if octopus != nil && options != nil { + if missingPackages, findErr := findPackagesWithoutVersions(octopus, options); findErr == nil && len(missingPackages) > 0 { + return packages.NewMissingPackageVersionsError(missingPackages, cause) + } + } + + if strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) { + return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) + } + return cause +} + +// findPackagesWithoutVersions repeats the package version resolution the server does when it assembles a +// release, so we can report which packages have no version available in their feed. +func findPackagesWithoutVersions(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease) ([]releases.ReleaseTemplatePackage, error) { + project, err := selectors.FindProject(octopus, options.ProjectName) + if err != nil { + return nil, err + } + + gitReferenceKey := "" + if project.PersistenceSettings != nil && project.PersistenceSettings.Type() == projects.PersistenceSettingsTypeVersionControlled { + gitReferenceKey = options.GitReference + if options.GitCommit != "" { // prefer a specific git commit if one was specified + gitReferenceKey = options.GitCommit + } + } + + deploymentProcess, err := octopus.DeploymentProcesses.Get(project, gitReferenceKey) + if err != nil { + return nil, err + } + + channel, err := findChannelForDiagnosis(octopus, project, options.ChannelName) + if err != nil { + return nil, err + } + + deploymentProcessTemplate, err := octopus.DeploymentProcesses.GetTemplate(deploymentProcess, channel.ID, "") + if err != nil { + return nil, err + } + + packageVersionBaseline, err := BuildPackageVersionBaselineForChannel(octopus, deploymentProcessTemplate, channel) + if err != nil { + return nil, err + } + + overrides := packages.BuildPackageVersionOverrides(packageVersionBaseline, options.DefaultPackageVersion, options.PackageVersionOverrides) + resolvedVersions := packages.ApplyPackageOverrides(packageVersionBaseline, overrides) + + return packages.FindPackagesWithoutVersions(deploymentProcessTemplate.Packages, resolvedVersions), nil +} + +// findChannelForDiagnosis locates the channel the server would have used. When no channel was specified we +// can only guess; the default channel is the best approximation available to us. +func findChannelForDiagnosis(octopus *octopusApiClient.Client, project *projects.Project, channelName string) (*channels.Channel, error) { + if channelName != "" { + return selectors.FindChannel(octopus, project, channelName) + } + + existingChannels, err := octopus.Projects.GetChannels(project) + if err != nil { + return nil, err + } + if len(existingChannels) == 1 { + return existingChannels[0], nil + } + for _, c := range existingChannels { + if c.IsDefault { + return c, nil + } + } + return nil, fmt.Errorf("cannot determine the default channel for project %s", project.GetName()) +} + func AskQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, options *executor.TaskOptionsCreateRelease) error { if octopus == nil { return cliErrors.NewArgumentNullOrEmptyError("octopus") diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 65ee97c7..87078d8e 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -3,6 +3,7 @@ package create_test import ( "bytes" "errors" + "net/http" "net/url" "os" "testing" @@ -19,6 +20,7 @@ import ( "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/constants" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/credentials" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/feeds" @@ -2829,3 +2831,208 @@ func TestReleaseCreate_ApplyPackageOverride(t *testing.T) { }, result) }) } + +func TestReleaseCreate_FindPackagesWithoutVersions(t *testing.T) { + resolvable := releases.ReleaseTemplatePackage{ + ActionName: "Deploy Website", + FeedID: "feeds-builtin", + FeedName: "Octopus Server (built-in)", + PackageID: "acme-web", + PackageReferenceName: "acme-web", + IsResolvable: true, + } + + t.Run("reports a resolvable package with no version", func(t *testing.T) { + missing := packages.FindPackagesWithoutVersions( + []releases.ReleaseTemplatePackage{resolvable}, + []*packages.StepPackageVersion{{PackageID: "acme-web", ActionName: "Deploy Website", PackageReferenceName: "acme-web", Version: ""}}) + + assert.Equal(t, []releases.ReleaseTemplatePackage{resolvable}, missing) + }) + + t.Run("ignores a package which has a version", func(t *testing.T) { + missing := packages.FindPackagesWithoutVersions( + []releases.ReleaseTemplatePackage{resolvable}, + []*packages.StepPackageVersion{{PackageID: "acme-web", ActionName: "Deploy Website", PackageReferenceName: "acme-web", Version: "1.0.0"}}) + + assert.Equal(t, []releases.ReleaseTemplatePackage{}, missing) + }) + + t.Run("ignores packages which don't need a version at release creation time", func(t *testing.T) { + fixed := resolvable + fixed.FixedVersion = "1.0.0" + unresolvable := resolvable + unresolvable.IsResolvable = false + + missing := packages.FindPackagesWithoutVersions( + []releases.ReleaseTemplatePackage{fixed, unresolvable}, + []*packages.StepPackageVersion{{PackageID: "acme-web", ActionName: "Deploy Website", PackageReferenceName: "acme-web", Version: ""}}) + + assert.Equal(t, []releases.ReleaseTemplatePackage{}, missing) + }) + + t.Run("matches on step and package reference, not just package ID", func(t *testing.T) { + secondStep := resolvable + secondStep.ActionName = "Deploy Worker" + + missing := packages.FindPackagesWithoutVersions( + []releases.ReleaseTemplatePackage{resolvable, secondStep}, + []*packages.StepPackageVersion{ + {PackageID: "acme-web", ActionName: "Deploy Website", PackageReferenceName: "acme-web", Version: "1.0.0"}, + {PackageID: "acme-web", ActionName: "Deploy Worker", PackageReferenceName: "acme-web", Version: ""}, + }) + + assert.Equal(t, []releases.ReleaseTemplatePackage{secondStep}, missing) + }) +} + +func TestReleaseCreate_MissingPackageVersionsError(t *testing.T) { + cause := errors.New("Octopus API error: Object reference not set to an instance of an object. []") + + t.Run("names the package, step and feed", func(t *testing.T) { + err := packages.NewMissingPackageVersionsError([]releases.ReleaseTemplatePackage{{ + ActionName: "Deploy Website", + FeedID: "feeds-builtin", + FeedName: "Octopus Server (built-in)", + PackageID: "acme-web", + PackageReferenceName: "acme-web", + }}, cause) + + assert.EqualError(t, err, heredoc.Doc(` + cannot create release; no version could be found for the following packages: + - 'acme-web' in step 'Deploy Website' (feed 'Octopus Server (built-in)') + push the package(s) to the feed, or supply a version with --package or --package-version`)) + + assert.Equal(t, cause, errors.Unwrap(err)) + }) + + t.Run("qualifies the package with its reference name where they differ", func(t *testing.T) { + err := packages.NewMissingPackageVersionsError([]releases.ReleaseTemplatePackage{{ + ActionName: "Deploy Website", + FeedID: "Feeds-1001", + PackageID: "acme-web", + PackageReferenceName: "extra-config", + }}, cause) + + // no FeedName in this response, so it falls back to the feed ID + assert.EqualError(t, err, heredoc.Doc(` + cannot create release; no version could be found for the following packages: + - 'acme-web/extra-config' in step 'Deploy Website' (feed 'Feeds-1001') + push the package(s) to the feed, or supply a version with --package or --package-version`)) + }) +} + +func TestReleaseCreate_DiagnoseCreateReleaseFailure(t *testing.T) { + t.Run("passes through errors which aren't server faults", func(t *testing.T) { + cause := errors.New("no such host") + assert.Equal(t, cause, create.DiagnoseCreateReleaseFailure(nil, nil, cause)) + + badRequest := &core.APIError{ErrorMessage: "release version 1.0.0 already exists", StatusCode: http.StatusBadRequest} + assert.Equal(t, error(badRequest), create.DiagnoseCreateReleaseFailure(nil, nil, badRequest)) + }) +} + +// issue #426: the server raises a null reference exception rather than telling us that a package +// referenced by the deployment process has no version available in its feed +func TestReleaseCreate_AutomationMode_MissingPackageDiagnosis(t *testing.T) { + const spaceID = "Spaces-1" + const fireProjectID = "Projects-22" + const builtinFeedID = "feeds-builtin" + + space1 := fixtures.NewSpace(spaceID, "Default Space") + depProcess := fixtures.NewDeploymentProcessForProject(spaceID, fireProjectID) + fireProject := fixtures.NewProject(spaceID, fireProjectID, "Fire Project", "Lifecycles-1", "ProjectGroups-1", depProcess.ID) + defaultChannel := fixtures.NewChannel(spaceID, "Channels-1", "Default", fireProjectID) + + nullReferenceError := &core.APIError{ErrorMessage: "Object reference not set to an instance of an object."} + + tests := []struct { + name string + run func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) + }{ + {"reports the package which has no version in its feed", 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", "create", "--project", fireProject.Name, "--version", "1.0.0"}) + 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").RespondWith(fireProject) + + api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1"). + RespondWithStatus(http.StatusInternalServerError, "500 Internal Server Error", nullReferenceError) + + // the CLI now goes back to the server to work out what the real problem was + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/deploymentprocesses/"+depProcess.ID).RespondWith(depProcess) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{defaultChannel}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/deploymentprocesses/template?channel=Channels-1"). + RespondWith(&deployments.DeploymentProcessTemplate{ + Packages: []releases.ReleaseTemplatePackage{{ + ActionName: "Deploy Website", + FeedID: builtinFeedID, + FeedName: "Octopus Server (built-in)", + PackageID: "acme-web", + PackageReferenceName: "acme-web", + IsResolvable: true, + }}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds?ids="+builtinFeedID+"&take=1").RespondWith(&feeds.Feeds{Items: []feeds.IFeed{ + &feeds.FeedResource{Name: "Octopus Server (built-in)", FeedType: feeds.FeedTypeBuiltIn, Resource: resources.Resource{ + ID: builtinFeedID, + Links: map[string]string{ + constants.LinkSearchPackageVersionsTemplate: "/api/Spaces-1/feeds/feeds-builtin/packages/versions{?packageId,take,skip,includePreRelease,versionRange,preReleaseTag,filter,includeReleaseNotes}", + }}}, + }}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds/feeds-builtin/packages/versions?packageId=acme-web&take=1"). + RespondWith(&resources.Resources[*octopusPackages.PackageVersion]{Items: []*octopusPackages.PackageVersion{}}) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, heredoc.Doc(` + cannot create release; no version could be found for the following packages: + - 'acme-web' in step 'Deploy Website' (feed 'Octopus Server (built-in)') + push the package(s) to the feed, or supply a version with --package or --package-version`)) + + assert.Equal(t, "", stdOut.String()) + }}, + + {"falls back to a hint when it can't identify a missing package", 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", "create", "--project", fireProject.Name}) + 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").RespondWith(fireProject) + + api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1"). + RespondWithStatus(http.StatusInternalServerError, "500 Internal Server Error", nullReferenceError) + + // the diagnosis is best-effort; this server can't tell us about the deployment process + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/deploymentprocesses/"+depProcess.ID).RespondWithStatus(http.StatusNotFound, "404 Not Found", nil) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, "Octopus API error: Object reference not set to an instance of an object. [] \nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release") + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + api := testutil.NewMockHttpServer() + + rootCmd := cmdRoot.NewCmdRoot(testutil.NewMockFactoryWithSpace(api, space1), nil, nil) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + + test.run(t, api, rootCmd, stdout, stderr) + }) + } +} diff --git a/pkg/packages/packages.go b/pkg/packages/packages.go index 3eff889a..e32d0986 100644 --- a/pkg/packages/packages.go +++ b/pkg/packages/packages.go @@ -180,6 +180,88 @@ func BuildPackageVersionBaseline(octopus *octopusApiClient.Client, packages []re return result, nil } +// FindPackagesWithoutVersions returns the deployment process template packages which the server +// expects to have a version at release creation time, but for which no version could be found in the feed. +// Packages with a fixed version, or which aren't resolvable until deployment time, are excluded because +// they don't need one. +func FindPackagesWithoutVersions(templatePackages []releases.ReleaseTemplatePackage, resolvedVersions []*StepPackageVersion) []releases.ReleaseTemplatePackage { + result := make([]releases.ReleaseTemplatePackage, 0) + for _, templatePackage := range templatePackages { + if templatePackage.FixedVersion != "" || !templatePackage.IsResolvable { + continue + } + for _, resolved := range resolvedVersions { + if resolved.PackageID == templatePackage.PackageID && + resolved.ActionName == templatePackage.ActionName && + resolved.PackageReferenceName == templatePackage.PackageReferenceName { + if strings.TrimSpace(resolved.Version) == "" { + result = append(result, templatePackage) + } + break + } + } + } + return result +} + +// MissingPackageVersionsError is raised when one or more packages referenced by the deployment process +// have no version available in their feed. The server can't assemble a release in this state; rather than +// reporting that, it raises a null reference exception, so the CLI detects the situation itself. +type MissingPackageVersionsError struct { + Packages []releases.ReleaseTemplatePackage + cause error +} + +func NewMissingPackageVersionsError(missingPackages []releases.ReleaseTemplatePackage, cause error) *MissingPackageVersionsError { + return &MissingPackageVersionsError{Packages: missingPackages, cause: cause} +} + +func (e *MissingPackageVersionsError) Unwrap() error { return e.cause } + +func (e *MissingPackageVersionsError) Error() string { + sb := &strings.Builder{} + sb.WriteString("cannot create release; no version could be found for the following packages:") + for _, p := range e.Packages { + packageName := p.PackageID + if p.PackageReferenceName != "" && p.PackageReferenceName != p.PackageID { + packageName = fmt.Sprintf("%s/%s", packageName, p.PackageReferenceName) + } + feedName := p.FeedName + if feedName == "" { + feedName = p.FeedID + } + sb.WriteString(fmt.Sprintf("\n - '%s' in step '%s' (feed '%s')", packageName, p.ActionName, feedName)) + } + sb.WriteString("\npush the package(s) to the feed, or supply a version with --package or --package-version") + return sb.String() +} + +// BuildPackageVersionOverrides converts the --package-version and --package command line flags into +// resolved overrides, using the baseline to work out which step or package each override refers to. +// Anything that can't be parsed or resolved is ignored; the server reports those. +func BuildPackageVersionOverrides(packageVersionBaseline []*StepPackageVersion, defaultPackageVersion string, packageOverrideFlags []string) []*PackageVersionOverride { + packageVersionOverrides := make([]*PackageVersionOverride, 0, len(packageOverrideFlags)+1) + + if defaultPackageVersion != "" { + // blind apply to everything + packageVersionOverrides = append(packageVersionOverrides, &PackageVersionOverride{Version: defaultPackageVersion}) + } + + for _, s := range packageOverrideFlags { + ambOverride, err := ParsePackageOverrideString(s) + if err != nil { + continue // silently ignore anything that wasn't parseable (should we emit a warning?) + } + resolvedOverride, err := ResolvePackageOverride(ambOverride, packageVersionBaseline) + if err != nil { + continue // silently ignore anything that wasn't parseable (should we emit a warning?) + } + packageVersionOverrides = append(packageVersionOverrides, resolvedOverride) + } + + return packageVersionOverrides +} + type PackageVersionOverride struct { ActionName string // optional, but one or both of ActionName or PackageID must be supplied PackageID string // optional, but one or both of ActionName or PackageID must be supplied @@ -539,25 +621,8 @@ func AskPackageOverrideLoop( initialPackageOverrideFlags []string, // the --package command line flag (multiple occurrences) asker question.Asker, stdout io.Writer) ([]*StepPackageVersion, []*PackageVersionOverride, error) { - packageVersionOverrides := make([]*PackageVersionOverride, 0) - // pickup any partial package specifications that may have arrived on the commandline - if defaultPackageVersion != "" { - // blind apply to everything - packageVersionOverrides = append(packageVersionOverrides, &PackageVersionOverride{Version: defaultPackageVersion}) - } - - for _, s := range initialPackageOverrideFlags { - ambOverride, err := ParsePackageOverrideString(s) - if err != nil { - continue // silently ignore anything that wasn't parseable (should we emit a warning?) - } - resolvedOverride, err := ResolvePackageOverride(ambOverride, packageVersionBaseline) - if err != nil { - continue // silently ignore anything that wasn't parseable (should we emit a warning?) - } - packageVersionOverrides = append(packageVersionOverrides, resolvedOverride) - } + packageVersionOverrides := BuildPackageVersionOverrides(packageVersionBaseline, defaultPackageVersion, initialPackageOverrideFlags) overriddenPackageVersions := ApplyPackageOverrides(packageVersionBaseline, packageVersionOverrides) From e417d72e8092e4384bc1e0a6fdf2e8218cbd27d3 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:03:53 +1000 Subject: [PATCH 02/26] fix: only diagnose the null reference failure, not every 5xx The package diagnosis ran for any APIError with a 5xx status. On an unrelated server error that had the side effect of (a) replacing a real server message with MissingPackageVersionsError, whose Error() doesn't include the cause, and (b) firing ~6 extra requests at a server that is already failing. Require the null reference message before diagnosing, which is the only failure this code knows how to explain. The fallback hint no longer needs its own check, since reaching it now implies the message matched. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create.go | 10 +++++----- pkg/cmd/release/create/create_test.go | 10 ++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 2f1d835b..90ae84cb 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -429,8 +429,11 @@ const serverNullReferenceMessage = "Object reference not set to an instance of a // it can. The server raises a null reference exception, surfaced as a bare 500, when it can't select a // version for a package; see https://github.com/OctopusDeploy/cli/issues/426 func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease, cause error) error { + // only the specific null reference failure is worth diagnosing. Any other 5xx is a real server error + // that we must report as-is; replacing it would hide the cause, and re-querying the server would pile + // more requests onto something that is already failing. var apiError *core.APIError - if !errors.As(cause, &apiError) || apiError.StatusCode < 500 { + if !errors.As(cause, &apiError) || apiError.StatusCode < 500 || !strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) { return cause } @@ -441,10 +444,7 @@ func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *exe } } - if strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) { - return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) - } - return cause + return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) } // findPackagesWithoutVersions repeats the package version resolution the server does when it assembles a diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 87078d8e..ce04cbd9 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -2930,6 +2930,16 @@ func TestReleaseCreate_DiagnoseCreateReleaseFailure(t *testing.T) { badRequest := &core.APIError{ErrorMessage: "release version 1.0.0 already exists", StatusCode: http.StatusBadRequest} assert.Equal(t, error(badRequest), create.DiagnoseCreateReleaseFailure(nil, nil, badRequest)) }) + + t.Run("passes through server faults which aren't the null reference we know how to diagnose", func(t *testing.T) { + // an unrelated 5xx must be reported as-is; we mustn't replace it with a package diagnosis + // (nor go back to an already-failing server to run one) + serverError := &core.APIError{ErrorMessage: "The database is unavailable", StatusCode: http.StatusInternalServerError} + assert.Equal(t, error(serverError), create.DiagnoseCreateReleaseFailure(nil, nil, serverError)) + + badGateway := &core.APIError{ErrorMessage: "Bad Gateway", StatusCode: http.StatusBadGateway} + assert.Equal(t, error(badGateway), create.DiagnoseCreateReleaseFailure(nil, nil, badGateway)) + }) } // issue #426: the server raises a null reference exception rather than telling us that a package From 23678715a0aebf0166a881ff534280f26dec42c7 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:05:36 +1000 Subject: [PATCH 03/26] fix: honour --ignore-channel-rules and channel IDs in the diagnosis Two ways the replay could diverge from what the server actually did: - With --ignore-channel-rules the server resolves package versions without applying the channel's version rules, but the replay always applied them. A package with versions in its feed, none satisfying the rules, would be reported as "no version could be found", misdiagnosing the real failure. Build the baseline without the rule filter in that case. - --channel reaches the server as ChannelIDOrName, but the lookup matched on name only, so passing a channel ID silently dropped the diagnosis to the generic hint. Match on either. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create.go | 31 ++++++++++---- pkg/cmd/release/create/create_test.go | 59 +++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 90ae84cb..5c7786de 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -478,7 +478,15 @@ func findPackagesWithoutVersions(octopus *octopusApiClient.Client, options *exec return nil, err } - packageVersionBaseline, err := BuildPackageVersionBaselineForChannel(octopus, deploymentProcessTemplate, channel) + // mirror what the server did: with --ignore-channel-rules it selects versions without applying the + // channel's version rules, so applying them here would report packages as missing when they only + // failed the rules. + var packageVersionBaseline []*packages.StepPackageVersion + if options.IgnoreChannelRules { + packageVersionBaseline, err = packages.BuildPackageVersionBaseline(octopus, deploymentProcessTemplate.Packages, nil) + } else { + packageVersionBaseline, err = BuildPackageVersionBaselineForChannel(octopus, deploymentProcessTemplate, channel) + } if err != nil { return nil, err } @@ -489,17 +497,24 @@ func findPackagesWithoutVersions(octopus *octopusApiClient.Client, options *exec return packages.FindPackagesWithoutVersions(deploymentProcessTemplate.Packages, resolvedVersions), nil } -// findChannelForDiagnosis locates the channel the server would have used. When no channel was specified we -// can only guess; the default channel is the best approximation available to us. -func findChannelForDiagnosis(octopus *octopusApiClient.Client, project *projects.Project, channelName string) (*channels.Channel, error) { - if channelName != "" { - return selectors.FindChannel(octopus, project, channelName) - } - +// findChannelForDiagnosis locates the channel the server would have used. --channel reaches the server as +// ChannelIDOrName, so we match on either. When no channel was specified we can only guess; the default +// channel is the best approximation available to us. +func findChannelForDiagnosis(octopus *octopusApiClient.Client, project *projects.Project, channelIDOrName string) (*channels.Channel, error) { existingChannels, err := octopus.Projects.GetChannels(project) if err != nil { return nil, err } + + if channelIDOrName != "" { + for _, c := range existingChannels { + if strings.EqualFold(c.Name, channelIDOrName) || c.ID == channelIDOrName { + return c, nil + } + } + return nil, fmt.Errorf("no channel found with name or ID of %s", channelIDOrName) + } + if len(existingChannels) == 1 { return existingChannels[0], nil } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index ce04cbd9..e7fee464 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -3010,6 +3010,65 @@ func TestReleaseCreate_AutomationMode_MissingPackageDiagnosis(t *testing.T) { assert.Equal(t, "", stdOut.String()) }}, + {"doesn't apply channel version rules when --ignore-channel-rules was specified", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + // the server resolved versions without the channel rules, so the diagnosis must too; + // otherwise a package which only fails the rules gets reported as having no version at all + ruledChannel := fixtures.NewChannel(spaceID, "Channels-1", "Default", fireProjectID) + ruledChannel.Rules = []channels.ChannelRule{{ + Tag: "^pre$", + VersionRange: "[5.0,6.0)", + ActionPackages: []octopusPackages.DeploymentActionPackage{ + {DeploymentAction: "Deploy Website", PackageReference: "acme-web"}, + }, + }} + + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"release", "create", "--project", fireProject.Name, "--ignore-channel-rules"}) + 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").RespondWith(fireProject) + + api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1"). + RespondWithStatus(http.StatusInternalServerError, "500 Internal Server Error", nullReferenceError) + + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/deploymentprocesses/"+depProcess.ID).RespondWith(depProcess) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{ruledChannel}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/deploymentprocesses/template?channel=Channels-1"). + RespondWith(&deployments.DeploymentProcessTemplate{ + Packages: []releases.ReleaseTemplatePackage{{ + ActionName: "Deploy Website", + FeedID: builtinFeedID, + FeedName: "Octopus Server (built-in)", + PackageID: "acme-web", + PackageReferenceName: "acme-web", + IsResolvable: true, + }}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds?ids="+builtinFeedID+"&take=1").RespondWith(&feeds.Feeds{Items: []feeds.IFeed{ + &feeds.FeedResource{Name: "Octopus Server (built-in)", FeedType: feeds.FeedTypeBuiltIn, Resource: resources.Resource{ + ID: builtinFeedID, + Links: map[string]string{ + constants.LinkSearchPackageVersionsTemplate: "/api/Spaces-1/feeds/feeds-builtin/packages/versions{?packageId,take,skip,includePreRelease,versionRange,preReleaseTag,filter,includeReleaseNotes}", + }}}, + }}) + // no versionRange or preReleaseTag in the query, despite the channel carrying a rule for this package + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds/feeds-builtin/packages/versions?packageId=acme-web&take=1"). + RespondWith(&resources.Resources[*octopusPackages.PackageVersion]{Items: []*octopusPackages.PackageVersion{}}) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, heredoc.Doc(` + cannot create release; no version could be found for the following packages: + - 'acme-web' in step 'Deploy Website' (feed 'Octopus Server (built-in)') + push the package(s) to the feed, or supply a version with --package or --package-version`)) + }}, + {"falls back to a hint when it can't identify a missing package", 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() From 9201499bde5e7cbf47c22eeaa93e835c003279ec Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:23:46 +1000 Subject: [PATCH 04/26] fix: report the server's own error alongside the package diagnosis ea972e2 narrowed the diagnosis to failures carrying the server's null reference message, to stop an unrelated 5xx being reported as a package problem. That works, but it also switches the fix off on current servers: the #426 path there fails with "There are no viable release plans in any channels", not a null reference, so the message the server sends for this is version-dependent and can't be relied on as the trigger. Address the underlying complaint instead. MissingPackageVersionsError now prints what the server actually said, so a misattributed diagnosis costs the user a misleading paragraph rather than the real cause, which was previously reachable only via Unwrap and never printed (main.go prints err.Error() alone). With nothing hidden, the trigger widens back to any 5xx and keeps working across server versions. The null reference message itself is still suppressed from that output -- it says nothing the diagnosis doesn't say better -- so the integration test's guard against it resurfacing stays valid. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create.go | 19 +++++++------ pkg/cmd/release/create/create_test.go | 13 +++++++-- pkg/packages/packages.go | 13 +++++++++ pkg/packages/packages_test.go | 40 +++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 pkg/packages/packages_test.go diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 5c7786de..3d3be1bb 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -421,19 +421,17 @@ func BuildPackageVersionBaselineForChannel(octopus *octopusApiClient.Client, dep return result, nil } -// serverNullReferenceMessage is what an Octopus Server sends back when it hits an unhandled -// null reference exception; it carries no information about what actually went wrong. -const serverNullReferenceMessage = "Object reference not set to an instance of an object" - // DiagnoseCreateReleaseFailure replaces an opaque server-side failure with an actionable message where // it can. The server raises a null reference exception, surfaced as a bare 500, when it can't select a // version for a package; see https://github.com/OctopusDeploy/cli/issues/426 +// +// Any 5xx is diagnosed, not just the null reference one, because the message a server sends for this +// varies by version: current servers report "no viable release plans" instead. The cost of being wrong +// is bounded, since MissingPackageVersionsError reports what the server actually said alongside the +// diagnosis. func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease, cause error) error { - // only the specific null reference failure is worth diagnosing. Any other 5xx is a real server error - // that we must report as-is; replacing it would hide the cause, and re-querying the server would pile - // more requests onto something that is already failing. var apiError *core.APIError - if !errors.As(cause, &apiError) || apiError.StatusCode < 500 || !strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) { + if !errors.As(cause, &apiError) || apiError.StatusCode < 500 { return cause } @@ -444,7 +442,10 @@ func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *exe } } - return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) + if strings.Contains(apiError.ErrorMessage, packages.ServerNullReferenceMessage) { + return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) + } + return cause } // findPackagesWithoutVersions repeats the package version resolution the server does when it assembles a diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index e7fee464..77b85acc 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -2931,15 +2931,22 @@ func TestReleaseCreate_DiagnoseCreateReleaseFailure(t *testing.T) { assert.Equal(t, error(badRequest), create.DiagnoseCreateReleaseFailure(nil, nil, badRequest)) }) - t.Run("passes through server faults which aren't the null reference we know how to diagnose", func(t *testing.T) { - // an unrelated 5xx must be reported as-is; we mustn't replace it with a package diagnosis - // (nor go back to an already-failing server to run one) + t.Run("passes through server faults it cannot diagnose", func(t *testing.T) { + // a 5xx is only replaced when the CLI can positively name the packages behind it. With no + // client to go and look, and no null reference message to explain, the server error stands. serverError := &core.APIError{ErrorMessage: "The database is unavailable", StatusCode: http.StatusInternalServerError} assert.Equal(t, error(serverError), create.DiagnoseCreateReleaseFailure(nil, nil, serverError)) badGateway := &core.APIError{ErrorMessage: "Bad Gateway", StatusCode: http.StatusBadGateway} assert.Equal(t, error(badGateway), create.DiagnoseCreateReleaseFailure(nil, nil, badGateway)) }) + + t.Run("explains a bare null reference fault even when no packages are missing", func(t *testing.T) { + nullRef := &core.APIError{ErrorMessage: "Object reference not set to an instance of an object.", StatusCode: http.StatusInternalServerError} + err := create.DiagnoseCreateReleaseFailure(nil, nil, nullRef) + assert.ErrorIs(t, err, nullRef) + assert.Contains(t, err.Error(), "the server failed with an unhandled error") + }) } // issue #426: the server raises a null reference exception rather than telling us that a package diff --git a/pkg/packages/packages.go b/pkg/packages/packages.go index e32d0986..52f38eed 100644 --- a/pkg/packages/packages.go +++ b/pkg/packages/packages.go @@ -204,6 +204,11 @@ func FindPackagesWithoutVersions(templatePackages []releases.ReleaseTemplatePack return result } +// ServerNullReferenceMessage is what an Octopus Server sends back when it hits an unhandled null +// reference exception; it carries no information about what actually went wrong, so it is worth +// replacing rather than reporting. +const ServerNullReferenceMessage = "Object reference not set to an instance of an object" + // MissingPackageVersionsError is raised when one or more packages referenced by the deployment process // have no version available in their feed. The server can't assemble a release in this state; rather than // reporting that, it raises a null reference exception, so the CLI detects the situation itself. @@ -233,6 +238,14 @@ func (e *MissingPackageVersionsError) Error() string { sb.WriteString(fmt.Sprintf("\n - '%s' in step '%s' (feed '%s')", packageName, p.ActionName, feedName)) } sb.WriteString("\npush the package(s) to the feed, or supply a version with --package or --package-version") + // this diagnosis is inferred from a failure the server doesn't describe, so it can be wrong. + // Report what the server actually said too, unless that's the null reference message, which + // says nothing the lines above don't already say better. + if e.cause != nil { + if causeText := e.cause.Error(); !strings.Contains(causeText, ServerNullReferenceMessage) { + sb.WriteString(fmt.Sprintf("\nthe server reported: %s", causeText)) + } + } return sb.String() } diff --git a/pkg/packages/packages_test.go b/pkg/packages/packages_test.go new file mode 100644 index 00000000..c6e9a813 --- /dev/null +++ b/pkg/packages/packages_test.go @@ -0,0 +1,40 @@ +package packages_test + +import ( + "errors" + "testing" + + "github.com/OctopusDeploy/cli/pkg/packages" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/releases" + "github.com/stretchr/testify/assert" +) + +func TestMissingPackageVersionsError_Error(t *testing.T) { + missing := []releases.ReleaseTemplatePackage{ + {PackageID: "acme.web", ActionName: "Deploy Web", FeedID: "feeds-builtin", FeedName: "Octopus Server (built-in)"}, + } + + t.Run("names the package, the step and the feed", func(t *testing.T) { + err := packages.NewMissingPackageVersionsError(missing, nil) + assert.Contains(t, err.Error(), "no version could be found for the following packages") + assert.Contains(t, err.Error(), "'acme.web' in step 'Deploy Web' (feed 'Octopus Server (built-in)')") + assert.Contains(t, err.Error(), "push the package(s) to the feed") + }) + + // the diagnosis is inferred from a failure the server doesn't describe, so if we guessed wrong + // the user still needs to be able to see what actually went wrong + t.Run("reports what the server said alongside the diagnosis", func(t *testing.T) { + cause := errors.New("There are no viable release plans in any channels") + err := packages.NewMissingPackageVersionsError(missing, cause) + assert.Contains(t, err.Error(), "the server reported: There are no viable release plans in any channels") + assert.ErrorIs(t, err, cause) + }) + + t.Run("omits the null reference message, which explains nothing", func(t *testing.T) { + cause := errors.New("Octopus API error: " + packages.ServerNullReferenceMessage + " []") + err := packages.NewMissingPackageVersionsError(missing, cause) + assert.NotContains(t, err.Error(), packages.ServerNullReferenceMessage) + assert.NotContains(t, err.Error(), "the server reported") + assert.ErrorIs(t, err, cause) // still unwrappable, just not printed + }) +} From 815919742824c4eb8a76e1e70eafb4999d72cf75 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:48:07 +1000 Subject: [PATCH 05/26] fix: only replay the diagnosis for a 500, not any 5xx The replay can't be gated on the server's message -- verified against a current server, the #426 scenario comes back as a 500 carrying "There are no viable release plans in any channels", not the null reference message the issue reported -- so the trigger stays message-independent. It can be gated on the status code, though: this failure is always raised by the API itself as a 500, so a 502/503/504 is something in front of the server and is never worth ~6 extra requests. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create.go | 8 +++++--- pkg/cmd/release/create/create_test.go | 13 ++++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 3d3be1bb..7fd59ad1 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "net/http" "os" "strings" "time" @@ -425,13 +426,14 @@ func BuildPackageVersionBaselineForChannel(octopus *octopusApiClient.Client, dep // it can. The server raises a null reference exception, surfaced as a bare 500, when it can't select a // version for a package; see https://github.com/OctopusDeploy/cli/issues/426 // -// Any 5xx is diagnosed, not just the null reference one, because the message a server sends for this +// Any 500 is diagnosed, not just the null reference one, because the message a server sends for this // varies by version: current servers report "no viable release plans" instead. The cost of being wrong // is bounded, since MissingPackageVersionsError reports what the server actually said alongside the -// diagnosis. +// diagnosis. Other 5xx codes are excluded: the failure we are looking for is always raised by the API +// itself as a 500, so a 502/503/504 is something in front of the server and never worth replaying. func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease, cause error) error { var apiError *core.APIError - if !errors.As(cause, &apiError) || apiError.StatusCode < 500 { + if !errors.As(cause, &apiError) || apiError.StatusCode != http.StatusInternalServerError { return cause } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 77b85acc..c6f0ad8e 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -2932,13 +2932,20 @@ func TestReleaseCreate_DiagnoseCreateReleaseFailure(t *testing.T) { }) t.Run("passes through server faults it cannot diagnose", func(t *testing.T) { - // a 5xx is only replaced when the CLI can positively name the packages behind it. With no + // a 500 is only replaced when the CLI can positively name the packages behind it. With no // client to go and look, and no null reference message to explain, the server error stands. serverError := &core.APIError{ErrorMessage: "The database is unavailable", StatusCode: http.StatusInternalServerError} assert.Equal(t, error(serverError), create.DiagnoseCreateReleaseFailure(nil, nil, serverError)) + }) - badGateway := &core.APIError{ErrorMessage: "Bad Gateway", StatusCode: http.StatusBadGateway} - assert.Equal(t, error(badGateway), create.DiagnoseCreateReleaseFailure(nil, nil, badGateway)) + t.Run("passes through 5xx codes which aren't the API's own failure", func(t *testing.T) { + // the release plan failure is always raised by the API itself as a 500; anything else in the + // 5xx range came from in front of the server, so there is nothing worth replaying + for _, statusCode := range []int{http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout} { + // carrying the null reference message, to show it's the status code doing the work here + gatewayError := &core.APIError{ErrorMessage: "Object reference not set to an instance of an object.", StatusCode: statusCode} + assert.Equal(t, error(gatewayError), create.DiagnoseCreateReleaseFailure(nil, nil, gatewayError)) + } }) t.Run("explains a bare null reference fault even when no packages are missing", func(t *testing.T) { From 827828793c7855ba6ea068790e6e925a3ece2a17 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:50:11 +1000 Subject: [PATCH 06/26] test: cover resolving the diagnosis channel by ID 66ee411 made findChannelForDiagnosis match on channel ID as well as name, but nothing exercised it. Reverting that match to name-only now fails this case, which is the point of it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create_test.go | 49 +++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index c6f0ad8e..432e9819 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -3083,6 +3083,55 @@ func TestReleaseCreate_AutomationMode_MissingPackageDiagnosis(t *testing.T) { push the package(s) to the feed, or supply a version with --package or --package-version`)) }}, + {"finds the channel when --channel was given as an ID rather than a name", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + // --channel reaches the server as ChannelIDOrName with no client-side resolution, so an ID + // is a legitimate input; matching on name alone would silently drop the diagnosis + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"release", "create", "--project", fireProject.Name, "--channel", defaultChannel.ID, "--version", "1.0.0"}) + 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").RespondWith(fireProject) + + api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1"). + RespondWithStatus(http.StatusInternalServerError, "500 Internal Server Error", nullReferenceError) + + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/deploymentprocesses/"+depProcess.ID).RespondWith(depProcess) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{defaultChannel}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/deploymentprocesses/template?channel=Channels-1"). + RespondWith(&deployments.DeploymentProcessTemplate{ + Packages: []releases.ReleaseTemplatePackage{{ + ActionName: "Deploy Website", + FeedID: builtinFeedID, + FeedName: "Octopus Server (built-in)", + PackageID: "acme-web", + PackageReferenceName: "acme-web", + IsResolvable: true, + }}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds?ids="+builtinFeedID+"&take=1").RespondWith(&feeds.Feeds{Items: []feeds.IFeed{ + &feeds.FeedResource{Name: "Octopus Server (built-in)", FeedType: feeds.FeedTypeBuiltIn, Resource: resources.Resource{ + ID: builtinFeedID, + Links: map[string]string{ + constants.LinkSearchPackageVersionsTemplate: "/api/Spaces-1/feeds/feeds-builtin/packages/versions{?packageId,take,skip,includePreRelease,versionRange,preReleaseTag,filter,includeReleaseNotes}", + }}}, + }}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/feeds/feeds-builtin/packages/versions?packageId=acme-web&take=1"). + RespondWith(&resources.Resources[*octopusPackages.PackageVersion]{Items: []*octopusPackages.PackageVersion{}}) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, heredoc.Doc(` + cannot create release; no version could be found for the following packages: + - 'acme-web' in step 'Deploy Website' (feed 'Octopus Server (built-in)') + push the package(s) to the feed, or supply a version with --package or --package-version`)) + }}, + {"falls back to a hint when it can't identify a missing package", 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() From b6dc414322031b373f761df2714e1abf42384aea Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:45:14 +1000 Subject: [PATCH 07/26] fix: report unknown release versions instead of a server null reference `release deploy` passed --version straight to the executions API, which answers an unknown version with "Object reference not set to an instance of an object". Resolve the release before deploying so a version that doesn't exist is reported by name, and call out `latest` explicitly since it is not a supported alias. Refs #294 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy.go | 12 +++- pkg/cmd/release/deploy/deploy_test.go | 76 +++++++++++++------- pkg/cmd/release/progression/shared/shared.go | 11 +-- pkg/question/selectors/releases.go | 45 ++++++++++++ 4 files changed, 109 insertions(+), 35 deletions(-) create mode 100644 pkg/question/selectors/releases.go diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 835c523a..b701cdce 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -344,6 +344,16 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error return err } options.ProjectName = project.GetName() + + if options.ReleaseVersion != "" { + // resolve the release up front; the executions API reports an unknown version as an + // unhelpful null reference error, and having the ID saves looking it up again later + release, err := selectors.FindRelease(octopus, f.GetCurrentSpace().ID, project, options.ReleaseVersion) + if err != nil { + return err + } + options.ReleaseID = release.ID + } } } @@ -453,7 +463,7 @@ func AskQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker ques return err } } else { - selectedRelease, err = releases.GetReleaseInProject(octopus, space.ID, selectedProject.ID, options.ReleaseVersion) + selectedRelease, err = selectors.FindRelease(octopus, space.ID, selectedProject, options.ReleaseVersion) if err != nil { return err } diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 24d8d238..49c5358c 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -1780,6 +1780,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.9").RespondWith(release10) _, err := testutil.ReceivePair(cmdReceiver) assert.EqualError(t, err, "environment(s) must be specified") @@ -1788,6 +1789,45 @@ func TestDeployCreate_AutomationMode(t *testing.T) { assert.Equal(t, "", stdErr.String()) }}, + {"release deploy reports a release version that doesn't exist", 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", "9.9", "--environment", "dev"}) + 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/9.9"). + RespondWithStatus(404, "404 Not Found", &core.APIError{ErrorMessage: "The resource you requested was not found."}) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, "cannot find a release with version '9.9' in project 'Fire Project'") + + assert.Equal(t, "", stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + + {"release deploy explains that 'latest' is not a supported release version", 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", "latest", "--environment", "dev"}) + 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/latest").RespondWithStatus(404, "NotFound", nil) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.EqualError(t, err, "cannot find a release with version 'latest' in project 'Fire Project'; 'latest' is not a supported alias, specify an exact version. Run 'octopus release list --project \"Fire Project\"' to see the available versions") + + assert.Equal(t, "", stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + {"release deploy specifying project, version, env only (bare minimum) assuming 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() @@ -1798,6 +1838,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -1820,12 +1861,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { }, }) - // now it's going to try and look up the project/version to generate the web URL - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ - Items: []*projects.Project{fireProject}, - }) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + // no lookup to generate the web URL; the release was already resolved before deploying _, err = testutil.ReceivePair(cmdReceiver) assert.Nil(t, err) @@ -1848,6 +1884,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/2.1").RespondWith(release10) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -1870,12 +1907,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { }, }) - // now it's going to try and look up the project/version to generate the web URL - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ - Items: []*projects.Project{fireProject}, - }) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/2.1").RespondWith(release10) + // no lookup to generate the web URL; the release was already resolved before deploying _, err = testutil.ReceivePair(cmdReceiver) assert.Nil(t, err) @@ -1898,6 +1930,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1").RespondWith(&deployments.CreateDeploymentResponseV1{ @@ -1928,6 +1961,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted serverTasks := []*deployments.DeploymentServerTask{ @@ -1959,6 +1993,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) @@ -1980,12 +2015,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { }, }) - // now it's going to try and look up the project/version to generate the web URL - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ - Items: []*projects.Project{fireProject}, - }) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + // no lookup to generate the web URL; the release was already resolved before deploying _, err = testutil.ReceivePair(cmdReceiver) assert.Nil(t, err) @@ -2008,6 +2038,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) @@ -2029,12 +2060,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { }, }) - // now it's going to try and look up the project/version to generate the web URL - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ - Items: []*projects.Project{fireProject}, - }) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + // no lookup to generate the web URL; the release was already resolved before deploying _, err = testutil.ReceivePair(cmdReceiver) assert.Nil(t, err) @@ -2166,6 +2192,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -2246,6 +2273,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) diff --git a/pkg/cmd/release/progression/shared/shared.go b/pkg/cmd/release/progression/shared/shared.go index 94f669b3..a181a771 100644 --- a/pkg/cmd/release/progression/shared/shared.go +++ b/pkg/cmd/release/progression/shared/shared.go @@ -40,14 +40,5 @@ func SelectRelease(octopus *client.Client, project *projects.Project, ask questi } func FindRelease(octopus *client.Client, project *projects.Project, version string) (*releases.Release, error) { - existingRelease, err := releases.GetReleaseInProject(octopus, octopus.GetSpaceID(), project.GetID(), version) - if err != nil { - return nil, err - } - - if existingRelease == nil { - return nil, fmt.Errorf("unable to locate a release with version/release number '%s'", version) - } - - return existingRelease, nil + return selectors.FindRelease(octopus, octopus.GetSpaceID(), project, version) } diff --git a/pkg/question/selectors/releases.go b/pkg/question/selectors/releases.go new file mode 100644 index 00000000..04e44cb0 --- /dev/null +++ b/pkg/question/selectors/releases.go @@ -0,0 +1,45 @@ +package selectors + +import ( + "errors" + "fmt" + "net/http" + "strings" + + "github.com/OctopusDeploy/cli/pkg/constants" + octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/releases" +) + +// latestReleaseAlias is the value the old `octo` CLI accepted to mean "the newest release". +// This CLI has no equivalent, so it is called out explicitly when the lookup fails. +const latestReleaseAlias = "latest" + +// FindRelease looks up a release by version within a project. A version that doesn't exist is +// reported here, because the executions API answers one with a null reference error instead. +func FindRelease(octopus *octopusApiClient.Client, spaceID string, project *projects.Project, releaseVersion string) (*releases.Release, error) { + release, err := releases.GetReleaseInProject(octopus, spaceID, project.GetID(), releaseVersion) + if err != nil { + var apiError *core.APIError + if errors.As(err, &apiError) && apiError.StatusCode == http.StatusNotFound { + return nil, releaseNotFoundError(project, releaseVersion) + } + return nil, err + } + // a 404 with an empty body doesn't reach the error path above; it decodes as an empty release + if release == nil || release.GetID() == "" { + return nil, releaseNotFoundError(project, releaseVersion) + } + + return release, nil +} + +func releaseNotFoundError(project *projects.Project, releaseVersion string) error { + if strings.EqualFold(releaseVersion, latestReleaseAlias) { + return fmt.Errorf("cannot find a release with version '%s' in project '%s'; '%s' is not a supported alias, specify an exact version. Run '%s release list --project \"%s\"' to see the available versions", + releaseVersion, project.GetName(), releaseVersion, constants.ExecutableName, project.GetName()) + } + return fmt.Errorf("cannot find a release with version '%s' in project '%s'", releaseVersion, project.GetName()) +} From f1f1607fcd0f2243f6b48b670931eec8e2e120cc Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:08:39 +1000 Subject: [PATCH 08/26] fix: don't assert "not found" when the release lookup is ambiguous The SDK's DoRawJsonRequest short-circuits on `resp.ContentLength == 0` and returns `(resp, nil)` for any status code, so DoRequest hands back a zero-valued Release with a nil error. A 404 with no body lands there, but so does a 403 with an empty body or a 502 from a proxy, and the status code is not recoverable at this layer. Reporting all of those as "cannot find a release with version X" is misleading during an outage or a permissions failure. Introduce selectors.ReleaseNotFoundError, which records whether the server confirmed the answer with a 404 carrying an APIError body, and hedge the wording when it did not. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy_test.go | 2 +- pkg/question/selectors/releases.go | 47 ++++++++++++++++++++------- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 49c5358c..d854548f 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -1822,7 +1822,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/latest").RespondWithStatus(404, "NotFound", nil) _, err := testutil.ReceivePair(cmdReceiver) - assert.EqualError(t, err, "cannot find a release with version 'latest' in project 'Fire Project'; 'latest' is not a supported alias, specify an exact version. Run 'octopus release list --project \"Fire Project\"' to see the available versions") + assert.EqualError(t, err, "could not resolve a release with version 'latest' in project 'Fire Project'; the server returned an empty response, which usually means there is no such release, but can also mean the lookup itself failed. 'latest' is not a supported alias, specify an exact version. Run 'octopus release list --project \"Fire Project\"' to see the available versions") assert.Equal(t, "", stdOut.String()) assert.Equal(t, "", stdErr.String()) diff --git a/pkg/question/selectors/releases.go b/pkg/question/selectors/releases.go index 04e44cb0..56e8026e 100644 --- a/pkg/question/selectors/releases.go +++ b/pkg/question/selectors/releases.go @@ -17,29 +17,54 @@ import ( // This CLI has no equivalent, so it is called out explicitly when the lookup fails. const latestReleaseAlias = "latest" +// ReleaseNotFoundError reports a release version that the server didn't return a release for. +// +// Confirmed distinguishes the two ways that answer arrives. A 404 carrying an APIError body is a +// definite "no such release". An empty response body is not: the SDK's DoRawJsonRequest short-circuits +// on `resp.ContentLength == 0` and returns (resp, nil) for *any* status code, so a 403 with no body, or +// a 502 from a proxy, decodes into a zero-valued Release with a nil error and is indistinguishable from +// a 404 by the time it reaches us. The status code isn't recoverable at this layer, so the message +// hedges rather than asserting the release is missing. +type ReleaseNotFoundError struct { + ProjectName string + ReleaseVersion string + Confirmed bool +} + +func (e *ReleaseNotFoundError) Error() string { + var message string + if e.Confirmed { + message = fmt.Sprintf("cannot find a release with version '%s' in project '%s'", e.ReleaseVersion, e.ProjectName) + } else { + message = fmt.Sprintf("could not resolve a release with version '%s' in project '%s'; the server returned an empty response, which usually means there is no such release, but can also mean the lookup itself failed", e.ReleaseVersion, e.ProjectName) + } + + if strings.EqualFold(e.ReleaseVersion, latestReleaseAlias) { + message += fmt.Sprintf(". '%s' is not a supported alias, specify an exact version. Run '%s release list --project \"%s\"' to see the available versions", + e.ReleaseVersion, constants.ExecutableName, e.ProjectName) + } + + return message +} + // FindRelease looks up a release by version within a project. A version that doesn't exist is // reported here, because the executions API answers one with a null reference error instead. +// Anything else (a permissions failure, a transport error) is returned untouched, so callers that +// would rather let the server be the authority can tell the two apart with errors.As. func FindRelease(octopus *octopusApiClient.Client, spaceID string, project *projects.Project, releaseVersion string) (*releases.Release, error) { release, err := releases.GetReleaseInProject(octopus, spaceID, project.GetID(), releaseVersion) if err != nil { var apiError *core.APIError if errors.As(err, &apiError) && apiError.StatusCode == http.StatusNotFound { - return nil, releaseNotFoundError(project, releaseVersion) + return nil, &ReleaseNotFoundError{ProjectName: project.GetName(), ReleaseVersion: releaseVersion, Confirmed: true} } return nil, err } - // a 404 with an empty body doesn't reach the error path above; it decodes as an empty release + // an empty response body doesn't reach the error path above; it decodes as an empty release. + // See ReleaseNotFoundError for why this can't be reported as a definite "not found". if release == nil || release.GetID() == "" { - return nil, releaseNotFoundError(project, releaseVersion) + return nil, &ReleaseNotFoundError{ProjectName: project.GetName(), ReleaseVersion: releaseVersion} } return release, nil } - -func releaseNotFoundError(project *projects.Project, releaseVersion string) error { - if strings.EqualFold(releaseVersion, latestReleaseAlias) { - return fmt.Errorf("cannot find a release with version '%s' in project '%s'; '%s' is not a supported alias, specify an exact version. Run '%s release list --project \"%s\"' to see the available versions", - releaseVersion, project.GetName(), releaseVersion, constants.ExecutableName, project.GetName()) - } - return fmt.Errorf("cannot find a release with version '%s' in project '%s'", releaseVersion, project.GetName()) -} From a44de49348edfb96ca75d3e372fe898470f3120c Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:16 +1000 Subject: [PATCH 09/26] refactor: drop the now-unreachable web-URL release lookup With the release resolved before the deploy, `options.ReleaseID` is always set on both paths that reach the link: AskQuestions in interactive mode, the pre-flight lookup in automation mode (the executor rejects the deploy unless both ProjectName and ReleaseVersion are set, which is exactly when the pre-flight runs). The FindProject + GetReleaseInProject fallback can only run when the pre-flight lookup already failed, where repeating it would fail too. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index b701cdce..bd533a39 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -387,20 +387,10 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error // output web URL all the time, so long as output format is not JSON or basic if err == nil && !constants.IsProgrammaticOutputFormat(outputFormat) { - releaseID := options.ReleaseID - if releaseID == "" { - // we may already have the release ID from AskQuestions. If not, we need to go and look up the release ID to link to it - // which needs the project ID. Errors here are ignorable; it's not the end of the world if we can't print the web link - prj, err := selectors.FindProject(octopus, options.ProjectName) - if err == nil { - rel, err := releases.GetReleaseInProject(octopus, f.GetCurrentSpace().ID, prj.ID, options.ReleaseVersion) - if err == nil { - releaseID = rel.ID - } - } - } - - if releaseID != "" { + // both paths that reach here have already resolved the release: AskQuestions in interactive + // mode, the pre-flight lookup in automation mode. It stays empty only when that lookup failed + // for a reason we deliberately ignored, in which case repeating it here would fail too. + if releaseID := options.ReleaseID; releaseID != "" { link := output.Bluef("%s/app#/%s/releases/%s", f.GetCurrentHost(), f.GetCurrentSpace().ID, releaseID) cmd.Printf("\nView this release on Octopus Deploy: %s\n", link) } From fcde44c6e94be69116b5f7bdbdfbd046afec20e2 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:06 +1000 Subject: [PATCH 10/26] fix: only a missing release aborts the deploy pre-flight The pre-flight lookup is new to `release deploy`; before it, the automation path never read the release and the executions API only ever saw the version string. Failing the whole deploy on any lookup error would break a CI service account scoped to deploy but not to ReleaseView, and would turn a transient 5xx on that GET into an aborted deployment that previously succeeded. Fail only on a ReleaseNotFoundError, which is the case issue #294 is about. For anything else, carry on without the release ID and let the server remain the authority on permissions and availability. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy.go | 13 +++++++-- pkg/cmd/release/deploy/deploy_test.go | 42 +++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index bd533a39..6e7dea1f 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -347,12 +347,19 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error if options.ReleaseVersion != "" { // resolve the release up front; the executions API reports an unknown version as an - // unhelpful null reference error, and having the ID saves looking it up again later + // unhelpful null reference error, and having the ID saves looking it up again later. + // Only a "no such release" answer is fatal: this lookup is new to the deploy path, so + // anything else (no ReleaseView permission, a transient 5xx) must not fail a deploy + // that would previously have succeeded. In those cases the server stays the authority + // and we simply go without the release ID. release, err := selectors.FindRelease(octopus, f.GetCurrentSpace().ID, project, options.ReleaseVersion) - if err != nil { + var releaseNotFound *selectors.ReleaseNotFoundError + if errors.As(err, &releaseNotFound) { return err } - options.ReleaseID = release.ID + if err == nil { + options.ReleaseID = release.ID + } } } diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index d854548f..d71a2d0d 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -1828,6 +1828,48 @@ func TestDeployCreate_AutomationMode(t *testing.T) { assert.Equal(t, "", stdErr.String()) }}, + {"release deploy proceeds when the release lookup fails for a reason other than not-found", 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"}) + 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) + + // an account allowed to deploy but not to read releases must not be blocked by the pre-flight lookup + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0"). + RespondWithStatus(403, "403 Forbidden", &core.APIError{ErrorMessage: "You do not have permission to perform this action."}) + + 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"}, + 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) + + // no release ID, so no web link; the deployment itself still went ahead + assert.Equal(t, "Successfully started 1 deployment(s)\n", stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + {"release deploy specifying project, version, env only (bare minimum) assuming 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() From b4ec69a2244f92604a49850f24422db2e6cda542 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:16 +1000 Subject: [PATCH 11/26] refactor: call selectors.FindRelease directly from GetReleaseID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shared.FindRelease was left as a one-line passthrough, so remove it. Doing so also puts GetReleaseID's spaceID parameter to use — it was accepted and then ignored in favour of octopus.GetSpaceID(). Both callers already pass opts.Client.GetSpaceID(), so the resolved space is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/progression/shared/shared.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/cmd/release/progression/shared/shared.go b/pkg/cmd/release/progression/shared/shared.go index a181a771..e9d7249d 100644 --- a/pkg/cmd/release/progression/shared/shared.go +++ b/pkg/cmd/release/progression/shared/shared.go @@ -16,7 +16,7 @@ func GetReleaseID(octopus *client.Client, spaceID string, projectIdentifier stri return "", err } - selectedRelease, err := FindRelease(octopus, selectedProject, version) + selectedRelease, err := selectors.FindRelease(octopus, spaceID, selectedProject, version) if err != nil { return "", err } @@ -38,7 +38,3 @@ func SelectRelease(octopus *client.Client, project *projects.Project, ask questi return selectedRelease, nil } - -func FindRelease(octopus *client.Client, project *projects.Project, version string) (*releases.Release, error) { - return selectors.FindRelease(octopus, octopus.GetSpaceID(), project, version) -} From e18d9e541c8c01fd0836ed06e457ac1ba50d62d4 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 15 Sep 2026 16:57:44 +1000 Subject: [PATCH 12/26] test: expect the release pre-flight lookup in the --priority cases The --priority tests arrived on main (#708) after this branch was cut, so they were the only deploy cases not already expecting the release lookup this branch adds. Same one-line expectation as every other case here. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index d71a2d0d..1e2fe63d 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -2125,6 +2125,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) @@ -2163,6 +2164,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) From 8907e0e0de188180f8aa78630e9c2b63746a1bcc Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 12:24:27 +1000 Subject: [PATCH 13/26] fix: accept IDs as well as names for --channel, --environment and --tenant The executions API only matches channels, environments and tenants by name, so `release create`, `release deploy` and `runbook run` passed whatever the caller typed straight through and the server rejected IDs. `--project` already worked because the server accepts a project ID or name. Resolve those identifiers client side through the shared selectors package before handing them to the executor, preferring an ID match over a name match so it behaves the same way as `--project`. Fixes #250 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/channel/delete/delete_test.go | 2 +- pkg/cmd/channel/view/view_test.go | 4 +- pkg/cmd/release/create/create.go | 8 + pkg/cmd/release/create/create_test.go | 64 ++++++++ pkg/cmd/release/deploy/deploy.go | 53 +++++- pkg/cmd/release/deploy/deploy_test.go | 87 +++++++++- pkg/cmd/runbook/run/run.go | 18 +++ pkg/cmd/runbook/run/run_test.go | 78 +++++++++ pkg/executionscommon/executionscommon.go | 39 +---- pkg/question/selectors/channels.go | 13 +- pkg/question/selectors/environments.go | 54 +++++-- pkg/question/selectors/find_test.go | 198 +++++++++++++++++++++++ pkg/question/selectors/tenants.go | 38 +++++ 13 files changed, 590 insertions(+), 66 deletions(-) create mode 100644 pkg/question/selectors/find_test.go create mode 100644 pkg/question/selectors/tenants.go diff --git a/pkg/cmd/channel/delete/delete_test.go b/pkg/cmd/channel/delete/delete_test.go index d4a57197..eab6c2ce 100644 --- a/pkg/cmd/channel/delete/delete_test.go +++ b/pkg/cmd/channel/delete/delete_test.go @@ -167,7 +167,7 @@ func TestChannelDelete(t *testing.T) { // No DELETE request is expected; api.Close() asserts nothing further was requested. _, err := testutil.ReceivePair(cmdReceiver) - assert.EqualError(t, err, "no channel found with name of Channels-99") + assert.EqualError(t, err, "cannot find a channel in project 'Fire Project' with the ID or name of 'Channels-99'") assert.Equal(t, "", stdErr.String()) }}, diff --git a/pkg/cmd/channel/view/view_test.go b/pkg/cmd/channel/view/view_test.go index 556f85f5..96e69fe2 100644 --- a/pkg/cmd/channel/view/view_test.go +++ b/pkg/cmd/channel/view/view_test.go @@ -238,7 +238,7 @@ func TestChannelView(t *testing.T) { }) _, err := testutil.ReceivePair(cmdReceiver) - assert.EqualError(t, err, "no channel found with name of Channels-99") + assert.EqualError(t, err, "cannot find a channel in project 'Fire Project' with the ID or name of 'Channels-99'") assert.Equal(t, "", stdOut.String()) assert.Equal(t, "", stdErr.String()) @@ -262,7 +262,7 @@ func TestChannelView(t *testing.T) { }) _, err := testutil.ReceivePair(cmdReceiver) - assert.EqualError(t, err, "no channel found with name of Nonexistent") + assert.EqualError(t, err, "cannot find a channel in project 'Fire Project' with the ID or name of 'Nonexistent'") assert.Equal(t, "", stdOut.String()) assert.Equal(t, "", stdErr.String()) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index b4b50caa..a9bfd01c 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -310,6 +310,14 @@ func createRun(cmd *cobra.Command, f factory.Factory, flags *CreateFlags) error return err } options.ProjectName = project.GetName() + + if options.ChannelName != "" { // the executions API only matches channels by name, so resolve any ID we were given + channel, err := selectors.FindChannel(octopus, project, options.ChannelName) + if err != nil { + return err + } + options.ChannelName = channel.Name + } } } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 65ee97c7..872b87b3 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -1209,6 +1209,7 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { protectedBranchNamePatterns := []string{} cacProject := fixtures.NewProject(space1.ID, cacProjectID, "CaC Project", "Lifecycles-1", "ProjectGroups-1", depProcess.ID) + betaChannel := fixtures.NewChannel(space1.ID, "Channels-31", "BetaChannel", cacProjectID) cacProject.PersistenceSettings = projects.NewGitPersistenceSettings( ".octopus", credentials.NewAnonymous(), @@ -1588,6 +1589,53 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { assert.EqualError(t, err, "cannot specify both --release-notes and --release-notes-file at the same time") }}, + {"release creation specifying the project and channel by ID", 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", "create", "--project", cacProjectID, "--channel", betaChannel.ID}) + 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/"+cacProjectID).RespondWith(cacProject) + + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{betaChannel}, + }) + + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1") + + // the executions API only matches channels by name, so the ID must have been resolved before we got here + requestBody, err := testutil.ReadJson[releases.CreateReleaseCommandV1](req.Request.Body) + assert.Nil(t, err) + + assert.Equal(t, releases.CreateReleaseCommandV1{ + SpaceID: "Spaces-1", + ProjectIDOrName: cacProject.Name, + ChannelIDOrName: betaChannel.Name, + }, requestBody) + + req.RespondWith(&releases.CreateReleaseResponseV1{ + ReleaseID: "Releases-999", + ReleaseVersion: "1.2.3", + }) + + releaseInfo := releases.NewRelease(betaChannel.ID, cacProject.ID, "1.2.3") + api.ExpectRequest(t, "GET", "/api/Spaces-1/releases/Releases-999").RespondWith(releaseInfo) + api.ExpectRequest(t, "GET", "/api/Spaces-1/channels/"+betaChannel.ID).RespondWith(betaChannel) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + + assert.Equal(t, heredoc.Doc(` + Successfully created release version 1.2.3 using channel BetaChannel + + View this release on Octopus Deploy: http://server/app#/Spaces-1/releases/Releases-999 + `), stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + {"release creation with all the flags", 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() @@ -1611,6 +1659,10 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProject.GetName()).RespondWith(cacProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{betaChannel}, + }) + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1") // check that it sent the server the right request body @@ -1682,6 +1734,10 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProject.GetName()).RespondWith(cacProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{betaChannel}, + }) + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1") // check that it sent the server the right request body @@ -1748,6 +1804,10 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProject.GetName()).RespondWith(cacProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{betaChannel}, + }) + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1") // check that it sent the server the right request body @@ -1817,6 +1877,10 @@ func TestReleaseCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProject.GetName()).RespondWith(cacProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+cacProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{betaChannel}, + }) + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1") // check that it sent the server the right request body diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 835c523a..29e79f2a 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -36,6 +36,7 @@ import ( "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/releases" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/spaces" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/variables" "github.com/spf13/cobra" ) @@ -258,6 +259,15 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error options.ForcePackageDownloadWasSpecified = true } + // the executions API only matches tenants by name, so resolve any IDs we were given + if len(options.Tenants) > 0 { + selectedTenants, err := selectors.FindTenants(octopus, options.Tenants) + if err != nil { + return err + } + options.Tenants = util.SliceTransform(selectedTenants, func(t *tenants.Tenant) string { return t.Name }) + } + if f.IsPromptEnabled() { now := time.Now if cmd.Context() != nil { // allow context to override the definition of 'now' for testing @@ -346,6 +356,13 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error options.ProjectName = project.GetName() } + // the executions API only matches environments by name, so resolve any IDs we were given + if len(options.Environments) > 0 { + options.Environments, err = resolveEnvironmentNames(octopus, f.GetCurrentSpace(), options.Environments) + if err != nil { + return err + } + } } // the executor will raise errors if any required options are missing @@ -501,18 +518,21 @@ func AskQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker ques if len(deploymentEnvironmentIDs) == 0 { // if the Q&A process earlier hasn't loaded environments already, we need to load them now if selectedChannel.Type == channels.ChannelTypeLifecycle { - selectedEnvironments, err := executionscommon.FindEnvironments(octopus, options.Environments) + selectedEnvironments, err := selectors.FindEnvironments(octopus, options.Environments) if err != nil { return err } deploymentEnvironmentIDs = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.ID }) + options.Environments = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) } else if selectedChannel.Type == channels.ChannelTypeEphemeral { - deploymentEnvironmentIDs, err = findEphemeralEnvironmentIDs(octopus, space, options.Environments) - + selectedEnvironments, err := findEphemeralEnvironments(octopus, space, options.Environments) if err != nil { return err } + + deploymentEnvironmentIDs = util.SliceTransform(selectedEnvironments, func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.ID }) + options.Environments = util.SliceTransform(selectedEnvironments, func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.Name }) } } @@ -664,7 +684,7 @@ func AskQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker ques return nil } -func findEphemeralEnvironmentIDs(octopus *octopusApiClient.Client, space *spaces.Space, environments []string) ([]string, error) { +func findEphemeralEnvironments(octopus *octopusApiClient.Client, space *spaces.Space, environmentIdentifiers []string) ([]*ephemeralenvironments.EphemeralEnvironment, error) { allEphemeralEnvironments, err := ephemeralenvironments.GetAll(octopus, space.ID) if err != nil { return nil, err @@ -674,8 +694,8 @@ func findEphemeralEnvironmentIDs(octopus *octopusApiClient.Client, space *spaces return nil, errors.New("no ephemeral environments exist to deploy to") } - var selectedEnvironments []string - if len(environments) == 0 { + var selectedEnvironments []*ephemeralenvironments.EphemeralEnvironment + if len(environmentIdentifiers) == 0 { return nil, nil } @@ -685,17 +705,33 @@ func findEphemeralEnvironmentIDs(octopus *octopusApiClient.Client, space *spaces envMap[strings.ToLower(ephemeralEnv.Name)] = ephemeralEnv } - for _, envIdentifier := range environments { + for _, envIdentifier := range environmentIdentifiers { ephemeralEnv, found := envMap[strings.ToLower(envIdentifier)] if !found { return nil, fmt.Errorf("environment '%s' not found in ephemeral environments", envIdentifier) } - selectedEnvironments = append(selectedEnvironments, ephemeralEnv.ID) + selectedEnvironments = append(selectedEnvironments, ephemeralEnv) } return selectedEnvironments, nil } +// resolveEnvironmentNames maps environment names or IDs onto canonical environment names, because +// the executions API only matches environments by name. Ephemeral environments aren't part of the +// regular environment list, so they're looked up separately when the regular lookup comes up empty. +func resolveEnvironmentNames(octopus *octopusApiClient.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { + selectedEnvironments, err := selectors.FindEnvironments(octopus, environmentIdentifiers) + if err == nil { + return util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }), nil + } + + ephemeralEnvironments, ephemeralErr := findEphemeralEnvironments(octopus, space, environmentIdentifiers) + if ephemeralErr != nil { + return nil, err // ephemeral environments are the rarer case; report why the regular lookup failed + } + return util.SliceTransform(ephemeralEnvironments, func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.Name }), nil +} + func selectDeploymentEnvironmentsForEphemeralChannel(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, options *executor.TaskOptionsDeployRelease, selectedRelease *releases.Release) ([]string, error) { var deploymentEnvironmentIds []string var selectedEnvironments []*ephemeralenvironments.EphemeralEnvironment @@ -763,6 +799,7 @@ func selectDeploymentEnvironmentsForLifecycleChannel(octopus *octopusApiClient.C if err != nil { return nil, err } + options.Environments = []string{selectedEnvironment.Name} _, _ = fmt.Fprintf(stdout, "Environment %s\n", output.Cyan(selectedEnvironment.Name)) } selectedEnvironments = []*environments.Environment{selectedEnvironment} diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 24d8d238..4acedb1b 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -508,7 +508,7 @@ func TestDeployCreate_AskQuestions(t *testing.T) { assert.Equal(t, &executor.TaskOptionsDeployRelease{ ProjectName: "Fire Project", ReleaseVersion: "2.1", - Environments: []string{"ephemeral environment"}, + Environments: []string{"Ephemeral Environment"}, // the identifier from the command line is resolved to the canonical name GuidedFailureMode: "", Variables: make(map[string]string, 0), ReleaseID: release21.ID, @@ -1728,7 +1728,12 @@ func TestDeployCreate_AutomationMode(t *testing.T) { ////release20.ProjectDeploymentProcessSnapshotID = depProcessSnapshot.ID //release20.ProjectVariableSetSnapshotID = variableSnapshotWithPromptedVariables.ID // - //devEnvironment := fixtures.NewEnvironment(spaceID, "Environments-12", "dev") + devEnvironment := fixtures.NewEnvironment(spaceID, "Environments-12", "dev") + testEnvironment := fixtures.NewEnvironment(spaceID, "Environments-13", "test") + ephemeralEnvironment := fixtures.NewEphemeralEnvironment(spaceID, "Environments-123", "Ephemeral Environment", "Environments-12") + + cokeTenant := fixtures.NewTenant(spaceID, "Tenants-29", "Coke", "Regions/us-east", "Importance/High") + pepsiTenant := fixtures.NewTenant(spaceID, "Tenants-37", "Pepsi", "Regions/us-east", "Importance/Low") // TEST STARTS HERE tests := []struct { @@ -1798,6 +1803,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -1838,6 +1844,60 @@ func TestDeployCreate_AutomationMode(t *testing.T) { assert.Equal(t, "", stdErr.String()) }}, + {"release deploy specifying project, environment and tenant by ID", 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", fireProjectID, "--version", "1.0", "--environment", devEnvironment.ID, "--tenant", cokeTenant.ID}) + 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/tenants/"+cokeTenant.ID).RespondWith(cokeTenant) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID).RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") + + // the executions API only matches environments and tenants by name, so the IDs must have been resolved before we got here + requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) + assert.Nil(t, err) + + assert.Equal(t, deployments.CreateDeploymentTenantedCommandV1{ + ReleaseVersion: "1.0", + EnvironmentName: devEnvironment.Name, + Tenants: []string{cokeTenant.Name}, + CreateExecutionAbstractCommandV1: deployments.CreateExecutionAbstractCommandV1{ + SpaceID: "Spaces-1", + ProjectIDOrName: fireProject.Name, + }, + }, requestBody) + + req.RespondWith(&deployments.CreateDeploymentResponseV1{ + DeploymentServerTasks: []*deployments.DeploymentServerTask{ + {DeploymentID: "Deployments-203", ServerTaskID: "ServerTasks-29394"}, + }, + }) + + // now it's going to try and look up the project/version to generate the web URL + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ + Items: []*projects.Project{fireProject}, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + + assert.Equal(t, heredoc.Docf(` + Successfully started 1 deployment(s) + + View this release on Octopus Deploy: http://server/app#/Spaces-1/releases/%s + `, release10.ID), stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + {"release deploy specifying project, version, ephemeral env only (bare minimum)", 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() @@ -1848,6 +1908,13 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/v2?skip=0&take=2147483647&type=Ephemeral").RespondWith(resources.Resources[*ephemeralenvironments.EphemeralEnvironment]{ + Items: []*ephemeralenvironments.EphemeralEnvironment{ephemeralEnvironment}, + PagedResults: resources.PagedResults{ + TotalResults: 1, + }, + }) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -1898,6 +1965,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1").RespondWith(&deployments.CreateDeploymentResponseV1{ @@ -1928,6 +1996,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted serverTasks := []*deployments.DeploymentServerTask{ @@ -1958,7 +2027,13 @@ func TestDeployCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{cokeTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Pepsi").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Pepsi").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{pepsiTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) @@ -2008,6 +2083,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) @@ -2166,6 +2242,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") @@ -2245,7 +2322,13 @@ func TestDeployCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{cokeTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Pepsi").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Pepsi").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{pepsiTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index b0a5fa40..0f67559a 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -38,6 +38,7 @@ import ( "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/releases" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/runbooks" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/spaces" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" "github.com/spf13/cobra" ) @@ -246,6 +247,23 @@ func runbookRun(cmd *cobra.Command, f factory.Factory, flags *RunFlags) error { flags.Project.Value = project.Name + // the executions API only matches environments and tenants by name, so resolve any IDs we were given + if len(flags.Environments.Value) > 0 { + selectedEnvironments, err := selectors.FindEnvironments(octopus, flags.Environments.Value) + if err != nil { + return err + } + flags.Environments.Value = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) + } + + if len(flags.Tenants.Value) > 0 { + selectedTenants, err := selectors.FindTenants(octopus, flags.Tenants.Value) + if err != nil { + return err + } + flags.Tenants.Value = util.SliceTransform(selectedTenants, func(t *tenants.Tenant) string { return t.Name }) + } + if f.IsPromptEnabled() && flags.RunbookName.Value == "" && len(flags.RunbookTags.Value) == 0 { var runBySelection string err = f.Ask(&survey.Select{ diff --git a/pkg/cmd/runbook/run/run_test.go b/pkg/cmd/runbook/run/run_test.go index a3d50e83..bd8f3326 100644 --- a/pkg/cmd/runbook/run/run_test.go +++ b/pkg/cmd/runbook/run/run_test.go @@ -15,8 +15,11 @@ import ( "github.com/OctopusDeploy/cli/test/fixtures" "github.com/OctopusDeploy/cli/test/testutil" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/runbooks" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -39,6 +42,12 @@ func TestRunbookRun_AutomationMode(t *testing.T) { fireProject := fixtures.NewProject(spaceID, fireProjectID, "Fire Project", "Lifecycles-1", "ProjectGroups-1", "deploymentprocess-"+fireProjectID) _ = fireProject + devEnvironment := fixtures.NewEnvironment(spaceID, "Environments-12", "dev") + testEnvironment := fixtures.NewEnvironment(spaceID, "Environments-13", "test") + + cokeTenant := fixtures.NewTenant(spaceID, "Tenants-29", "Coke", "Regions/us-east", "Importance/High") + pepsiTenant := fixtures.NewTenant(spaceID, "Tenants-37", "Pepsi", "Regions/us-east", "Importance/Low") + // TEST STARTS HERE tests := []struct { name string @@ -107,6 +116,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") @@ -146,6 +156,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1").RespondWith(&runbooks.RunbookRunResponseV1{ @@ -175,6 +186,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) serverTasks := []*runbooks.RunbookRunServerTask{ {RunbookRunID: "RunbookRun-203", ServerTaskID: "ServerTasks-29394"}, @@ -196,6 +208,48 @@ func TestRunbookRun_AutomationMode(t *testing.T) { assert.Equal(t, "", stdErr.String()) }}, + {"runbook run specifying project, environment and tenant by ID", 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", fireProjectID, "--runbook", "Provision Database", "--environment", devEnvironment.ID, "--tenant", cokeTenant.ID}) + 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/"+fireProjectID).RespondWithJSON(fixtures.AsServerResponse(fireProject)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/"+cokeTenant.ID).RespondWith(cokeTenant) + + req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") + + // the executions API only matches environments and tenants by name, so the IDs must have been resolved before we got here + requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) + assert.Nil(t, err) + + assert.Equal(t, runbooks.RunbookRunCommandV1{ + RunbookName: "Provision Database", + EnvironmentNames: []string{devEnvironment.Name}, + Tenants: []string{cokeTenant.Name}, + CreateExecutionAbstractCommandV1: deployments.CreateExecutionAbstractCommandV1{ + SpaceID: "Spaces-1", + ProjectIDOrName: fireProject.Name, + }, + }, requestBody) + + req.RespondWith(&runbooks.RunbookRunResponseV1{ + RunbookRunServerTasks: []*runbooks.RunbookRunServerTask{ + {RunbookRunID: "RunbookRun-203", ServerTaskID: "ServerTasks-29394"}, + }, + }) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + + assert.Equal(t, "Successfully started 1 runbook run(s)\n", stdOut.String()) + assert.Equal(t, "", stdErr.String()) + }}, + {"runbook run specifying project, runbook, env only (bare minimum) assuming 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() @@ -206,6 +260,11 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{cokeTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Pepsi").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Pepsi").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{pepsiTenant}}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) @@ -245,6 +304,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) @@ -302,6 +362,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) @@ -373,6 +434,12 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { fireProject.PersistenceSettings.(projects.GitPersistenceSettings).SetRunbooksAreInGit() _ = fireProject + devEnvironment := fixtures.NewEnvironment(spaceID, "Environments-12", "dev") + testEnvironment := fixtures.NewEnvironment(spaceID, "Environments-13", "test") + + cokeTenant := fixtures.NewTenant(spaceID, "Tenants-29", "Coke", "Regions/us-east", "Importance/High") + pepsiTenant := fixtures.NewTenant(spaceID, "Tenants-37", "Pepsi", "Regions/us-east", "Importance/Low") + // TEST STARTS HERE tests := []struct { name string @@ -441,6 +508,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) _, err := testutil.ReceivePair(cmdReceiver) assert.EqualError(t, err, "git reference must be specified") @@ -459,6 +527,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/git/create/v1") @@ -499,6 +568,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) // Note: because we didn't specify --tenant or --tenant-tag, automation-mode code is going to assume untenanted api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/git/create/v1").RespondWith(&runbooks.GitRunbookRunResponseV1{ @@ -528,6 +598,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) serverTasks := []*runbooks.RunbookRunServerTask{ {RunbookRunID: "RunbookRun-203", ServerTaskID: "ServerTasks-29394"}, @@ -559,6 +630,11 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{cokeTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Pepsi").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Pepsi").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{pepsiTenant}}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/git/create/v1") requestBody, err := testutil.ReadJson[runbooks.GitRunbookRunCommandV1](req.Request.Body) @@ -599,6 +675,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/git/create/v1") requestBody, err := testutil.ReadJson[runbooks.GitRunbookRunCommandV1](req.Request.Body) @@ -660,6 +737,7 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/git/create/v1") requestBody, err := testutil.ReadJson[runbooks.GitRunbookRunCommandV1](req.Request.Body) diff --git a/pkg/executionscommon/executionscommon.go b/pkg/executionscommon/executionscommon.go index 91af36fa..e6b7d72f 100644 --- a/pkg/executionscommon/executionscommon.go +++ b/pkg/executionscommon/executionscommon.go @@ -8,6 +8,7 @@ import ( "github.com/AlecAivazis/survey/v2" cliErrors "github.com/OctopusDeploy/cli/pkg/errors" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/question/selectors" "github.com/OctopusDeploy/cli/pkg/surveyext" "github.com/OctopusDeploy/cli/pkg/util" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" @@ -462,40 +463,8 @@ func ScheduledStartTimeAnswerFormatter(datePicker *surveyext.DatePicker, t time. } } -// given an array of environment names, maps these all to actual objects by querying the server +// FindEnvironments maps an array of environment names or IDs onto the matching objects. +// Kept as an alias so existing callers don't have to change; selectors owns the lookup. func FindEnvironments(client *octopusApiClient.Client, environmentNamesOrIds []string) ([]*environments.Environment, error) { - if len(environmentNamesOrIds) == 0 { - return nil, nil - } - // there's no "bulk lookup" API, so we either need to do a foreach loop to find each environment individually, or load the entire server's worth of environments - // it's probably going to be cheaper to just list out all the environments and match them client side, so we'll do that for simplicity's sake - allEnvs, err := client.Environments.GetAll() - if err != nil { - return nil, err - } - - nameLookup := make(map[string]*environments.Environment, len(allEnvs)) - idLookup := make(map[string]*environments.Environment, len(allEnvs)) - - for _, env := range allEnvs { - nameLookup[strings.ToLower(env.GetName())] = env - idLookup[strings.ToLower(env.GetID())] = env - } - - var result []*environments.Environment - for _, n := range environmentNamesOrIds { - nameOrId := strings.ToLower(n) - env := nameLookup[nameOrId] - if env != nil { - result = append(result, env) - } else { - env = idLookup[nameOrId] - if env != nil { - result = append(result, env) - } else { - return nil, fmt.Errorf("cannot find environment %s", nameOrId) - } - } - } - return result, nil + return selectors.FindEnvironments(client, environmentNamesOrIds) } diff --git a/pkg/question/selectors/channels.go b/pkg/question/selectors/channels.go index 7a5452b6..59f330e9 100644 --- a/pkg/question/selectors/channels.go +++ b/pkg/question/selectors/channels.go @@ -26,15 +26,22 @@ func Channel(octopus *octopusApiClient.Client, ask question.Asker, io io.Writer, }) } -func FindChannel(octopus *octopusApiClient.Client, project *projects.Project, channelName string) (*channels.Channel, error) { +// FindChannel looks a channel up within a project by either its ID or its name. An ID match +// wins over a name match, so it stays consistent with how projects and tenants resolve. +func FindChannel(octopus *octopusApiClient.Client, project *projects.Project, channelIdentifier string) (*channels.Channel, error) { foundChannels, err := octopus.Projects.GetChannels(project) // TODO change this to channel partial name search on server; will require go client update if err != nil { return nil, err } + for _, c := range foundChannels { + if strings.EqualFold(c.ID, channelIdentifier) { + return c, nil + } + } for _, c := range foundChannels { // server doesn't support channel search by exact name so we must emulate it - if strings.EqualFold(c.Name, channelName) { + if strings.EqualFold(c.Name, channelIdentifier) { return c, nil } } - return nil, fmt.Errorf("no channel found with name of %s", channelName) + return nil, fmt.Errorf("cannot find a channel in project '%s' with the ID or name of '%s'", project.GetName(), channelIdentifier) } diff --git a/pkg/question/selectors/environments.go b/pkg/question/selectors/environments.go index 0570782f..2176b400 100644 --- a/pkg/question/selectors/environments.go +++ b/pkg/question/selectors/environments.go @@ -2,10 +2,11 @@ package selectors import ( "fmt" + "strings" + "github.com/OctopusDeploy/cli/pkg/question" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" - "strings" ) type GetAllEnvironmentsCallback func() ([]*environments.Environment, error) @@ -34,25 +35,48 @@ func EnvironmentSelect(ask question.Asker, getAllEnvironmentsCallback GetAllEnvi }) } -func FindEnvironment(octopus *client.Client, environmentName string) (*environments.Environment, error) { - resultPage, err := octopus.Environments.Get(environments.EnvironmentsQuery{PartialName: environmentName}) +// FindEnvironment looks an environment up by either its ID or its name. +func FindEnvironment(octopus *client.Client, environmentIdentifier string) (*environments.Environment, error) { + found, err := FindEnvironments(octopus, []string{environmentIdentifier}) if err != nil { return nil, err } - // environmentsQuery has "Name" but it's just an alias in the server for PartialName; we need to filter client side - for resultPage != nil && len(resultPage.Items) > 0 { - for _, c := range resultPage.Items { // server doesn't support search by exact name so we must emulate it - if strings.EqualFold(c.Name, environmentName) { - return c, nil - } - } - resultPage, err = resultPage.GetNextPage(octopus.Environments.GetClient()) - if err != nil { - return nil, err - } // if there are no more pages, then GetNextPage will return nil, which breaks us out of the loop + return found[0], nil +} + +// FindEnvironments looks environments up by either their IDs or their names. An ID match +// wins over a name match, so it stays consistent with how projects and tenants resolve. +func FindEnvironments(octopus *client.Client, environmentIdentifiers []string) ([]*environments.Environment, error) { + if len(environmentIdentifiers) == 0 { + return nil, nil + } + // there's no "bulk lookup" API, so we either need to do a foreach loop to find each environment individually, or load the entire server's worth of environments + // it's probably going to be cheaper to just list out all the environments and match them client side, so we'll do that for simplicity's sake + allEnvs, err := octopus.Environments.GetAll() + if err != nil { + return nil, err + } + + idLookup := make(map[string]*environments.Environment, len(allEnvs)) + nameLookup := make(map[string]*environments.Environment, len(allEnvs)) + for _, env := range allEnvs { + idLookup[strings.ToLower(env.GetID())] = env + nameLookup[strings.ToLower(env.GetName())] = env } - return nil, fmt.Errorf("no environment found with name of %s", environmentName) + result := make([]*environments.Environment, 0, len(environmentIdentifiers)) + for _, identifier := range environmentIdentifiers { + key := strings.ToLower(identifier) + env, found := idLookup[key] + if !found { + env, found = nameLookup[key] + } + if !found { + return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) + } + result = append(result, env) + } + return result, nil } func EnvironmentsMultiSelect(ask question.Asker, getAllEnvironmentsCallback GetAllEnvironmentsCallback, message string, required bool) ([]*environments.Environment, error) { diff --git a/pkg/question/selectors/find_test.go b/pkg/question/selectors/find_test.go new file mode 100644 index 00000000..0ff5612f --- /dev/null +++ b/pkg/question/selectors/find_test.go @@ -0,0 +1,198 @@ +package selectors_test + +import ( + "net/url" + "testing" + + "github.com/OctopusDeploy/cli/pkg/question/selectors" + "github.com/OctopusDeploy/cli/pkg/util" + "github.com/OctopusDeploy/cli/test/fixtures" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" + octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" + "github.com/stretchr/testify/assert" +) + +var serverUrl, _ = url.Parse("http://server") + +const placeholderApiKey = "API-XXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + +var findRootResource = testutil.NewRootResource() + +const findSpaceID = "Spaces-1" +const findProjectID = "Projects-22" + +// beginRequest spins up a mock server and hands back the client to run `action` against; +// the octopus client makes network calls on construction so it has to live in the goroutine +func beginRequest[T any](api *testutil.MockHttpServer, action func(octopus *octopusApiClient.Client) (T, error)) chan testutil.Pair[T, error] { + return testutil.GoBegin2(func() (T, error) { + defer api.Close() + octopus, _ := octopusApiClient.NewClient(testutil.NewMockHttpClientWithTransport(api), serverUrl, placeholderApiKey, "") + return action(octopus) + }) +} + +func TestFindEnvironments(t *testing.T) { + devEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-12", "dev") + prodEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-13", "production") + + // an environment which is *named* like an ID, to prove the precedence rule + decoyEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-99", "Environments-13") + + allEnvironments := []*environments.Environment{devEnvironment, prodEnvironment, decoyEnvironment} + + tests := []struct { + name string + identifiers []string + expectedIDs []string + expectedErr string + }{ + {"finds an environment by name", []string{"dev"}, []string{devEnvironment.ID}, ""}, + {"finds an environment by name, ignoring case", []string{"DEV"}, []string{devEnvironment.ID}, ""}, + {"finds an environment by ID", []string{"Environments-12"}, []string{devEnvironment.ID}, ""}, + {"finds several environments at once", []string{"Environments-12", "production"}, []string{devEnvironment.ID, prodEnvironment.ID}, ""}, + {"prefers an ID match over a name match", []string{"Environments-13"}, []string{prodEnvironment.ID}, ""}, + {"errors when nothing matches", []string{"Environments-404"}, nil, "cannot find an environment with the ID or name of 'Environments-404'"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*environments.Environment, error) { + return selectors.FindEnvironments(octopus, test.identifiers) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith(allEnvironments) + + result, err := testutil.ReceivePair(receiver) + if test.expectedErr == "" { + assert.Nil(t, err) + assert.Equal(t, test.expectedIDs, util.SliceTransform(result, func(env *environments.Environment) string { return env.ID })) + } else { + assert.EqualError(t, err, test.expectedErr) + } + }) + } +} + +func TestFindEnvironment(t *testing.T) { + devEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-12", "dev") + + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) (*environments.Environment, error) { + return selectors.FindEnvironment(octopus, "Environments-12") + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment}) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, devEnvironment.ID, result.ID) +} + +func TestFindChannel(t *testing.T) { + project := fixtures.NewProject(findSpaceID, findProjectID, "Fire Project", "Lifecycles-1", "ProjectGroups-1", "deploymentprocess-"+findProjectID) + + defaultChannel := fixtures.NewChannel(findSpaceID, "Channels-1", "Default", findProjectID) + betaChannel := fixtures.NewChannel(findSpaceID, "Channels-2", "Beta", findProjectID) + + // a channel which is *named* like an ID, to prove the precedence rule + decoyChannel := fixtures.NewChannel(findSpaceID, "Channels-3", "Channels-2", findProjectID) + + allChannels := []*channels.Channel{defaultChannel, betaChannel, decoyChannel} + + tests := []struct { + name string + identifier string + expectedID string + expectedErr string + }{ + {"finds a channel by name", "Beta", betaChannel.ID, ""}, + {"finds a channel by name, ignoring case", "beta", betaChannel.ID, ""}, + {"finds a channel by ID", "Channels-1", defaultChannel.ID, ""}, + {"prefers an ID match over a name match", "Channels-2", betaChannel.ID, ""}, + {"errors when nothing matches", "Channels-404", "", "cannot find a channel in project 'Fire Project' with the ID or name of 'Channels-404'"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) (*channels.Channel, error) { + return selectors.FindChannel(octopus, project, test.identifier) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+findProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: allChannels, + }) + + result, err := testutil.ReceivePair(receiver) + if test.expectedErr == "" { + assert.Nil(t, err) + assert.Equal(t, test.expectedID, result.ID) + } else { + assert.EqualError(t, err, test.expectedErr) + } + }) + } +} + +func TestFindTenants(t *testing.T) { + cokeTenant := fixtures.NewTenant(findSpaceID, "Tenants-29", "Coke", "Regions/us-east") + + t.Run("finds a tenant by ID", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*tenants.Tenant, error) { + return selectors.FindTenants(octopus, []string{"Tenants-29"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Tenants-29").RespondWith(cokeTenant) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []string{cokeTenant.ID}, util.SliceTransform(result, func(tenant *tenants.Tenant) string { return tenant.ID })) + }) + + t.Run("falls back to a name lookup when the ID doesn't exist", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*tenants.Tenant, error) { + return selectors.FindTenants(octopus, []string{"Coke"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{ + Items: []*tenants.Tenant{cokeTenant}, + }) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []string{cokeTenant.ID}, util.SliceTransform(result, func(tenant *tenants.Tenant) string { return tenant.ID })) + }) + + t.Run("errors when nothing matches", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*tenants.Tenant, error) { + return selectors.FindTenants(octopus, []string{"Tenants-404"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Tenants-404").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Tenants-404").RespondWith(resources.Resources[*tenants.Tenant]{}) + + _, err := testutil.ReceivePair(receiver) + assert.EqualError(t, err, "cannot find a tenant with the ID or name of 'Tenants-404'") + }) +} diff --git a/pkg/question/selectors/tenants.go b/pkg/question/selectors/tenants.go new file mode 100644 index 00000000..6b51d26d --- /dev/null +++ b/pkg/question/selectors/tenants.go @@ -0,0 +1,38 @@ +package selectors + +import ( + "errors" + "fmt" + + octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/services" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" +) + +// FindTenant looks a tenant up by either its ID or its name. +func FindTenant(octopus *octopusApiClient.Client, tenantIdentifier string) (*tenants.Tenant, error) { + tenant, err := octopus.Tenants.GetByIdentifier(tenantIdentifier) + if err != nil { + if errors.Is(err, services.ErrItemNotFound) { + return nil, fmt.Errorf("cannot find a tenant with the ID or name of '%s'", tenantIdentifier) + } + return nil, err + } + return tenant, nil +} + +// FindTenants looks tenants up by either their IDs or their names. +func FindTenants(octopus *octopusApiClient.Client, tenantIdentifiers []string) ([]*tenants.Tenant, error) { + if len(tenantIdentifiers) == 0 { + return nil, nil + } + result := make([]*tenants.Tenant, 0, len(tenantIdentifiers)) + for _, identifier := range tenantIdentifiers { + tenant, err := FindTenant(octopus, identifier) + if err != nil { + return nil, err + } + result = append(result, tenant) + } + return result, nil +} From aee96fe54df098aaff3a09444cab376acfee0086 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:02 +1000 Subject: [PATCH 14/26] fix: resolve tenant names with a paginated exact-match lookup `Tenants.GetByIdentifier`'s name fallback (`GetByName`) issues a single `tenants?partialName=` query and scans only the first page of the result. `partialName` is a contains filter, so an exact name that sorts past a page's worth of other tenants containing the same substring - e.g. `--tenant Smith` in a space full of `... Smith` tenants - came back as `ErrItemNotFound` and failed the deploy, even though the same name worked before this branch, when it was passed through and matched server side. `selectors.FindTenant` now does the ID lookup itself and walks every page of the partial name search looking for an exact match, keeping the same ID-beats-name precedence. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/question/selectors/find_test.go | 29 +++++++++++++++++++ pkg/question/selectors/tenants.go | 43 +++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 6 deletions(-) diff --git a/pkg/question/selectors/find_test.go b/pkg/question/selectors/find_test.go index 0ff5612f..83b208dd 100644 --- a/pkg/question/selectors/find_test.go +++ b/pkg/question/selectors/find_test.go @@ -181,6 +181,35 @@ func TestFindTenants(t *testing.T) { assert.Equal(t, []string{cokeTenant.ID}, util.SliceTransform(result, func(tenant *tenants.Tenant) string { return tenant.ID })) }) + t.Run("finds an exact name match beyond the first page of the partial name search", func(t *testing.T) { + // `partialName` is a contains filter, so a tenant exactly named "Smith" can be pushed off + // the first page by every other tenant whose name also contains "Smith" + aaronSmith := fixtures.NewTenant(findSpaceID, "Tenants-30", "Aaron Smith") + smith := fixtures.NewTenant(findSpaceID, "Tenants-31", "Smith") + + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*tenants.Tenant, error) { + return selectors.FindTenants(octopus, []string{"Smith"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Smith").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Smith").RespondWith(resources.Resources[*tenants.Tenant]{ + Items: []*tenants.Tenant{aaronSmith}, + PagedResults: resources.PagedResults{ + Links: resources.Links{PageNext: "/api/Spaces-1/tenants?partialName=Smith&skip=1"}, + }, + }) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Smith&skip=1").RespondWith(resources.Resources[*tenants.Tenant]{ + Items: []*tenants.Tenant{smith}, + }) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []string{smith.ID}, util.SliceTransform(result, func(tenant *tenants.Tenant) string { return tenant.ID })) + }) + t.Run("errors when nothing matches", func(t *testing.T) { api := testutil.NewMockHttpServer() receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*tenants.Tenant, error) { diff --git a/pkg/question/selectors/tenants.go b/pkg/question/selectors/tenants.go index 6b51d26d..ffb0af34 100644 --- a/pkg/question/selectors/tenants.go +++ b/pkg/question/selectors/tenants.go @@ -3,22 +3,53 @@ package selectors import ( "errors" "fmt" + "strings" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" - "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/services" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" ) -// FindTenant looks a tenant up by either its ID or its name. +// FindTenant looks a tenant up by either its ID or its name. An ID match wins over a name +// match, so it stays consistent with how projects, environments and channels resolve. +// +// Deliberately not Tenants.GetByIdentifier: its name fallback issues a single `partialName` +// (i.e. contains) query and only scans the first page of the result, so an exact name that +// sorts past that page is reported as not found. Names are on the deploy hot path and used +// to be resolved server side, so a miss here is a regression rather than an inconvenience. func FindTenant(octopus *octopusApiClient.Client, tenantIdentifier string) (*tenants.Tenant, error) { - tenant, err := octopus.Tenants.GetByIdentifier(tenantIdentifier) + if tenantIdentifier == "" { + return nil, errors.New("cannot find a tenant without an ID or name") + } + + tenant, err := octopus.Tenants.GetByID(tenantIdentifier) if err != nil { - if errors.Is(err, services.ErrItemNotFound) { - return nil, fmt.Errorf("cannot find a tenant with the ID or name of '%s'", tenantIdentifier) + var apiError *core.APIError + if errors.As(err, &apiError) && apiError.StatusCode != 404 { + return nil, err } + // a 404 (or an identifier that doesn't look like an ID at all) just means "try the name" + } else if tenant != nil { + return tenant, nil + } + + resultPage, err := octopus.Tenants.Get(tenants.TenantsQuery{PartialName: tenantIdentifier}) + if err != nil { return nil, err } - return tenant, nil + for resultPage != nil && len(resultPage.Items) > 0 { + for _, t := range resultPage.Items { // the server has no exact-name search, so we emulate one + if strings.EqualFold(t.Name, tenantIdentifier) { + return t, nil + } + } + resultPage, err = resultPage.GetNextPage(octopus.Tenants.GetClient()) + if err != nil { + return nil, err + } // if there are no more pages, GetNextPage returns nil, which breaks us out of the loop + } + + return nil, fmt.Errorf("cannot find a tenant with the ID or name of '%s'", tenantIdentifier) } // FindTenants looks tenants up by either their IDs or their names. From 88aba55dd018f9fd5a7176d58a672bd9f8a72339 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:13:29 +1000 Subject: [PATCH 15/26] fix: resolve environments one identifier at a time, and share the ephemeral fallback with runbook run The ephemeral fallback was all-or-nothing over the whole `--environment` list: a list mixing a regular and an ephemeral environment could never resolve, because the regular lookup errored on the ephemeral name and the ephemeral lookup then errored on the regular one, leaving the user with `cannot find an environment with the ID or name of ''` - blaming an environment that exists. It also fell back on *any* error from the regular lookup, including a transport failure. `selectors.ResolveEnvironmentNames` now resolves each identifier in turn against the regular environment list, consulting the ephemeral list only for identifiers that list doesn't have (fetched once, lazily). Single-type lists behave exactly as before; mixed lists resolve, and a genuine miss names the identifier that actually went missing. `runbook run` uses the same resolver, so an ephemeral environment name that used to be passed through to the server no longer fails client side. Also flips ephemeral name/ID indexing in `findEphemeralEnvironments` so an ID match wins a collision, matching the precedence everywhere else. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy.go | 22 ++---- pkg/cmd/runbook/run/run.go | 3 +- pkg/question/selectors/environments.go | 98 ++++++++++++++++++++++---- pkg/question/selectors/find_test.go | 57 +++++++++++++++ 4 files changed, 148 insertions(+), 32 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index 29e79f2a..4db42603 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -358,7 +358,7 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error // the executions API only matches environments by name, so resolve any IDs we were given if len(options.Environments) > 0 { - options.Environments, err = resolveEnvironmentNames(octopus, f.GetCurrentSpace(), options.Environments) + options.Environments, err = selectors.ResolveEnvironmentNames(octopus, f.GetCurrentSpace(), options.Environments) if err != nil { return err } @@ -701,9 +701,11 @@ func findEphemeralEnvironments(octopus *octopusApiClient.Client, space *spaces.S envMap := make(map[string]*ephemeralenvironments.EphemeralEnvironment, len(allEphemeralEnvironments.Items)*2) for _, ephemeralEnv := range allEphemeralEnvironments.Items { - envMap[strings.ToLower(ephemeralEnv.ID)] = ephemeralEnv envMap[strings.ToLower(ephemeralEnv.Name)] = ephemeralEnv } + for _, ephemeralEnv := range allEphemeralEnvironments.Items { // IDs go in second so an ID match wins a collision with another environment's name + envMap[strings.ToLower(ephemeralEnv.ID)] = ephemeralEnv + } for _, envIdentifier := range environmentIdentifiers { ephemeralEnv, found := envMap[strings.ToLower(envIdentifier)] @@ -716,22 +718,6 @@ func findEphemeralEnvironments(octopus *octopusApiClient.Client, space *spaces.S return selectedEnvironments, nil } -// resolveEnvironmentNames maps environment names or IDs onto canonical environment names, because -// the executions API only matches environments by name. Ephemeral environments aren't part of the -// regular environment list, so they're looked up separately when the regular lookup comes up empty. -func resolveEnvironmentNames(octopus *octopusApiClient.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { - selectedEnvironments, err := selectors.FindEnvironments(octopus, environmentIdentifiers) - if err == nil { - return util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }), nil - } - - ephemeralEnvironments, ephemeralErr := findEphemeralEnvironments(octopus, space, environmentIdentifiers) - if ephemeralErr != nil { - return nil, err // ephemeral environments are the rarer case; report why the regular lookup failed - } - return util.SliceTransform(ephemeralEnvironments, func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.Name }), nil -} - func selectDeploymentEnvironmentsForEphemeralChannel(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, options *executor.TaskOptionsDeployRelease, selectedRelease *releases.Release) ([]string, error) { var deploymentEnvironmentIds []string var selectedEnvironments []*ephemeralenvironments.EphemeralEnvironment diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index 0f67559a..12c45854 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -249,11 +249,10 @@ func runbookRun(cmd *cobra.Command, f factory.Factory, flags *RunFlags) error { // the executions API only matches environments and tenants by name, so resolve any IDs we were given if len(flags.Environments.Value) > 0 { - selectedEnvironments, err := selectors.FindEnvironments(octopus, flags.Environments.Value) + flags.Environments.Value, err = selectors.ResolveEnvironmentNames(octopus, f.GetCurrentSpace(), flags.Environments.Value) if err != nil { return err } - flags.Environments.Value = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) } if len(flags.Tenants.Value) > 0 { diff --git a/pkg/question/selectors/environments.go b/pkg/question/selectors/environments.go index 2176b400..b7a832ec 100644 --- a/pkg/question/selectors/environments.go +++ b/pkg/question/selectors/environments.go @@ -7,6 +7,8 @@ import ( "github.com/OctopusDeploy/cli/pkg/question" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments/v2/ephemeralenvironments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/spaces" ) type GetAllEnvironmentsCallback func() ([]*environments.Environment, error) @@ -56,21 +58,13 @@ func FindEnvironments(octopus *client.Client, environmentIdentifiers []string) ( if err != nil { return nil, err } - - idLookup := make(map[string]*environments.Environment, len(allEnvs)) - nameLookup := make(map[string]*environments.Environment, len(allEnvs)) - for _, env := range allEnvs { - idLookup[strings.ToLower(env.GetID())] = env - nameLookup[strings.ToLower(env.GetName())] = env - } + lookup := newIdentifierLookup(allEnvs, + func(env *environments.Environment) string { return env.GetID() }, + func(env *environments.Environment) string { return env.GetName() }) result := make([]*environments.Environment, 0, len(environmentIdentifiers)) for _, identifier := range environmentIdentifiers { - key := strings.ToLower(identifier) - env, found := idLookup[key] - if !found { - env, found = nameLookup[key] - } + env, found := lookup.find(identifier) if !found { return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) } @@ -79,6 +73,86 @@ func FindEnvironments(octopus *client.Client, environmentIdentifiers []string) ( return result, nil } +// ResolveEnvironmentNames maps environment names or IDs onto canonical environment names, because +// the executions API only matches environments by name. +// +// Ephemeral environments aren't part of the regular environment list, so that list is consulted - +// once, lazily - for any identifier the regular list doesn't have. Resolving one identifier at a +// time means a list mixing the two kinds still reports the identifier that actually went missing. +func ResolveEnvironmentNames(octopus *client.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { + if len(environmentIdentifiers) == 0 { + return nil, nil + } + allEnvs, err := octopus.Environments.GetAll() + if err != nil { + return nil, err + } + regular := newIdentifierLookup(allEnvs, + func(env *environments.Environment) string { return env.GetID() }, + func(env *environments.Environment) string { return env.GetName() }) + + var ephemeral *identifierLookup[*ephemeralenvironments.EphemeralEnvironment] + + names := make([]string, 0, len(environmentIdentifiers)) + for _, identifier := range environmentIdentifiers { + if env, found := regular.find(identifier); found { + names = append(names, env.GetName()) + continue + } + + if ephemeral == nil { + if space == nil { + return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) + } + allEphemeral, ephemeralErr := ephemeralenvironments.GetAll(octopus, space.ID) + if ephemeralErr != nil { + // ephemeral environments are the rarer case, and the endpoint doesn't exist on + // every server; either way the identifier is genuinely not a regular environment + return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) + } + lookup := newIdentifierLookup(allEphemeral.Items, + func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.ID }, + func(env *ephemeralenvironments.EphemeralEnvironment) string { return env.Name }) + ephemeral = &lookup + } + + if env, found := ephemeral.find(identifier); found { + names = append(names, env.Name) + continue + } + return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) + } + return names, nil +} + +// identifierLookup indexes items by both ID and name so an identifier can be matched against +// either, with an ID match winning when an item's name collides with another item's ID. +type identifierLookup[T any] struct { + byID map[string]T + byName map[string]T +} + +func newIdentifierLookup[T any](items []T, id func(T) string, name func(T) string) identifierLookup[T] { + lookup := identifierLookup[T]{ + byID: make(map[string]T, len(items)), + byName: make(map[string]T, len(items)), + } + for _, item := range items { + lookup.byID[strings.ToLower(id(item))] = item + lookup.byName[strings.ToLower(name(item))] = item + } + return lookup +} + +func (l identifierLookup[T]) find(identifier string) (T, bool) { + key := strings.ToLower(identifier) + if item, found := l.byID[key]; found { + return item, true + } + item, found := l.byName[key] + return item, found +} + func EnvironmentsMultiSelect(ask question.Asker, getAllEnvironmentsCallback GetAllEnvironmentsCallback, message string, required bool) ([]*environments.Environment, error) { allEnvs, err := getAllEnvironmentsCallback() if err != nil { diff --git a/pkg/question/selectors/find_test.go b/pkg/question/selectors/find_test.go index 83b208dd..daeded28 100644 --- a/pkg/question/selectors/find_test.go +++ b/pkg/question/selectors/find_test.go @@ -11,6 +11,7 @@ import ( "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments/v2/ephemeralenvironments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tenants" "github.com/stretchr/testify/assert" @@ -80,6 +81,62 @@ func TestFindEnvironments(t *testing.T) { } } +func TestResolveEnvironmentNames(t *testing.T) { + findSpace := fixtures.NewSpace(findSpaceID, "Default") + devEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-12", "dev") + ephemeralEnvironment := fixtures.NewEphemeralEnvironment(findSpaceID, "Environments-123", "Ephemeral Environment", "Environments-12") + + t.Run("resolves a mix of regular and ephemeral environments", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]string, error) { + return selectors.ResolveEnvironmentNames(octopus, findSpace, []string{"DEV", "Environments-123"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/v2?skip=0&take=2147483647&type=Ephemeral").RespondWith(resources.Resources[*ephemeralenvironments.EphemeralEnvironment]{ + Items: []*ephemeralenvironments.EphemeralEnvironment{ephemeralEnvironment}, + }) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []string{"dev", "Ephemeral Environment"}, result) + }) + + t.Run("doesn't look at ephemeral environments when everything resolves", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]string, error) { + return selectors.ResolveEnvironmentNames(octopus, findSpace, []string{"Environments-12"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment}) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []string{"dev"}, result) + }) + + t.Run("names the environment that is actually missing", func(t *testing.T) { + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]string, error) { + return selectors.ResolveEnvironmentNames(octopus, findSpace, []string{"dev", "Environments-404"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/v2?skip=0&take=2147483647&type=Ephemeral").RespondWith(resources.Resources[*ephemeralenvironments.EphemeralEnvironment]{ + Items: []*ephemeralenvironments.EphemeralEnvironment{ephemeralEnvironment}, + }) + + _, err := testutil.ReceivePair(receiver) + assert.EqualError(t, err, "cannot find an environment with the ID or name of 'Environments-404'") + }) +} + func TestFindEnvironment(t *testing.T) { devEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-12", "dev") From 4ba0de5ea21ad7f694f11dc1c5a44c9bec74e768 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:51:59 +1000 Subject: [PATCH 16/26] fix: keep the resolved environment identity for later runbook lookups `runbook run` resolved `--environment` to canonical names up front, then handed those names back to the ID-first `executionscommon.FindEnvironments` when picking run targets and when previewing prompted variables for a by-tag run. With the collision the selector tests already cover - environment A is `Environments-99`/`Environments-13` and environment B is `Environments-13`/`production` - `--environment Environments-99` resolved to A, and the second lookup then resolved A's name to B. The run still submitted A's name, but target selection and the prompted-variable check used B's preview. `selectors.ResolveEnvironments` now returns the ID and name of each environment an identifier picks out (`ResolveEnvironmentNames` is a thin wrapper for callers that only want names), and `runbook run` threads that resolved identity down through `runDbRunbook`/`runGitRunbook`/ `runRunbooksByTag` and into the Ask* questions, so nothing resolves an environment twice. The run-target helpers now take environment IDs, since that's all they ever used. The by-tag preview also stops re-listing every environment once per matching runbook. The name lookup is kept as a fallback for the exported `Ask*` entry points, which can be called without pre-resolved environments. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/runbook/run/run.go | 67 ++++++++++++++++---------- pkg/cmd/runbook/run/run_by_tag.go | 42 +++++++++------- pkg/cmd/runbook/run/run_test.go | 60 +++++++++++++++++++++++ pkg/question/selectors/environments.go | 34 ++++++++++--- pkg/question/selectors/find_test.go | 22 +++++++++ 5 files changed, 175 insertions(+), 50 deletions(-) diff --git a/pkg/cmd/runbook/run/run.go b/pkg/cmd/runbook/run/run.go index 12c45854..458638c0 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -247,12 +247,17 @@ func runbookRun(cmd *cobra.Command, f factory.Factory, flags *RunFlags) error { flags.Project.Value = project.Name - // the executions API only matches environments and tenants by name, so resolve any IDs we were given + // the executions API only matches environments and tenants by name, so resolve any IDs we were given. + // Run previews and target selection need the IDs, so keep the whole resolved identity rather than + // looking the names up again later - an ID-first lookup of a name that collides with another + // environment's ID would land on the other environment. + var resolvedEnvironments []*selectors.ResolvedEnvironment if len(flags.Environments.Value) > 0 { - flags.Environments.Value, err = selectors.ResolveEnvironmentNames(octopus, f.GetCurrentSpace(), flags.Environments.Value) + resolvedEnvironments, err = selectors.ResolveEnvironments(octopus, f.GetCurrentSpace(), flags.Environments.Value) if err != nil { return err } + flags.Environments.Value = util.SliceTransform(resolvedEnvironments, func(env *selectors.ResolvedEnvironment) string { return env.Name }) } if len(flags.Tenants.Value) > 0 { @@ -299,20 +304,20 @@ func runbookRun(cmd *cobra.Command, f factory.Factory, flags *RunFlags) error { if len(flags.RunbookTags.Value) > 0 { if shared.AreRunbooksInGit(project) { - return runRunbooksByTag(cmd, f, flags, octopus, project, parsedVariables, outputFormat, true) + return runRunbooksByTag(cmd, f, flags, octopus, project, resolvedEnvironments, parsedVariables, outputFormat, true) } else { - return runRunbooksByTag(cmd, f, flags, octopus, project, parsedVariables, outputFormat, false) + return runRunbooksByTag(cmd, f, flags, octopus, project, resolvedEnvironments, parsedVariables, outputFormat, false) } } if shared.AreRunbooksInGit(project) { - return runGitRunbook(cmd, f, flags, octopus, project, parsedVariables, outputFormat) + return runGitRunbook(cmd, f, flags, octopus, project, resolvedEnvironments, parsedVariables, outputFormat) } else { - return runDbRunbook(cmd, f, flags, octopus, project, parsedVariables, outputFormat) + return runDbRunbook(cmd, f, flags, octopus, project, resolvedEnvironments, parsedVariables, outputFormat) } } -func runDbRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, parsedVariables map[string]string, outputFormat string) error { +func runDbRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, resolvedEnvironments []*selectors.ResolvedEnvironment, parsedVariables map[string]string, outputFormat string) error { commonOptions := &executor.TaskOptionsRunbookRunBase{ ProjectName: project.Name, @@ -351,7 +356,7 @@ func runDbRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopu } } - err := AskDbRunbookRunQuestions(octopus, cmd.OutOrStdout(), f.Ask, f.GetCurrentSpace(), project, options, now) + err := AskDbRunbookRunQuestions(octopus, cmd.OutOrStdout(), f.Ask, f.GetCurrentSpace(), project, options, resolvedEnvironments, now) if err != nil { return err } @@ -452,7 +457,7 @@ func runDbRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopu return nil } -func runGitRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, parsedVariables map[string]string, outputFormat string) error { +func runGitRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, resolvedEnvironments []*selectors.ResolvedEnvironment, parsedVariables map[string]string, outputFormat string) error { commonOptions := &executor.TaskOptionsRunbookRunBase{ ProjectName: project.Name, @@ -494,7 +499,7 @@ func runGitRunbook(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octop } } - err := AskGitRunbookRunQuestions(octopus, cmd.OutOrStdout(), f.Ask, f.GetCurrentSpace(), project, options, now) + err := AskGitRunbookRunQuestions(octopus, cmd.OutOrStdout(), f.Ask, f.GetCurrentSpace(), project, options, resolvedEnvironments, now) if err != nil { return err } @@ -699,7 +704,7 @@ func askCommonAdvancedOptions( return nil } -func AskDbRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, space *spaces.Space, project *projects.Project, options *executor.TaskOptionsRunbookRun, now func() time.Time) error { +func AskDbRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, space *spaces.Space, project *projects.Project, options *executor.TaskOptionsRunbookRun, resolvedEnvironments []*selectors.ResolvedEnvironment, now func() time.Time) error { if octopus == nil { return cliErrors.NewArgumentNullOrEmptyError("octopus") } @@ -737,14 +742,18 @@ func AskDbRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer return err } - // machine selection later on needs to refer back to the environments. + // machine selection later on needs to refer back to the environments. When the environments came + // from the command line the caller has already resolved them, so reuse those IDs rather than + // resolving options.Environments - now canonical names - a second time. var selectedEnvironments []*environments.Environment + environmentIDs := util.SliceTransform(resolvedEnvironments, func(env *selectors.ResolvedEnvironment) string { return env.ID }) if len(options.Environments) == 0 { selectedEnvironments, err = selectRunEnvironments(asker, octopus, space, project, selectedRunbook) if err != nil { return err } options.Environments = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) + environmentIDs = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.ID }) } else { _, _ = fmt.Fprintf(stdout, "Environments %s\n", output.Cyan(strings.Join(options.Environments, ","))) } @@ -846,14 +855,15 @@ func AskDbRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer } if !isRunTargetsSpecified { - if len(selectedEnvironments) == 0 { // if the Q&A process earlier hasn't loaded environments already, we need to load them now - selectedEnvironments, err = executionscommon.FindEnvironments(octopus, options.Environments) + if len(environmentIDs) == 0 { // if the Q&A process earlier hasn't loaded environments already, we need to load them now + envs, err := executionscommon.FindEnvironments(octopus, options.Environments) if err != nil { return err } + environmentIDs = util.SliceTransform(envs, func(env *environments.Environment) string { return env.ID }) } - options.RunTargets, err = askRunbookTargets(octopus, asker, space.ID, selectedSnapshot.ID, selectedEnvironments) + options.RunTargets, err = askRunbookTargets(octopus, asker, space.ID, selectedSnapshot.ID, environmentIDs) if err != nil { return err } @@ -877,7 +887,7 @@ func AskDbRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer return nil } -func AskGitRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, space *spaces.Space, project *projects.Project, options *executor.TaskOptionsGitRunbookRun, now func() time.Time) error { +func AskGitRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, space *spaces.Space, project *projects.Project, options *executor.TaskOptionsGitRunbookRun, resolvedEnvironments []*selectors.ResolvedEnvironment, now func() time.Time) error { if octopus == nil { return cliErrors.NewArgumentNullOrEmptyError("octopus") } @@ -926,14 +936,18 @@ func AskGitRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Write return err } - // machine selection later on needs to refer back to the environments. + // machine selection later on needs to refer back to the environments. When the environments came + // from the command line the caller has already resolved them, so reuse those IDs rather than + // resolving options.Environments - now canonical names - a second time. var selectedEnvironments []*environments.Environment + environmentIDs := util.SliceTransform(resolvedEnvironments, func(env *selectors.ResolvedEnvironment) string { return env.ID }) if len(options.Environments) == 0 { selectedEnvironments, err = selectGitRunEnvironments(asker, octopus, space, project, selectedRunbook, options.GitReference) if err != nil { return err } options.Environments = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) + environmentIDs = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.ID }) } else { _, _ = fmt.Fprintf(stdout, "Environments %s\n", output.Cyan(strings.Join(options.Environments, ","))) } @@ -1069,14 +1083,15 @@ func AskGitRunbookRunQuestions(octopus *octopusApiClient.Client, stdout io.Write } if !isRunTargetsSpecified { - if len(selectedEnvironments) == 0 { // if the Q&A process earlier hasn't loaded environments already, we need to load them now - selectedEnvironments, err = executionscommon.FindEnvironments(octopus, options.Environments) + if len(environmentIDs) == 0 { // if the Q&A process earlier hasn't loaded environments already, we need to load them now + envs, err := executionscommon.FindEnvironments(octopus, options.Environments) if err != nil { return err } + environmentIDs = util.SliceTransform(envs, func(env *environments.Environment) string { return env.ID }) } - options.RunTargets, err = askGitRunbookTargets(octopus, asker, space.ID, project.ID, selectedRunbook.ID, options.GitReference, selectedEnvironments) + options.RunTargets, err = askGitRunbookTargets(octopus, asker, space.ID, project.ID, selectedRunbook.ID, options.GitReference, environmentIDs) if err != nil { return err } @@ -1208,11 +1223,11 @@ func resolveRunbookPreviewVariables( return result, sensitiveVars, nil } -func askRunbookTargets(octopus *octopusApiClient.Client, asker question.Asker, spaceID string, runbookSnapshotID string, selectedEnvironments []*environments.Environment) ([]string, error) { +func askRunbookTargets(octopus *octopusApiClient.Client, asker question.Asker, spaceID string, runbookSnapshotID string, environmentIDs []string) ([]string, error) { var results []string - for _, env := range selectedEnvironments { - preview, err := runbooks.GetRunbookSnapshotRunPreview(octopus, spaceID, runbookSnapshotID, env.ID, true) + for _, environmentID := range environmentIDs { + preview, err := runbooks.GetRunbookSnapshotRunPreview(octopus, spaceID, runbookSnapshotID, environmentID, true) if err != nil { return nil, err } @@ -1245,11 +1260,11 @@ func askRunbookTargets(octopus *octopusApiClient.Client, asker question.Asker, s return nil, nil } -func askGitRunbookTargets(octopus *octopusApiClient.Client, asker question.Asker, spaceID string, projectID string, runbookID string, gitRef string, selectedEnvironments []*environments.Environment) ([]string, error) { +func askGitRunbookTargets(octopus *octopusApiClient.Client, asker question.Asker, spaceID string, projectID string, runbookID string, gitRef string, environmentIDs []string) ([]string, error) { var results []string - for _, env := range selectedEnvironments { - preview, err := runbooks.GetGitRunbookRunPreview(octopus, spaceID, projectID, runbookID, gitRef, env.ID, true) + for _, environmentID := range environmentIDs { + preview, err := runbooks.GetGitRunbookRunPreview(octopus, spaceID, projectID, runbookID, gitRef, environmentID, true) if err != nil { return nil, err } diff --git a/pkg/cmd/runbook/run/run_by_tag.go b/pkg/cmd/runbook/run/run_by_tag.go index d281bff5..21df5cbd 100644 --- a/pkg/cmd/runbook/run/run_by_tag.go +++ b/pkg/cmd/runbook/run/run_by_tag.go @@ -16,6 +16,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/factory" "github.com/OctopusDeploy/cli/pkg/output" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/question/selectors" "github.com/OctopusDeploy/cli/pkg/util" "github.com/OctopusDeploy/cli/pkg/util/flag" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" @@ -166,7 +167,7 @@ func processRunbookTasks(octopus *octopusApiClient.Client, space *spaces.Space, return results } -func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, parsedVariables map[string]string, outputFormat string, isGit bool) error { +func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, octopus *octopusApiClient.Client, project *projects.Project, resolvedEnvironments []*selectors.ResolvedEnvironment, parsedVariables map[string]string, outputFormat string, isGit bool) error { var allRunbooks []*runbooks.Runbook var err error @@ -197,6 +198,10 @@ func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, oc cmd.Println() } + // the caller has already resolved any environments given on the command line; keep their IDs so + // the run previews below don't have to resolve the canonical names a second time + environmentIDs := util.SliceTransform(resolvedEnvironments, func(env *selectors.ResolvedEnvironment) string { return env.ID }) + var selectedEnvironments []*environments.Environment if f.IsPromptEnabled() { if len(flags.Environments.Value) == 0 { @@ -209,6 +214,7 @@ func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, oc return err } flags.Environments.Value = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.Name }) + environmentIDs = util.SliceTransform(selectedEnvironments, func(env *environments.Environment) string { return env.ID }) } if len(flags.Tenants.Value) == 0 && len(flags.TenantTags.Value) == 0 { @@ -226,27 +232,27 @@ func runRunbooksByTag(cmd *cobra.Command, f factory.Factory, flags *RunFlags, oc // Check if any runbooks have prompted variables - block execution if found if len(parsedVariables) == 0 { + if len(environmentIDs) == 0 { // nothing was pre-resolved, so fall back to a name lookup + envs, err := executionscommon.FindEnvironments(octopus, flags.Environments.Value[:1]) + if err == nil { + environmentIDs = util.SliceTransform(envs, func(env *environments.Environment) string { return env.ID }) + } + } + // one preview per runbook is enough to spot prompted variables, so only the first environment is used + previewEnvironmentID := "" + if len(environmentIDs) > 0 { + previewEnvironmentID = environmentIDs[0] + } + hasPromptedVars := false var runbookWithPrompts string for _, runbook := range matchingRunbooks { var preview *runbooks.RunPreview - if isGit { - // Get preview for first environment to check for prompted variables - if len(flags.Environments.Value) > 0 { - envs, err := executionscommon.FindEnvironments(octopus, flags.Environments.Value[:1]) - if err == nil && len(envs) > 0 { - preview, _ = runbooks.GetGitRunbookRunPreview(octopus, f.GetCurrentSpace().ID, project.ID, runbook.ID, flags.GitRef.Value, envs[0].ID, true) - } - } - } else { - // For DB runbooks, we need the published snapshot - if runbook.PublishedRunbookSnapshotID != "" { - if len(flags.Environments.Value) > 0 { - envs, err := executionscommon.FindEnvironments(octopus, flags.Environments.Value[:1]) - if err == nil && len(envs) > 0 { - preview, _ = runbooks.GetRunbookSnapshotRunPreview(octopus, f.GetCurrentSpace().ID, runbook.PublishedRunbookSnapshotID, envs[0].ID, true) - } - } + if previewEnvironmentID != "" { + if isGit { + preview, _ = runbooks.GetGitRunbookRunPreview(octopus, f.GetCurrentSpace().ID, project.ID, runbook.ID, flags.GitRef.Value, previewEnvironmentID, true) + } else if runbook.PublishedRunbookSnapshotID != "" { // for DB runbooks, we need the published snapshot + preview, _ = runbooks.GetRunbookSnapshotRunPreview(octopus, f.GetCurrentSpace().ID, runbook.PublishedRunbookSnapshotID, previewEnvironmentID, true) } } if preview != nil && len(preview.Form.Elements) > 0 { diff --git a/pkg/cmd/runbook/run/run_test.go b/pkg/cmd/runbook/run/run_test.go index bd8f3326..fbcc42f5 100644 --- a/pkg/cmd/runbook/run/run_test.go +++ b/pkg/cmd/runbook/run/run_test.go @@ -804,6 +804,66 @@ func TestGitRunbookRun_AutomationMode(t *testing.T) { } } +// --environment is resolved once, up front, and the resolved identity has to be carried through to +// the run preview. Looking the canonical name up again would go through an ID-first lookup and land +// on whichever environment happens to have that name as its ID. +func TestRunbookRunByTag_UsesTheResolvedEnvironmentForThePreview(t *testing.T) { + const spaceID = "Spaces-1" + const fireProjectID = "Projects-22" + + space1 := fixtures.NewSpace(spaceID, "Default Space") + fireProject := fixtures.NewProject(spaceID, fireProjectID, "Fire Project", "Lifecycles-1", "ProjectGroups-1", "deploymentprocess-"+fireProjectID) + + devEnvironment := fixtures.NewEnvironment(spaceID, "Environments-12", "dev") + testEnvironment := fixtures.NewEnvironment(spaceID, "Environments-13", "test") + // an environment which is *named* like another environment's ID + decoyEnvironment := fixtures.NewEnvironment(spaceID, "Environments-99", "Environments-13") + + nightlyRunbook := fixtures.NewRunbook(spaceID, fireProjectID, "Runbooks-1", "Provision Database") + nightlyRunbook.RunbookTags = []string{"nightly"} + nightlyRunbook.PublishedRunbookSnapshotID = "RunbookSnapshots-1" + + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + api := testutil.NewMockHttpServer() + rootCmd := cmdRoot.NewCmdRoot(testutil.NewMockFactoryWithSpace(api, space1), nil, nil) + rootCmd.SetContext(ctxWithFakeNow) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"runbook", "run", "--project", "Fire Project", "--runbook-tag", "nightly", "--environment", "Environments-99"}) + 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)) + // the one and only environment lookup; the decoy wins because an ID match beats a name match + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment, decoyEnvironment}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Projects-22/runbooks?take=2147483647").RespondWith(resources.Resources[*runbooks.Runbook]{ + Items: []*runbooks.Runbook{nightlyRunbook}, + }) + // Environments-99, not Environments-13: the preview must use the environment we actually resolved + api.ExpectRequest(t, "GET", "/api/Spaces-1/runbookSnapshots/RunbookSnapshots-1/runbookRuns/preview/Environments-99?includeDisabledSteps=true"). + RespondWith(&runbooks.RunPreview{Form: deployments.NewFormWithValuesAndElements(map[string]string{}, []*deployments.Element{})}) + + 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) + // the executions API only matches by name, so the decoy's name is what gets submitted + assert.Equal(t, []string{"Environments-13"}, requestBody.EnvironmentNames) + req.RespondWith(&runbooks.RunbookRunResponseV1{ + RunbookRunServerTasks: []*runbooks.RunbookRunServerTask{ + {RunbookRunID: "RunbookRun-203", ServerTaskID: "ServerTasks-29394"}, + }, + }) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Equal(t, "", stderr.String()) +} + func TestRunbookRun_PrintAdvancedSummary(t *testing.T) { tests := []struct { name string diff --git a/pkg/question/selectors/environments.go b/pkg/question/selectors/environments.go index b7a832ec..1419a10d 100644 --- a/pkg/question/selectors/environments.go +++ b/pkg/question/selectors/environments.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/util" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments/v2/ephemeralenvironments" @@ -73,13 +74,34 @@ func FindEnvironments(octopus *client.Client, environmentIdentifiers []string) ( return result, nil } +// ResolvedEnvironment is an environment identified by both its ID and its name, so callers that +// need the name (the executions API only matches environments by name) and callers that need the +// ID (run/deployment previews, target selection) can share a single lookup. Resolving twice isn't +// safe: an ID-first lookup of a name that happens to be another environment's ID lands on the +// other environment. +type ResolvedEnvironment struct { + ID string + Name string +} + // ResolveEnvironmentNames maps environment names or IDs onto canonical environment names, because -// the executions API only matches environments by name. +// the executions API only matches environments by name. Prefer ResolveEnvironments when the caller +// also needs the environment's ID later on. +func ResolveEnvironmentNames(octopus *client.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { + resolved, err := ResolveEnvironments(octopus, space, environmentIdentifiers) + if err != nil { + return nil, err + } + return util.SliceTransform(resolved, func(env *ResolvedEnvironment) string { return env.Name }), nil +} + +// ResolveEnvironments maps environment names or IDs onto the ID and name of the environment each +// one picks out. // // Ephemeral environments aren't part of the regular environment list, so that list is consulted - // once, lazily - for any identifier the regular list doesn't have. Resolving one identifier at a // time means a list mixing the two kinds still reports the identifier that actually went missing. -func ResolveEnvironmentNames(octopus *client.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { +func ResolveEnvironments(octopus *client.Client, space *spaces.Space, environmentIdentifiers []string) ([]*ResolvedEnvironment, error) { if len(environmentIdentifiers) == 0 { return nil, nil } @@ -93,10 +115,10 @@ func ResolveEnvironmentNames(octopus *client.Client, space *spaces.Space, enviro var ephemeral *identifierLookup[*ephemeralenvironments.EphemeralEnvironment] - names := make([]string, 0, len(environmentIdentifiers)) + resolved := make([]*ResolvedEnvironment, 0, len(environmentIdentifiers)) for _, identifier := range environmentIdentifiers { if env, found := regular.find(identifier); found { - names = append(names, env.GetName()) + resolved = append(resolved, &ResolvedEnvironment{ID: env.GetID(), Name: env.GetName()}) continue } @@ -117,12 +139,12 @@ func ResolveEnvironmentNames(octopus *client.Client, space *spaces.Space, enviro } if env, found := ephemeral.find(identifier); found { - names = append(names, env.Name) + resolved = append(resolved, &ResolvedEnvironment{ID: env.ID, Name: env.Name}) continue } return nil, fmt.Errorf("cannot find an environment with the ID or name of '%s'", identifier) } - return names, nil + return resolved, nil } // identifierLookup indexes items by both ID and name so an identifier can be matched against diff --git a/pkg/question/selectors/find_test.go b/pkg/question/selectors/find_test.go index daeded28..e813b263 100644 --- a/pkg/question/selectors/find_test.go +++ b/pkg/question/selectors/find_test.go @@ -137,6 +137,28 @@ func TestResolveEnvironmentNames(t *testing.T) { }) } +func TestResolveEnvironments(t *testing.T) { + findSpace := fixtures.NewSpace(findSpaceID, "Default") + prodEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-13", "production") + // an environment which is *named* like another environment's ID + decoyEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-99", "Environments-13") + + // resolving the returned name a second time would hit the ID index and land on prodEnvironment, + // which is why callers need to keep the ID alongside the name + api := testutil.NewMockHttpServer() + receiver := beginRequest(api, func(octopus *octopusApiClient.Client) ([]*selectors.ResolvedEnvironment, error) { + return selectors.ResolveEnvironments(octopus, findSpace, []string{"Environments-99"}) + }) + + api.ExpectRequest(t, "GET", "/api/").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/spaces").RespondWith(findRootResource) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{prodEnvironment, decoyEnvironment}) + + result, err := testutil.ReceivePair(receiver) + assert.Nil(t, err) + assert.Equal(t, []*selectors.ResolvedEnvironment{{ID: "Environments-99", Name: "Environments-13"}}, result) +} + func TestFindEnvironment(t *testing.T) { devEnvironment := fixtures.NewEnvironment(findSpaceID, "Environments-12", "dev") From de14d35d3da860528bd54f9670eab44334e884de Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 15 Sep 2026 16:58:53 +1000 Subject: [PATCH 17/26] test: expect the environment lookup in the --priority cases The --priority tests arrived on main (#708) after this branch was cut, so they were the only deploy cases not already expecting the environments/all lookup this branch adds. Same one-line expectation as every other case here. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 4acedb1b..b2dbb9d9 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -2133,6 +2133,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) @@ -2171,6 +2172,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) From 266fe8c2a04007af568ac1012c52d390ab8458a9 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:42:06 +1000 Subject: [PATCH 18/26] 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 19/26] 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 20/26] 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 21/26] 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) +} From 2517c551e7a7469d2c97ba58621cab73e56b7927 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 15:06:35 +1000 Subject: [PATCH 22/26] test: add integration tests for the tier 1 release fixes Covers behaviour that only a real server exercises: unknown release versions, packages with no version in their feed, channel and environment IDs on the executions API, and comma-separated deployment targets. Refs #294, #426, #250, #556 Co-Authored-By: Claude Opus 5 (1M context) --- test/integration/release_test.go | 299 +++++++++++++++++++++++++++++++ 1 file changed, 299 insertions(+) diff --git a/test/integration/release_test.go b/test/integration/release_test.go index b6f476f3..5f439555 100644 --- a/test/integration/release_test.go +++ b/test/integration/release_test.go @@ -8,13 +8,19 @@ import ( octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/environments" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/lifecycles" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/packages" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/releases" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/tasks" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "os/exec" "testing" + "time" ) const space1ID = "Spaces-1" @@ -256,3 +262,296 @@ func TestReleaseListAndDelete(t *testing.T) { // the error struct contains an error message, but the server can/will change this over time, and we don't particularly care about it; 404 statuscode is the important bit }) } + +func createEnvironment(t *testing.T, apiClient *octopusApiClient.Client, name string) *environments.Environment { + environment, err := apiClient.Environments.Add(environments.NewEnvironment(name)) + if !testutil.AssertSuccess(t, err) { + return nil + } + t.Cleanup(func() { assert.Nil(t, apiClient.Environments.DeleteByID(environment.GetID())) }) + return environment +} + +func createCloudRegionTarget(t *testing.T, apiClient *octopusApiClient.Client, name string, environmentID string) *machines.DeploymentTarget { + target, err := apiClient.Machines.Add(machines.NewDeploymentTarget(name, machines.NewCloudRegionEndpoint(), []string{environmentID}, []string{"deploy"})) + if !testutil.AssertSuccess(t, err) { + return nil + } + t.Cleanup(func() { assert.Nil(t, apiClient.Machines.DeleteByID(target.GetID())) }) + return target +} + +// allowDeploymentsTo replaces the fixture lifecycle's phases with a single phase for the +// given environment, so releases in the project can be deployed to it. +func allowDeploymentsTo(t *testing.T, apiClient *octopusApiClient.Client, lifecycle *lifecycles.Lifecycle, environmentID string) bool { + phase := lifecycles.NewPhase("phase1") + phase.OptionalDeploymentTargets = []string{environmentID} + lifecycle.Phases = []*lifecycles.Phase{phase} + updated, err := apiClient.Lifecycles.Update(lifecycle) + if !testutil.AssertSuccess(t, err) { + return false + } + t.Cleanup(func() { + updated.Phases = nil + _, err := apiClient.Lifecycles.Update(updated) + assert.Nil(t, err) + }) + return true +} + +// waitForTaskToComplete blocks until the deployment's server task finishes; the project cannot be +// deleted while it is still running. Whether it succeeded is not this test's concern. +func waitForTaskToComplete(t *testing.T, apiClient *octopusApiClient.Client, taskID string) { + deadline := time.Now().Add(2 * time.Minute) + for time.Now().Before(deadline) { + found, err := apiClient.Tasks.Get(tasks.TasksQuery{IDs: []string{taskID}}) + if !testutil.AssertSuccess(t, err) { + return + } + if len(found.Items) == 1 && found.Items[0].IsCompleted != nil && *found.Items[0].IsCompleted { + return + } + time.Sleep(2 * time.Second) + } + t.Errorf("timed out waiting for task %s to complete", taskID) +} + +// scriptStep builds a single inline script step. With no target roles it runs on the server, so +// the project is deployable without any deployment targets. +func scriptStep(name string, targetRoles string) *deployments.DeploymentStep { + stepProperties := map[string]core.PropertyValue{} + action := &deployments.DeploymentAction{ + ActionType: "Octopus.Script", + Name: name, + Properties: map[string]core.PropertyValue{ + "Octopus.Action.Script.ScriptBody": core.NewPropertyValue("echo 'hello'", false), + }, + } + if targetRoles != "" { + stepProperties["Octopus.Action.TargetRoles"] = core.NewPropertyValue(targetRoles, false) + } else { + action.Properties["Octopus.Action.RunOnServer"] = core.NewPropertyValue("true", false) + } + return &deployments.DeploymentStep{Name: name, Properties: stepProperties, Actions: []*deployments.DeploymentAction{action}} +} + +// packageStep builds a single package step. The server rejects a package on an inline script, +// so a release that needs a package version has to go through this step type. +func packageStep(name string, targetRoles string, packageID string) *deployments.DeploymentStep { + return &deployments.DeploymentStep{ + Name: name, + Properties: map[string]core.PropertyValue{"Octopus.Action.TargetRoles": core.NewPropertyValue(targetRoles, false)}, + Actions: []*deployments.DeploymentAction{ + { + ActionType: "Octopus.TentaclePackage", + Name: name, + Properties: map[string]core.PropertyValue{}, + Packages: []*packages.PackageReference{ + { + PackageID: packageID, + FeedID: "feeds-builtin", + AcquisitionLocation: "Server", + Properties: map[string]string{"SelectionMode": "immediate"}, + }, + }, + }, + }, + } +} + +func setDeploymentProcess(t *testing.T, apiClient *octopusApiClient.Client, project *projects.Project, step *deployments.DeploymentStep) bool { + deploymentProcess, err := apiClient.DeploymentProcesses.Get(project, "") + if !testutil.AssertSuccess(t, err) { + return false + } + deploymentProcess.Steps = []*deployments.DeploymentStep{step} + _, err = apiClient.DeploymentProcesses.Update(deploymentProcess) + return testutil.AssertSuccess(t, err) +} + +func onlyReleaseInProject(t *testing.T, apiClient *octopusApiClient.Client, project *projects.Project) *releases.Release { + projectReleases, err := apiClient.Projects.GetReleases(project) + if !testutil.AssertSuccess(t, err) { + return nil + } + require.Equal(t, 1, len(projectReleases)) + return projectReleases[0] +} + +func onlyDeploymentOfRelease(t *testing.T, apiClient *octopusApiClient.Client, release *releases.Release) *deployments.Deployment { + releaseDeployments, err := apiClient.Deployments.GetDeployments(release) + if !testutil.AssertSuccess(t, err) { + return nil + } + require.Equal(t, 1, len(releaseDeployments.Items)) + return releaseDeployments.Items[0] +} + +// The executions API reports an unknown release version poorly - as a null reference error on the +// servers in issue #294, and as a bare "was not found" on current ones - so the CLI resolves the +// version up front and says what it looked for. +func TestReleaseDeployUnknownVersion(t *testing.T) { + runId := uuid.New() + apiClient, err := integration.GetApiClient(space1ID) + testutil.RequireSuccess(t, err) + + fx, err := integration.CreateCommonProject(t, apiClient, runId) + testutil.RequireSuccess(t, err) + project := fx.Project + + environment := createEnvironment(t, apiClient, fmt.Sprintf("env-%s", runId)) + if environment == nil { + return + } + + t.Run("the API does not answer with a usable release", func(t *testing.T) { + release, err := releases.GetReleaseInProject(apiClient, space1ID, project.GetID(), "9.9.9") + assert.True(t, err != nil || release == nil || release.GetID() == "") + }) + + t.Run("deploy names the version it could not find", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "deploy", "--project", project.Name, "--version", "9.9.9", "--environment", environment.Name) + assert.Error(t, err) + assert.Contains(t, stdErr, fmt.Sprintf("cannot find a release with version '9.9.9' in project '%s'", project.Name)) + assert.NotContains(t, stdErr, "Object reference not set") + }) + + t.Run("deploy reports that latest is not an alias", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "deploy", "--project", project.Name, "--version", "latest", "--environment", environment.Name) + assert.Error(t, err) + assert.Contains(t, stdErr, "'latest' is not a supported alias") + assert.NotContains(t, stdErr, "Object reference not set") + }) +} + +// A package with no version in its feed fails the release with no indication of which package is +// at fault, so the CLI diagnoses the failure and names them. See issue #426. +func TestReleaseCreateMissingPackageVersion(t *testing.T) { + runId := uuid.New() + apiClient, err := integration.GetApiClient(space1ID) + testutil.RequireSuccess(t, err) + + fx, err := integration.CreateCommonProject(t, apiClient, runId) + testutil.RequireSuccess(t, err) + project := fx.Project + + stepName := fmt.Sprintf("step-%s", runId) + packageID := fmt.Sprintf("package-%s", runId) + if !setDeploymentProcess(t, apiClient, project, packageStep(stepName, "deploy", packageID)) { + return + } + + t.Run("create names the package that has no version", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name) + assert.Error(t, err) + assert.Contains(t, stdErr, "no version could be found for the following packages") + assert.Contains(t, stdErr, packageID) + assert.Contains(t, stdErr, stepName) + assert.NotContains(t, stdErr, "Object reference not set") + }) +} + +// The executions API matches channels and environments by name only, so the CLI resolves IDs +// before sending them. See issue #250. +func TestReleaseCreateAndDeployByID(t *testing.T) { + runId := uuid.New() + apiClient, err := integration.GetApiClient(space1ID) + testutil.RequireSuccess(t, err) + + fx, err := integration.CreateCommonProject(t, apiClient, runId) + testutil.RequireSuccess(t, err) + project := fx.Project + + environment := createEnvironment(t, apiClient, fmt.Sprintf("env-%s", runId)) + if environment == nil { + return + } + if !allowDeploymentsTo(t, apiClient, fx.Lifecycle, environment.GetID()) { + return + } + if !setDeploymentProcess(t, apiClient, project, scriptStep(fmt.Sprintf("step-%s", runId), "")) { + return + } + t.Cleanup(func() { deleteAllReleasesInProject(t, apiClient, project) }) + + t.Run("create accepts a channel ID", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name, "--channel", fx.ProjectDefaultChannel.GetID(), "--version", "1.0.0") + if !testutil.AssertSuccess(t, err, stdErr) { + return + } + release := onlyReleaseInProject(t, apiClient, project) + if release == nil { + return + } + assert.Equal(t, fx.ProjectDefaultChannel.GetID(), release.ChannelID) + }) + + t.Run("deploy accepts an environment ID", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "deploy", "--project", project.Name, "--version", "1.0.0", "--environment", environment.GetID()) + if !testutil.AssertSuccess(t, err, stdErr) { + return + } + release := onlyReleaseInProject(t, apiClient, project) + if release == nil { + return + } + deployment := onlyDeploymentOfRelease(t, apiClient, release) + if deployment == nil { + return + } + assert.Equal(t, environment.GetID(), deployment.EnvironmentID) + waitForTaskToComplete(t, apiClient, deployment.TaskID) + }) +} + +// Comma-separated values are split before they reach the executions API, which otherwise reports +// the whole string as one unknown target. See issue #556. +func TestReleaseDeployCommaSeparatedTargets(t *testing.T) { + runId := uuid.New() + apiClient, err := integration.GetApiClient(space1ID) + testutil.RequireSuccess(t, err) + + fx, err := integration.CreateCommonProject(t, apiClient, runId) + testutil.RequireSuccess(t, err) + project := fx.Project + + environment := createEnvironment(t, apiClient, fmt.Sprintf("env-%s", runId)) + if environment == nil { + return + } + if !allowDeploymentsTo(t, apiClient, fx.Lifecycle, environment.GetID()) { + return + } + if !setDeploymentProcess(t, apiClient, project, scriptStep(fmt.Sprintf("step-%s", runId), "deploy")) { + return + } + + targetA := createCloudRegionTarget(t, apiClient, fmt.Sprintf("target-a-%s", runId), environment.GetID()) + targetB := createCloudRegionTarget(t, apiClient, fmt.Sprintf("target-b-%s", runId), environment.GetID()) + if targetA == nil || targetB == nil { + return + } + t.Cleanup(func() { deleteAllReleasesInProject(t, apiClient, project) }) + + _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name, "--version", "1.0.0") + if !testutil.AssertSuccess(t, err, stdErr) { + return + } + + t.Run("deploy splits a comma-separated target list", func(t *testing.T) { + _, stdErr, err := integration.RunCli(space1ID, "release", "deploy", "--project", project.Name, "--version", "1.0.0", "--environment", environment.Name, "--deployment-target", fmt.Sprintf("%s,%s", targetA.Name, targetB.Name)) + if !testutil.AssertSuccess(t, err, stdErr) { + return + } + release := onlyReleaseInProject(t, apiClient, project) + if release == nil { + return + } + deployment := onlyDeploymentOfRelease(t, apiClient, release) + if deployment == nil { + return + } + assert.ElementsMatch(t, []string{targetA.GetID(), targetB.GetID()}, deployment.SpecificMachineIDs) + waitForTaskToComplete(t, apiClient, deployment.TaskID) + }) +} From 27617796c6633a63820a683f35840f6a0563cb8b Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:46:43 +1000 Subject: [PATCH 23/26] test: reconcile the deploy and runbook expectations across the tier 1 fixes Each of the four fixes is green on its own branch, but merged they change each other's request sequences, and the unit tests that merged cleanly are the ones that break. Nothing here is a defect in an individual PR; it is ordinary merge fallout, recorded because whichever lands last will hit it. - #294 adds a release pre-flight lookup and #250 an environments/all lookup ahead of the deployment POST. The tests added by #556, and the --priority tests that arrived on main in #708, merged without conflict and went looking for the POST, finding a GET. For the tenanted comma test the result was a hang rather than a failure: MockHttpServer blocks waiting for a request the CLI no longer makes in that order. - #250's "specifying project, environment and tenant by ID" still expected the two post-deploy web-URL lookups that #294 drops; the pre-flight now supplies the release ID, so no request follows the POST. - #556's runbook comma test needed #250's environments/all lookup, which runbook run performs unconditionally. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy_test.go | 21 +++++++++++++++------ pkg/cmd/runbook/run/run_test.go | 1 + 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index f8bfa7a8..ce36d2e6 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -1847,6 +1847,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { // an account allowed to deploy but not to read releases must not be blocked by the pre-flight lookup api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0"). RespondWithStatus(403, "403 Forbidden", &core.APIError{ErrorMessage: "You do not have permission to perform this action."}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) @@ -1934,6 +1935,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/"+cokeTenant.ID).RespondWith(cokeTenant) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID).RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") @@ -1958,12 +1960,7 @@ func TestDeployCreate_AutomationMode(t *testing.T) { }, }) - // now it's going to try and look up the project/version to generate the web URL - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWithStatus(404, "NotFound", nil) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects?partialName=Fire+Project").RespondWith(resources.Resources[*projects.Project]{ - Items: []*projects.Project{fireProject}, - }) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + // no further requests: the pre-flight lookup already gave us the release ID for the web URL _, err = testutil.ReceivePair(cmdReceiver) assert.Nil(t, err) @@ -2470,6 +2467,8 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) @@ -2516,7 +2515,15 @@ func TestDeployCreate_AutomationMode(t *testing.T) { api.ExpectRequest(t, "GET", "/api/").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) + // the comma form must resolve each tenant individually, exactly as the repeated form does + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Coke").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Coke").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{cokeTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants/Pepsi").RespondWithStatus(404, "NotFound", nil) + api.ExpectRequest(t, "GET", "/api/Spaces-1/tenants?partialName=Pepsi").RespondWith(resources.Resources[*tenants.Tenant]{Items: []*tenants.Tenant{pepsiTenant}}) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProject.GetName()).RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/tenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentTenantedCommandV1](req.Request.Body) @@ -2563,6 +2570,8 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0").RespondWith(release10) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/deployments/create/untenanted/v1") requestBody, err := testutil.ReadJson[deployments.CreateDeploymentUntenantedCommandV1](req.Request.Body) diff --git a/pkg/cmd/runbook/run/run_test.go b/pkg/cmd/runbook/run/run_test.go index 5d92da7d..8bae3e7a 100644 --- a/pkg/cmd/runbook/run/run_test.go +++ b/pkg/cmd/runbook/run/run_test.go @@ -425,6 +425,7 @@ func TestRunbookRun_AutomationMode(t *testing.T) { 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)) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) req := api.ExpectRequest(t, "POST", "/api/Spaces-1/runbook-runs/create/v1") requestBody, err := testutil.ReadJson[runbooks.RunbookRunCommandV1](req.Request.Body) From 8ddd0afe146b5673809552d9d454bee925d453a3 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:46:58 +1000 Subject: [PATCH 24/26] fix: only a confirmed missing release aborts the deploy pre-flight FindRelease reports an empty response body as an unconfirmed ReleaseNotFoundError, because the SDK decodes a bodyless response as a zero-valued release with no error whatever its status code. Aborting on that turns a bodyless 403, or a proxy's 502, into a failed deployment that previously went straight to the executions API. Require Confirmed before treating the pre-flight as fatal; otherwise leave the release ID unset and let the deployment endpoint decide. The 'latest' alias test now answers with the APIError body a real server sends for a missing version (checked on 2026.3.14820), so it still exercises the confirmed rejection path. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/deploy/deploy.go | 12 +-- pkg/cmd/release/deploy/deploy_test.go | 112 ++++++++++++++++---------- 2 files changed, 76 insertions(+), 48 deletions(-) diff --git a/pkg/cmd/release/deploy/deploy.go b/pkg/cmd/release/deploy/deploy.go index d3976adb..bb34ca83 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -369,13 +369,15 @@ func deployRun(cmd *cobra.Command, f factory.Factory, flags *DeployFlags) error if options.ReleaseVersion != "" { // resolve the release up front; the executions API reports an unknown version as an // unhelpful null reference error, and having the ID saves looking it up again later. - // Only a "no such release" answer is fatal: this lookup is new to the deploy path, so - // anything else (no ReleaseView permission, a transient 5xx) must not fail a deploy - // that would previously have succeeded. In those cases the server stays the authority - // and we simply go without the release ID. + // Only a *confirmed* "no such release" is fatal: this lookup is new to the deploy path, + // so anything else must not fail a deploy that would previously have succeeded. That + // covers both a non-not-found error (no ReleaseView permission, a transient 5xx) and an + // unconfirmed ReleaseNotFoundError, which is what an empty response body decodes to + // whatever its status code was - a bodyless 403 or a proxy's 502 included. In those + // cases the server stays the authority and we simply go without the release ID. release, err := selectors.FindRelease(octopus, f.GetCurrentSpace().ID, project, options.ReleaseVersion) var releaseNotFound *selectors.ReleaseNotFoundError - if errors.As(err, &releaseNotFound) { + if errors.As(err, &releaseNotFound) && releaseNotFound.Confirmed { return err } if err == nil { diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index ce36d2e6..d4ddb489 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -1735,6 +1735,53 @@ func TestDeployCreate_AutomationMode(t *testing.T) { cokeTenant := fixtures.NewTenant(spaceID, "Tenants-29", "Coke", "Regions/us-east", "Importance/High") pepsiTenant := fixtures.NewTenant(spaceID, "Tenants-37", "Pepsi", "Regions/us-east", "Importance/Low") + // deploysDespiteFailedReleaseLookup asserts that a deployment still reaches the executions API when + // the pre-flight release lookup fails. Only a confirmed "no such release" may stop a deployment that + // would previously have gone straight to the server, so the cases using this differ only in how the + // lookup fails; respondToLookup answers the release request. + deploysDespiteFailedReleaseLookup := func(respondToLookup func(lookup *testutil.RequestWrapper)) func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + return 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"}) + 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) + + respondToLookup(api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0")) + api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) + + 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"}, + 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) + + // no release ID, so no web link; the deployment itself still went ahead + assert.Equal(t, "Successfully started 1 deployment(s)\n", stdOut.String()) + assert.Equal(t, "", stdErr.String()) + } + } + // TEST STARTS HERE tests := []struct { name string @@ -1824,57 +1871,36 @@ func TestDeployCreate_AutomationMode(t *testing.T) { 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) - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/latest").RespondWithStatus(404, "NotFound", nil) + // a real server answers this with a 404 carrying an APIError body, so the version is + // confirmed missing and the pre-flight is allowed to stop the deployment + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/latest"). + RespondWithStatus(404, "404 Not Found", &core.APIError{ErrorMessage: "The resource you requested was not found."}) _, err := testutil.ReceivePair(cmdReceiver) - assert.EqualError(t, err, "could not resolve a release with version 'latest' in project 'Fire Project'; the server returned an empty response, which usually means there is no such release, but can also mean the lookup itself failed. 'latest' is not a supported alias, specify an exact version. Run 'octopus release list --project \"Fire Project\"' to see the available versions") + assert.EqualError(t, err, "cannot find a release with version 'latest' in project 'Fire Project'. 'latest' is not a supported alias, specify an exact version. Run 'octopus release list --project \"Fire Project\"' to see the available versions") assert.Equal(t, "", stdOut.String()) assert.Equal(t, "", stdErr.String()) }}, - {"release deploy proceeds when the release lookup fails for a reason other than not-found", 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"}) - 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) - + {"release deploy proceeds when the release lookup is forbidden with an error body", deploysDespiteFailedReleaseLookup( // an account allowed to deploy but not to read releases must not be blocked by the pre-flight lookup - api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/releases/1.0"). - RespondWithStatus(403, "403 Forbidden", &core.APIError{ErrorMessage: "You do not have permission to perform this action."}) - api.ExpectRequest(t, "GET", "/api/Spaces-1/environments/all").RespondWith([]*environments.Environment{devEnvironment, testEnvironment}) - - 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"}, - 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) - - // no release ID, so no web link; the deployment itself still went ahead - assert.Equal(t, "Successfully started 1 deployment(s)\n", stdOut.String()) - assert.Equal(t, "", stdErr.String()) - }}, + func(lookup *testutil.RequestWrapper) { + lookup.RespondWithStatus(403, "403 Forbidden", &core.APIError{ErrorMessage: "You do not have permission to perform this action."}) + })}, + + {"release deploy proceeds when the release lookup is forbidden with an empty body", deploysDespiteFailedReleaseLookup( + // an empty body decodes as a zero-valued release with no error, so this arrives as an + // unconfirmed ReleaseNotFoundError rather than on the error path + func(lookup *testutil.RequestWrapper) { + lookup.RespondWithStatus(403, "403 Forbidden", nil) + })}, + + {"release deploy proceeds when the release lookup hits an empty-bodied gateway error", deploysDespiteFailedReleaseLookup( + // e.g. a reverse proxy in front of the server answering with Content-Length: 0 + func(lookup *testutil.RequestWrapper) { + lookup.RespondWithStatus(502, "502 Bad Gateway", nil) + })}, {"release deploy specifying project, version, env only (bare minimum) assuming untenanted", func(t *testing.T, api *testutil.MockHttpServer, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { From f020d5377b0cc506b05ba93a015b3ac1d9e7dcdc Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:50:05 +1000 Subject: [PATCH 25/26] test: prove the requested channel ID is honoured, not just accepted The channel-ID case asserted that the release landed in the project's only channel, which is also where the server puts a release when no channel is requested - so it passed whether or not --channel reached the server. Add a non-default channel and assert the release lands there. Verified against a live instance (localhost:8065, Spaces-1): the test passes as written, and fails with "expected Channels-353, actual Channels-352" when release create is patched to drop the resolved channel, which the old assertion did not detect. Co-Authored-By: Claude Opus 5 (1M context) --- test/integration/release_test.go | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/test/integration/release_test.go b/test/integration/release_test.go index 5f439555..7c0bbf19 100644 --- a/test/integration/release_test.go +++ b/test/integration/release_test.go @@ -272,6 +272,20 @@ func createEnvironment(t *testing.T, apiClient *octopusApiClient.Client, name st return environment } +// createSecondaryChannel adds a non-default channel to the project. It carries no lifecycle of its +// own, so it inherits the project's, and no version rules, so any release version is valid in it. +// It exists separately from the fixture's default channel so that a test asserting on the channel a +// release landed in can tell a channel the CLI asked for from the one the server would have picked. +func createSecondaryChannel(t *testing.T, apiClient *octopusApiClient.Client, project *projects.Project, name string) *channels.Channel { + channel, err := apiClient.Channels.Add(channels.NewChannel(name, project.GetID())) + if !testutil.AssertSuccess(t, err) { + return nil + } + t.Cleanup(func() { assert.Nil(t, apiClient.Channels.DeleteByID(channel.GetID())) }) + require.False(t, channel.IsDefault, "the project's default channel must stay the one the fixture created") + return channel +} + func createCloudRegionTarget(t *testing.T, apiClient *octopusApiClient.Client, name string, environmentID string) *machines.DeploymentTarget { target, err := apiClient.Machines.Add(machines.NewDeploymentTarget(name, machines.NewCloudRegionEndpoint(), []string{environmentID}, []string{"deploy"})) if !testutil.AssertSuccess(t, err) { @@ -472,10 +486,16 @@ func TestReleaseCreateAndDeployByID(t *testing.T) { if !setDeploymentProcess(t, apiClient, project, scriptStep(fmt.Sprintf("step-%s", runId), "")) { return } + // the release has to land in a channel the server wouldn't have chosen by itself, otherwise the + // assertion below passes just as well when --channel is dropped on the floor + secondaryChannel := createSecondaryChannel(t, apiClient, project, fmt.Sprintf("channel-%s", runId)) + if secondaryChannel == nil { + return + } t.Cleanup(func() { deleteAllReleasesInProject(t, apiClient, project) }) t.Run("create accepts a channel ID", func(t *testing.T) { - _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name, "--channel", fx.ProjectDefaultChannel.GetID(), "--version", "1.0.0") + _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name, "--channel", secondaryChannel.GetID(), "--version", "1.0.0") if !testutil.AssertSuccess(t, err, stdErr) { return } @@ -483,7 +503,8 @@ func TestReleaseCreateAndDeployByID(t *testing.T) { if release == nil { return } - assert.Equal(t, fx.ProjectDefaultChannel.GetID(), release.ChannelID) + assert.Equal(t, secondaryChannel.GetID(), release.ChannelID) + assert.NotEqual(t, fx.ProjectDefaultChannel.GetID(), release.ChannelID) }) t.Run("deploy accepts an environment ID", func(t *testing.T) { From 0bce93bb302d3e56de4bb77ab6b913c0ccd6a9fe Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 15 Sep 2026 17:42:57 +1000 Subject: [PATCH 26/26] test: reconcile the diagnosis channel-by-ID case with the identifier work nj/issue-426 grew this case after the previous merge, so the reconcile commit never saw it. With nj/issue-250 in the tree, a --channel given as an ID is resolved against the project's channels before the release is posted, so the case needs that lookup like every other channel case here. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/release/create/create_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 30be76cc..3792b867 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -3160,6 +3160,10 @@ func TestReleaseCreate_AutomationMode_MissingPackageDiagnosis(t *testing.T) { api.ExpectRequest(t, "GET", "/api/Spaces-1").RespondWith(rootResource) api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/Fire Project").RespondWith(fireProject) + api.ExpectRequest(t, "GET", "/api/Spaces-1/projects/"+fireProjectID+"/channels").RespondWith(resources.Resources[*channels.Channel]{ + Items: []*channels.Channel{defaultChannel}, + }) + api.ExpectRequest(t, "POST", "/api/Spaces-1/releases/create/v1"). RespondWithStatus(http.StatusInternalServerError, "500 Internal Server Error", nullReferenceError)