From e083816260f4429a38444d19d8a86bfb11beb069 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 12:24:27 +1000 Subject: [PATCH 1/4] 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 0df6d614..9779dd6e 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" ) @@ -237,6 +238,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 @@ -319,6 +329,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 @@ -474,18 +491,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 }) } } @@ -622,7 +642,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 @@ -632,8 +652,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 } @@ -643,17 +663,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 @@ -721,6 +757,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 fde01017..c2eab692 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, @@ -1542,7 +1542,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 { @@ -1612,6 +1617,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") @@ -1652,6 +1658,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() @@ -1662,6 +1722,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") @@ -1712,6 +1779,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{ @@ -1742,6 +1810,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{ @@ -1772,7 +1841,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) @@ -1822,6 +1897,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) @@ -1888,6 +1964,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") @@ -1961,7 +2038,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 ad57eb89..000c665e 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" ) @@ -228,6 +229,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 33c1904d..82173da2 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) @@ -299,6 +359,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) @@ -367,6 +428,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 @@ -435,6 +502,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") @@ -453,6 +521,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") @@ -493,6 +562,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{ @@ -522,6 +592,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"}, @@ -553,6 +624,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) @@ -593,6 +669,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) @@ -651,6 +728,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 4348d7bd..8e3d9827 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" @@ -431,40 +432,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 93ab1c651e8f3170f6df623393f95f73c5101b2c Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:02 +1000 Subject: [PATCH 2/4] 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 3d8333ba4a17d8b312287ba6c86b2240cfeb39a4 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:13:29 +1000 Subject: [PATCH 3/4] 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 9779dd6e..c5080efa 100644 --- a/pkg/cmd/release/deploy/deploy.go +++ b/pkg/cmd/release/deploy/deploy.go @@ -331,7 +331,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 } @@ -659,9 +659,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)] @@ -674,22 +676,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 000c665e..ff13cd72 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -231,11 +231,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 956f44505b1fd786113f7d2a53e287f50886a82d Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:51:59 +1000 Subject: [PATCH 4/4] 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 ff13cd72..da72cadb 100644 --- a/pkg/cmd/runbook/run/run.go +++ b/pkg/cmd/runbook/run/run.go @@ -229,12 +229,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 { @@ -281,20 +286,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, @@ -330,7 +335,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 } @@ -425,7 +430,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, @@ -464,7 +469,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 } @@ -663,7 +668,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") } @@ -701,14 +706,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, ","))) } @@ -809,14 +818,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 } @@ -826,7 +836,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") } @@ -875,14 +885,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, ","))) } @@ -1017,14 +1031,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 } @@ -1142,11 +1157,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 } @@ -1179,11 +1194,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 f72a030f..7138779a 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 82173da2..f64e8bb8 100644 --- a/pkg/cmd/runbook/run/run_test.go +++ b/pkg/cmd/runbook/run/run_test.go @@ -792,6 +792,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")