From 328af8c242fa00bf22221bd8cedcf8cf498dbb40 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 15 Sep 2026 16:19:26 +1000 Subject: [PATCH 1/2] refactor: generalise the enable/disable helper over targets and workers Moves the shared enable/disable logic out of pkg/cmd/target/shared and into pkg/machinescommon, behind a Machine interface and a MachineKind describing one machine repository. Deployment targets keep the behaviour and output they had; workers can now reuse it. The noun in the prompt, the "already enabled" message and the "no ... to disable were found" error all come from the kind, so there is one copy of the flow rather than one per machine type. Also parameterises the --disabled help text by noun, the way RegisterCreateTargetProxyFlags already does. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/target/azure-web-app/create/create.go | 2 +- pkg/cmd/target/cloud-region/create/create.go | 2 +- pkg/cmd/target/disable/disable.go | 7 +- pkg/cmd/target/enable/enable.go | 7 +- pkg/cmd/target/kubernetes/create/create.go | 2 +- .../listening-tentacle/create/create.go | 2 +- pkg/cmd/target/shared/disabledstate.go | 130 ---------- pkg/cmd/target/shared/disabledstate_test.go | 100 -------- pkg/cmd/target/ssh/create/create.go | 2 +- pkg/machinescommon/disabled.go | 6 +- pkg/machinescommon/disabledstate.go | 236 ++++++++++++++++++ pkg/machinescommon/disabledstate_test.go | 137 ++++++++++ 12 files changed, 390 insertions(+), 243 deletions(-) delete mode 100644 pkg/cmd/target/shared/disabledstate.go delete mode 100644 pkg/cmd/target/shared/disabledstate_test.go create mode 100644 pkg/machinescommon/disabledstate.go create mode 100644 pkg/machinescommon/disabledstate_test.go diff --git a/pkg/cmd/target/azure-web-app/create/create.go b/pkg/cmd/target/azure-web-app/create/create.go index 21d72f8e..092e861d 100644 --- a/pkg/cmd/target/azure-web-app/create/create.go +++ b/pkg/cmd/target/azure-web-app/create/create.go @@ -125,7 +125,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { shared.RegisterCreateTargetRoleFlags(cmd, createFlags.CreateTargetRoleFlags) shared.RegisterCreateTargetTenantFlags(cmd, createFlags.CreateTargetTenantFlags) shared.RegisterCreateTargetWorkerPoolFlags(cmd, createFlags.WorkerPoolFlags) - machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "deployment target") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd } diff --git a/pkg/cmd/target/cloud-region/create/create.go b/pkg/cmd/target/cloud-region/create/create.go index 30b987bb..ca470e42 100644 --- a/pkg/cmd/target/cloud-region/create/create.go +++ b/pkg/cmd/target/cloud-region/create/create.go @@ -86,7 +86,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { shared.RegisterCreateTargetRoleFlags(cmd, createFlags.CreateTargetRoleFlags) shared.RegisterCreateTargetWorkerPoolFlags(cmd, createFlags.WorkerPoolFlags) shared.RegisterCreateTargetTenantFlags(cmd, createFlags.CreateTargetTenantFlags) - machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "deployment target") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd diff --git a/pkg/cmd/target/disable/disable.go b/pkg/cmd/target/disable/disable.go index bcccc737..130ec423 100644 --- a/pkg/cmd/target/disable/disable.go +++ b/pkg/cmd/target/disable/disable.go @@ -3,9 +3,9 @@ package disable import ( "github.com/MakeNowJust/heredoc/v2" "github.com/OctopusDeploy/cli/pkg/cmd" - "github.com/OctopusDeploy/cli/pkg/cmd/target/shared" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" + "github.com/OctopusDeploy/cli/pkg/machinescommon" "github.com/OctopusDeploy/cli/pkg/usage" "github.com/spf13/cobra" ) @@ -21,8 +21,9 @@ func NewCmdDisable(f factory.Factory) *cobra.Command { %[1]s deployment-target disable 'web-server' `, constants.ExecutableName), RunE: func(c *cobra.Command, args []string) error { - opts := shared.NewSetDisabledStateOptions(args, cmd.NewDependencies(f, c), true) - return shared.SetDisabledState(opts) + dependencies := cmd.NewDependencies(f, c) + opts := machinescommon.NewSetDisabledStateOptions(args, dependencies, machinescommon.NewDeploymentTargetKind(dependencies), true) + return machinescommon.SetDisabledState(opts) }, } } diff --git a/pkg/cmd/target/enable/enable.go b/pkg/cmd/target/enable/enable.go index afa42f9e..835c5af3 100644 --- a/pkg/cmd/target/enable/enable.go +++ b/pkg/cmd/target/enable/enable.go @@ -3,9 +3,9 @@ package enable import ( "github.com/MakeNowJust/heredoc/v2" "github.com/OctopusDeploy/cli/pkg/cmd" - "github.com/OctopusDeploy/cli/pkg/cmd/target/shared" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" + "github.com/OctopusDeploy/cli/pkg/machinescommon" "github.com/OctopusDeploy/cli/pkg/usage" "github.com/spf13/cobra" ) @@ -21,8 +21,9 @@ func NewCmdEnable(f factory.Factory) *cobra.Command { %[1]s deployment-target enable 'web-server' `, constants.ExecutableName), RunE: func(c *cobra.Command, args []string) error { - opts := shared.NewSetDisabledStateOptions(args, cmd.NewDependencies(f, c), false) - return shared.SetDisabledState(opts) + dependencies := cmd.NewDependencies(f, c) + opts := machinescommon.NewSetDisabledStateOptions(args, dependencies, machinescommon.NewDeploymentTargetKind(dependencies), false) + return machinescommon.SetDisabledState(opts) }, } } diff --git a/pkg/cmd/target/kubernetes/create/create.go b/pkg/cmd/target/kubernetes/create/create.go index f4d506b3..13606c14 100644 --- a/pkg/cmd/target/kubernetes/create/create.go +++ b/pkg/cmd/target/kubernetes/create/create.go @@ -309,7 +309,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { shared.RegisterCreateTargetWorkerPoolFlags(cmd, createFlags.WorkerPoolFlags) shared.RegisterCreateTargetTenantFlags(cmd, createFlags.CreateTargetTenantFlags) shared.RegisterCreateTargetRoleFlags(cmd, createFlags.CreateTargetRoleFlags) - machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "deployment target") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd diff --git a/pkg/cmd/target/listening-tentacle/create/create.go b/pkg/cmd/target/listening-tentacle/create/create.go index f76acd78..47594af8 100644 --- a/pkg/cmd/target/listening-tentacle/create/create.go +++ b/pkg/cmd/target/listening-tentacle/create/create.go @@ -101,7 +101,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { machinescommon.RegisterCreateTargetProxyFlags(cmd, createFlags.CreateTargetProxyFlags, "Listening Tentacle") machinescommon.RegisterCreateTargetMachinePolicyFlags(cmd, createFlags.CreateTargetMachinePolicyFlags) shared.RegisterCreateTargetTenantFlags(cmd, createFlags.CreateTargetTenantFlags) - machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "deployment target") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd diff --git a/pkg/cmd/target/shared/disabledstate.go b/pkg/cmd/target/shared/disabledstate.go deleted file mode 100644 index 513afd9a..00000000 --- a/pkg/cmd/target/shared/disabledstate.go +++ /dev/null @@ -1,130 +0,0 @@ -package shared - -import ( - "errors" - "fmt" - - "github.com/OctopusDeploy/cli/pkg/cmd" - "github.com/OctopusDeploy/cli/pkg/output" - "github.com/OctopusDeploy/cli/pkg/question" - "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" -) - -type SetDisabledStateOptions struct { - *cmd.Dependencies - *GetTargetsOptions - IdOrName string - // Disabled is the state the deployment target should end up in. - Disabled bool - // Target is the deployment target chosen at the prompt. When set it is used directly, saving a - // round trip back to the server for something we already have. - Target *machines.DeploymentTarget -} - -func NewSetDisabledStateOptions(args []string, dependencies *cmd.Dependencies, disabled bool) *SetDisabledStateOptions { - idOrName := "" - if len(args) > 0 { - idOrName = args[0] - } - - // Only targets that aren't already in the requested state are worth offering. The machines - // endpoint can filter server-side for the enable case (the query field is omitempty, so only - // isDisabled=true can be expressed); the disable case is filtered client-side below. - query := machines.MachinesQuery{IsDisabled: !disabled} - - return &SetDisabledStateOptions{ - Dependencies: dependencies, - GetTargetsOptions: NewGetTargetsOptions(dependencies, query), - IdOrName: idOrName, - Disabled: disabled, - } -} - -// SetDisabledState enables or disables a deployment target, prompting for the target when no -// name or ID was supplied. -func SetDisabledState(opts *SetDisabledStateOptions) error { - if !opts.NoPrompt { - if err := PromptMissingTarget(opts); err != nil { - return err - } - } - - target := opts.Target - if target == nil { - if opts.IdOrName == "" { - return errors.New("deployment target identifier is required but was not provided") - } - - var err error - if target, err = opts.Client.Machines.GetByIdentifier(opts.IdOrName); err != nil { - return err - } - } - - state := disabledStateDescription(opts.Disabled) - if target.IsDisabled == opts.Disabled { - _, _ = fmt.Fprintf(opts.Out, "Deployment target '%s' %s is already %s.\n", target.Name, output.Dimf("(%s)", target.GetID()), state) - return nil - } - - target.IsDisabled = opts.Disabled - if _, err := machines.Update(opts.Client, target); err != nil { - return err - } - - _, _ = fmt.Fprintf(opts.Out, "Successfully %s deployment target '%s' %s.\n", state, target.Name, output.Dimf("(%s)", target.GetID())) - return nil -} - -func PromptMissingTarget(opts *SetDisabledStateOptions) error { - if opts.IdOrName != "" { - return nil - } - - targets, err := opts.GetTargetsCallback() - if err != nil { - return err - } - - // The server-side filter isn't guaranteed (older servers, and the disable case can't express - // it), so drop anything already in the requested state here as well. - candidates := make([]*machines.DeploymentTarget, 0, len(targets)) - for _, target := range targets { - if target.IsDisabled != opts.Disabled { - candidates = append(candidates, target) - } - } - - if len(candidates) == 0 { - return fmt.Errorf("no deployment targets to %s were found", actionDescription(opts.Disabled)) - } - - // deliberately not selectors.Select: that auto-selects when there is exactly one target, which - // would mutate the target without the user ever being asked. Enable/disable always asks. - selectedTarget, err := question.SelectMap( - opts.Ask, - fmt.Sprintf("Select the deployment target you wish to %s:", actionDescription(opts.Disabled)), - candidates, - func(target *machines.DeploymentTarget) string { return target.Name }) - if err != nil { - return err - } - - opts.Target = selectedTarget - opts.IdOrName = selectedTarget.GetID() - return nil -} - -func actionDescription(isDisabled bool) string { - if isDisabled { - return "disable" - } - return "enable" -} - -func disabledStateDescription(isDisabled bool) string { - if isDisabled { - return "disabled" - } - return "enabled" -} diff --git a/pkg/cmd/target/shared/disabledstate_test.go b/pkg/cmd/target/shared/disabledstate_test.go deleted file mode 100644 index 091f8f02..00000000 --- a/pkg/cmd/target/shared/disabledstate_test.go +++ /dev/null @@ -1,100 +0,0 @@ -package shared_test - -import ( - "testing" - - "github.com/OctopusDeploy/cli/pkg/cmd" - "github.com/OctopusDeploy/cli/pkg/cmd/target/shared" - "github.com/OctopusDeploy/cli/test/fixtures" - "github.com/OctopusDeploy/cli/test/testutil" - "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" - "github.com/stretchr/testify/assert" -) - -func TestPromptMissingTarget_IdentifierSupplied(t *testing.T) { - pa := []*testutil.PA{} - - asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) - opts := shared.NewSetDisabledStateOptions([]string{"Machines-1"}, &cmd.Dependencies{Ask: asker}, true) - - err := shared.PromptMissingTarget(opts) - checkRemainingPrompts() - - assert.NoError(t, err) - assert.Equal(t, "Machines-1", opts.IdOrName) -} - -func TestPromptMissingTarget_NoIdentifierSupplied(t *testing.T) { - pa := []*testutil.PA{ - testutil.NewSelectPrompt("Select the deployment target you wish to disable:", "", []string{"web-server", "db-server"}, "db-server"), - } - - asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) - opts := shared.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, true) - opts.GetTargetsCallback = func() ([]*machines.DeploymentTarget, error) { - return []*machines.DeploymentTarget{ - fixtures.NewDeploymentTarget("Spaces-1", "Machines-1", "web-server", false), - fixtures.NewDeploymentTarget("Spaces-1", "Machines-2", "db-server", false), - }, nil - } - - err := shared.PromptMissingTarget(opts) - checkRemainingPrompts() - - assert.NoError(t, err) - assert.Equal(t, "Machines-2", opts.IdOrName) -} - -func TestPromptMissingTarget_EnableUsesEnableWording(t *testing.T) { - pa := []*testutil.PA{ - testutil.NewSelectPrompt("Select the deployment target you wish to enable:", "", []string{"web-server", "db-server"}, "web-server"), - } - - asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) - opts := shared.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, false) - opts.GetTargetsCallback = func() ([]*machines.DeploymentTarget, error) { - return []*machines.DeploymentTarget{ - fixtures.NewDeploymentTarget("Spaces-1", "Machines-1", "web-server", true), - fixtures.NewDeploymentTarget("Spaces-1", "Machines-2", "db-server", true), - }, nil - } - - err := shared.PromptMissingTarget(opts) - checkRemainingPrompts() - - assert.NoError(t, err) - assert.Equal(t, "Machines-1", opts.IdOrName) -} - -// selectors.Select auto-selects when there is exactly one item; enable/disable must not do that -// because it would mutate the only target in the space without asking. -func TestPromptMissingTarget_AsksEvenWhenThereIsOnlyOneTarget(t *testing.T) { - pa := []*testutil.PA{ - testutil.NewSelectPrompt("Select the deployment target you wish to disable:", "", []string{"web-server"}, "web-server"), - } - - asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) - opts := shared.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, true) - opts.GetTargetsCallback = func() ([]*machines.DeploymentTarget, error) { - return []*machines.DeploymentTarget{fixtures.NewDeploymentTarget("Spaces-1", "Machines-1", "web-server", false)}, nil - } - - err := shared.PromptMissingTarget(opts) - checkRemainingPrompts() - - assert.NoError(t, err) - assert.Equal(t, "Machines-1", opts.IdOrName) -} - -func TestPromptMissingTarget_ErrorsWhenNoTargetIsInTheOppositeState(t *testing.T) { - asker, checkRemainingPrompts := testutil.NewMockAsker(t, []*testutil.PA{}) - opts := shared.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, true) - opts.GetTargetsCallback = func() ([]*machines.DeploymentTarget, error) { - return []*machines.DeploymentTarget{fixtures.NewDeploymentTarget("Spaces-1", "Machines-1", "web-server", true)}, nil - } - - err := shared.PromptMissingTarget(opts) - checkRemainingPrompts() - - assert.EqualError(t, err, "no deployment targets to disable were found") -} diff --git a/pkg/cmd/target/ssh/create/create.go b/pkg/cmd/target/ssh/create/create.go index c1fe2954..05a91e83 100644 --- a/pkg/cmd/target/ssh/create/create.go +++ b/pkg/cmd/target/ssh/create/create.go @@ -102,7 +102,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { machinescommon.RegisterCreateTargetProxyFlags(cmd, createFlags.CreateTargetProxyFlags, "SSH target") machinescommon.RegisterCreateTargetMachinePolicyFlags(cmd, createFlags.CreateTargetMachinePolicyFlags) shared.RegisterCreateTargetTenantFlags(cmd, createFlags.CreateTargetTenantFlags) - machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "deployment target") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd diff --git a/pkg/machinescommon/disabled.go b/pkg/machinescommon/disabled.go index e9fc77ae..66379b26 100644 --- a/pkg/machinescommon/disabled.go +++ b/pkg/machinescommon/disabled.go @@ -1,6 +1,8 @@ package machinescommon import ( + "fmt" + "github.com/OctopusDeploy/cli/pkg/util/flag" "github.com/spf13/cobra" ) @@ -17,6 +19,6 @@ func NewCreateTargetDisabledFlags() *CreateTargetDisabledFlags { } } -func RegisterCreateTargetDisabledFlags(cmd *cobra.Command, disabledFlags *CreateTargetDisabledFlags) { - cmd.Flags().BoolVar(&disabledFlags.Disabled.Value, disabledFlags.Disabled.Name, false, "Create the deployment target in a disabled state.") +func RegisterCreateTargetDisabledFlags(cmd *cobra.Command, disabledFlags *CreateTargetDisabledFlags, description string) { + cmd.Flags().BoolVar(&disabledFlags.Disabled.Value, disabledFlags.Disabled.Name, false, fmt.Sprintf("Create the %s in a disabled state.", description)) } diff --git a/pkg/machinescommon/disabledstate.go b/pkg/machinescommon/disabledstate.go new file mode 100644 index 00000000..a0bcf4f9 --- /dev/null +++ b/pkg/machinescommon/disabledstate.go @@ -0,0 +1,236 @@ +package machinescommon + +import ( + "fmt" + "math" + "strings" + + "github.com/OctopusDeploy/cli/pkg/cmd" + "github.com/OctopusDeploy/cli/pkg/output" + "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/workers" +) + +// Machine is the part of a deployment target or worker that enabling and disabling needs. +// Both implementations live in this package, so callers only pick a MachineKind. +type Machine interface { + ID() string + Name() string + IsDisabled() bool + SetDisabled(disabled bool) + Save() error +} + +// MachineKind adapts one machine repository - deployment targets or workers - to the +// enable/disable flow. +type MachineKind struct { + // Noun names the machine in prompts and output, e.g. "deployment target". + Noun string + // GetAllInState returns the machines the server reports in the given disabled state. The + // filter is best effort: the query field is omitempty, so only isDisabled=true can be + // expressed, and callers filter again. + GetAllInState func(isDisabled bool) ([]Machine, error) + // GetByIdentifier resolves a single machine by name or ID. + GetByIdentifier func(idOrName string) (Machine, error) +} + +func NewDeploymentTargetKind(dependencies *cmd.Dependencies) *MachineKind { + return &MachineKind{ + Noun: "deployment target", + GetAllInState: func(isDisabled bool) ([]Machine, error) { + res, err := dependencies.Client.Machines.Get(machines.MachinesQuery{IsDisabled: isDisabled, Skip: 0, Take: math.MaxInt32}) + if err != nil { + return nil, err + } + + result := make([]Machine, 0, len(res.Items)) + for _, target := range res.Items { + result = append(result, deploymentTarget{client: dependencies.Client, target: target}) + } + return result, nil + }, + GetByIdentifier: func(idOrName string) (Machine, error) { + target, err := dependencies.Client.Machines.GetByIdentifier(idOrName) + if err != nil { + return nil, err + } + return deploymentTarget{client: dependencies.Client, target: target}, nil + }, + } +} + +func NewWorkerKind(dependencies *cmd.Dependencies) *MachineKind { + return &MachineKind{ + Noun: "worker", + GetAllInState: func(isDisabled bool) ([]Machine, error) { + res, err := dependencies.Client.Workers.Get(machines.WorkersQuery{IsDisabled: isDisabled, Skip: 0, Take: math.MaxInt32}) + if err != nil { + return nil, err + } + + result := make([]Machine, 0, len(res.Items)) + for _, w := range res.Items { + result = append(result, worker{client: dependencies.Client, worker: w}) + } + return result, nil + }, + GetByIdentifier: func(idOrName string) (Machine, error) { + w, err := dependencies.Client.Workers.GetByIdentifier(idOrName) + if err != nil { + return nil, err + } + return worker{client: dependencies.Client, worker: w}, nil + }, + } +} + +type deploymentTarget struct { + client *client.Client + target *machines.DeploymentTarget +} + +func (m deploymentTarget) ID() string { return m.target.GetID() } +func (m deploymentTarget) Name() string { return m.target.Name } +func (m deploymentTarget) IsDisabled() bool { return m.target.IsDisabled } +func (m deploymentTarget) SetDisabled(disabled bool) { m.target.IsDisabled = disabled } + +func (m deploymentTarget) Save() error { + _, err := machines.Update(m.client, m.target) + return err +} + +type worker struct { + client *client.Client + worker *machines.Worker +} + +func (m worker) ID() string { return m.worker.GetID() } +func (m worker) Name() string { return m.worker.Name } +func (m worker) IsDisabled() bool { return m.worker.IsDisabled } +func (m worker) SetDisabled(disabled bool) { m.worker.IsDisabled = disabled } + +func (m worker) Save() error { + _, err := workers.Update(m.client, m.worker) + return err +} + +type SetDisabledStateOptions struct { + *cmd.Dependencies + *MachineKind + IdOrName string + // Disabled is the state the machine should end up in. + Disabled bool + // Machine is the machine chosen at the prompt. When set it is used directly, saving a round + // trip back to the server for something we already have. + Machine Machine +} + +func NewSetDisabledStateOptions(args []string, dependencies *cmd.Dependencies, kind *MachineKind, disabled bool) *SetDisabledStateOptions { + idOrName := "" + if len(args) > 0 { + idOrName = args[0] + } + + return &SetDisabledStateOptions{ + Dependencies: dependencies, + MachineKind: kind, + IdOrName: idOrName, + Disabled: disabled, + } +} + +// SetDisabledState enables or disables a deployment target or worker, prompting for it when no +// name or ID was supplied. +func SetDisabledState(opts *SetDisabledStateOptions) error { + if !opts.NoPrompt { + if err := PromptMissingMachine(opts); err != nil { + return err + } + } + + machine := opts.Machine + if machine == nil { + if opts.IdOrName == "" { + return fmt.Errorf("%s identifier is required but was not provided", opts.Noun) + } + + var err error + if machine, err = opts.GetByIdentifier(opts.IdOrName); err != nil { + return err + } + } + + state := disabledStateDescription(opts.Disabled) + if machine.IsDisabled() == opts.Disabled { + _, _ = fmt.Fprintf(opts.Out, "%s '%s' %s is already %s.\n", capitalise(opts.Noun), machine.Name(), output.Dimf("(%s)", machine.ID()), state) + return nil + } + + machine.SetDisabled(opts.Disabled) + if err := machine.Save(); err != nil { + return err + } + + _, _ = fmt.Fprintf(opts.Out, "Successfully %s %s '%s' %s.\n", state, opts.Noun, machine.Name(), output.Dimf("(%s)", machine.ID())) + return nil +} + +func PromptMissingMachine(opts *SetDisabledStateOptions) error { + if opts.IdOrName != "" { + return nil + } + + // Only machines that aren't already in the requested state are worth offering. + machineList, err := opts.GetAllInState(!opts.Disabled) + if err != nil { + return err + } + + // The server-side filter isn't guaranteed (older servers, and the disable case can't express + // it), so drop anything already in the requested state here as well. + candidates := make([]Machine, 0, len(machineList)) + for _, machine := range machineList { + if machine.IsDisabled() != opts.Disabled { + candidates = append(candidates, machine) + } + } + + if len(candidates) == 0 { + return fmt.Errorf("no %ss to %s were found", opts.Noun, actionDescription(opts.Disabled)) + } + + // deliberately not selectors.Select: that auto-selects when there is exactly one machine, + // which would mutate it without the user ever being asked. Enable/disable always asks. + selected, err := question.SelectMap( + opts.Ask, + fmt.Sprintf("Select the %s you wish to %s:", opts.Noun, actionDescription(opts.Disabled)), + candidates, + func(machine Machine) string { return machine.Name() }) + if err != nil { + return err + } + + opts.Machine = selected + opts.IdOrName = selected.ID() + return nil +} + +func actionDescription(isDisabled bool) string { + if isDisabled { + return "disable" + } + return "enable" +} + +func disabledStateDescription(isDisabled bool) string { + if isDisabled { + return "disabled" + } + return "enabled" +} + +func capitalise(noun string) string { + return strings.ToUpper(noun[:1]) + noun[1:] +} diff --git a/pkg/machinescommon/disabledstate_test.go b/pkg/machinescommon/disabledstate_test.go new file mode 100644 index 00000000..50571d3a --- /dev/null +++ b/pkg/machinescommon/disabledstate_test.go @@ -0,0 +1,137 @@ +package machinescommon_test + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/cmd" + "github.com/OctopusDeploy/cli/pkg/machinescommon" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/stretchr/testify/assert" +) + +// fakeMachine stands in for a deployment target or a worker; the real adapters are covered +// end-to-end by the deployment-target and worker enable/disable command tests. +type fakeMachine struct { + id string + name string + disabled bool +} + +func (m *fakeMachine) ID() string { return m.id } +func (m *fakeMachine) Name() string { return m.name } +func (m *fakeMachine) IsDisabled() bool { return m.disabled } +func (m *fakeMachine) SetDisabled(disabled bool) { m.disabled = disabled } +func (m *fakeMachine) Save() error { return nil } + +func kindReturning(noun string, machines ...machinescommon.Machine) *machinescommon.MachineKind { + return &machinescommon.MachineKind{ + Noun: noun, + GetAllInState: func(isDisabled bool) ([]machinescommon.Machine, error) { + return machines, nil + }, + } +} + +func TestPromptMissingMachine_IdentifierSupplied(t *testing.T) { + asker, checkRemainingPrompts := testutil.NewMockAsker(t, []*testutil.PA{}) + opts := machinescommon.NewSetDisabledStateOptions([]string{"Machines-1"}, &cmd.Dependencies{Ask: asker}, kindReturning("deployment target"), true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.NoError(t, err) + assert.Equal(t, "Machines-1", opts.IdOrName) +} + +func TestPromptMissingMachine_NoIdentifierSupplied(t *testing.T) { + pa := []*testutil.PA{ + testutil.NewSelectPrompt("Select the deployment target you wish to disable:", "", []string{"web-server", "db-server"}, "db-server"), + } + + asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) + kind := kindReturning("deployment target", + &fakeMachine{id: "Machines-1", name: "web-server"}, + &fakeMachine{id: "Machines-2", name: "db-server"}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.NoError(t, err) + assert.Equal(t, "Machines-2", opts.IdOrName) +} + +func TestPromptMissingMachine_EnableUsesEnableWording(t *testing.T) { + pa := []*testutil.PA{ + testutil.NewSelectPrompt("Select the deployment target you wish to enable:", "", []string{"web-server", "db-server"}, "web-server"), + } + + asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) + kind := kindReturning("deployment target", + &fakeMachine{id: "Machines-1", name: "web-server", disabled: true}, + &fakeMachine{id: "Machines-2", name: "db-server", disabled: true}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, false) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.NoError(t, err) + assert.Equal(t, "Machines-1", opts.IdOrName) +} + +// The noun comes from the kind, so workers get worker wording for free. +func TestPromptMissingMachine_WorkerUsesWorkerWording(t *testing.T) { + pa := []*testutil.PA{ + testutil.NewSelectPrompt("Select the worker you wish to disable:", "", []string{"build-worker"}, "build-worker"), + } + + asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) + kind := kindReturning("worker", &fakeMachine{id: "Workers-1", name: "build-worker"}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.NoError(t, err) + assert.Equal(t, "Workers-1", opts.IdOrName) +} + +// selectors.Select auto-selects when there is exactly one item; enable/disable must not do that +// because it would mutate the only machine in the space without asking. +func TestPromptMissingMachine_AsksEvenWhenThereIsOnlyOneMachine(t *testing.T) { + pa := []*testutil.PA{ + testutil.NewSelectPrompt("Select the deployment target you wish to disable:", "", []string{"web-server"}, "web-server"), + } + + asker, checkRemainingPrompts := testutil.NewMockAsker(t, pa) + kind := kindReturning("deployment target", &fakeMachine{id: "Machines-1", name: "web-server"}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.NoError(t, err) + assert.Equal(t, "Machines-1", opts.IdOrName) +} + +func TestPromptMissingMachine_ErrorsWhenNothingIsInTheOppositeState(t *testing.T) { + asker, checkRemainingPrompts := testutil.NewMockAsker(t, []*testutil.PA{}) + kind := kindReturning("deployment target", &fakeMachine{id: "Machines-1", name: "web-server", disabled: true}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.EqualError(t, err, "no deployment targets to disable were found") +} + +func TestPromptMissingMachine_ErrorsWhenNoWorkerIsInTheOppositeState(t *testing.T) { + asker, checkRemainingPrompts := testutil.NewMockAsker(t, []*testutil.PA{}) + kind := kindReturning("worker", &fakeMachine{id: "Workers-1", name: "build-worker", disabled: true}) + opts := machinescommon.NewSetDisabledStateOptions([]string{}, &cmd.Dependencies{Ask: asker}, kind, true) + + err := machinescommon.PromptMissingMachine(opts) + checkRemainingPrompts() + + assert.EqualError(t, err, "no workers to disable were found") +} From bf83e07c248e06326ad1b5906d4e3a2217503d3b Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 15 Sep 2026 16:19:26 +1000 Subject: [PATCH 2/2] feat: toggle the enabled state of workers Adds `octopus worker enable|disable [ | ]` and a `--disabled` flag on `worker listening-tentacle create` and `worker ssh create`, mirroring the deployment-target commands exactly: same prompt when no identifier is given, same short circuit when the worker is already in the requested state, and the flag is included in the generated automation command. Test support: a worker fixture, and the Workers link on the fake root resource so command tests can reach the workers endpoints. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/worker/disable/disable.go | 29 +++ pkg/cmd/worker/disable/disable_test.go | 110 ++++++++++++ pkg/cmd/worker/enable/enable.go | 29 +++ pkg/cmd/worker/enable/enable_test.go | 110 ++++++++++++ .../listening-tentacle/create/create.go | 7 +- pkg/cmd/worker/ssh/create/create.go | 7 +- pkg/cmd/worker/worker.go | 6 +- pkg/cmd/worker/worker_test.go | 47 +++++ test/fixtures/projects.go | 8 + test/integration/worker_test.go | 169 ++++++++++++++++++ test/testutil/fakeoctopusserver.go | 1 + 11 files changed, 520 insertions(+), 3 deletions(-) create mode 100644 pkg/cmd/worker/disable/disable.go create mode 100644 pkg/cmd/worker/disable/disable_test.go create mode 100644 pkg/cmd/worker/enable/enable.go create mode 100644 pkg/cmd/worker/enable/enable_test.go create mode 100644 pkg/cmd/worker/worker_test.go create mode 100644 test/integration/worker_test.go diff --git a/pkg/cmd/worker/disable/disable.go b/pkg/cmd/worker/disable/disable.go new file mode 100644 index 00000000..82ae7611 --- /dev/null +++ b/pkg/cmd/worker/disable/disable.go @@ -0,0 +1,29 @@ +package disable + +import ( + "github.com/MakeNowJust/heredoc/v2" + "github.com/OctopusDeploy/cli/pkg/cmd" + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/OctopusDeploy/cli/pkg/factory" + "github.com/OctopusDeploy/cli/pkg/machinescommon" + "github.com/OctopusDeploy/cli/pkg/usage" + "github.com/spf13/cobra" +) + +func NewCmdDisable(f factory.Factory) *cobra.Command { + return &cobra.Command{ + Args: usage.MaximumNArgs(1), + Use: "disable [ | ]", + Short: "Disable a worker", + Long: "Disable a worker in Octopus Deploy", + Example: heredoc.Docf(` + %[1]s worker disable Workers-100 + %[1]s worker disable 'build-worker' + `, constants.ExecutableName), + RunE: func(c *cobra.Command, args []string) error { + dependencies := cmd.NewDependencies(f, c) + opts := machinescommon.NewSetDisabledStateOptions(args, dependencies, machinescommon.NewWorkerKind(dependencies), true) + return machinescommon.SetDisabledState(opts) + }, + } +} diff --git a/pkg/cmd/worker/disable/disable_test.go b/pkg/cmd/worker/disable/disable_test.go new file mode 100644 index 00000000..752fe782 --- /dev/null +++ b/pkg/cmd/worker/disable/disable_test.go @@ -0,0 +1,110 @@ +package disable_test + +import ( + "bytes" + "testing" + + "github.com/AlecAivazis/survey/v2" + cmdRoot "github.com/OctopusDeploy/cli/pkg/cmd/root" + "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/test/fixtures" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" +) + +var rootResource = testutil.NewRootResource() + +const spaceID = "Spaces-1" + +func TestWorkerDisable(t *testing.T) { + space1 := fixtures.NewSpace(spaceID, "Default Space") + + tests := []struct { + name string + run func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) + }{ + {"disables a worker identified on the command line", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "disable", "Workers-100", "--no-prompt"}) + 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/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", false)) + + updateRequest := api.ExpectRequest(t, "PUT", "/api/Spaces-1/workers/Workers-100") + updated, err := testutil.ReadJson[machines.Worker](updateRequest.Request.Body) + assert.Nil(t, err) + assert.Equal(t, true, updated.IsDisabled) + updateRequest.RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", true)) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "Successfully disabled worker 'build-worker'") + assert.Equal(t, "", stdErr.String()) + }}, + + {"does not update a worker which is already disabled", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "disable", "Workers-100", "--no-prompt"}) + 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/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", true)) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "is already disabled") + assert.Equal(t, "", stdErr.String()) + }}, + + {"prompts for the worker when none was supplied", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "disable"}) + 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/workers?take=2147483647"). + RespondWith(resources.Resources[*machines.Worker]{Items: []*machines.Worker{ + fixtures.NewWorker(spaceID, "Workers-100", "build-worker", false), + fixtures.NewWorker(spaceID, "Workers-200", "test-worker", false), + }}) + + _ = qa.ExpectQuestion(t, &survey.Select{ + Message: "Select the worker you wish to disable:", + Options: []string{"build-worker", "test-worker"}, + }).AnswerWith("build-worker") + + api.ExpectRequest(t, "PUT", "/api/Spaces-1/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", true)) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "Successfully disabled worker 'build-worker'") + assert.Equal(t, "", stdErr.String()) + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + api, qa := testutil.NewMockServerAndAsker() + askProvider := question.NewAskProvider(qa.AsAsker()) + fac := testutil.NewMockFactoryWithSpaceAndPrompt(api, space1, askProvider) + rootCmd := cmdRoot.NewCmdRoot(fac, nil, askProvider) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + test.run(t, api, qa, rootCmd, stdout, stderr) + }) + } +} diff --git a/pkg/cmd/worker/enable/enable.go b/pkg/cmd/worker/enable/enable.go new file mode 100644 index 00000000..d70f4177 --- /dev/null +++ b/pkg/cmd/worker/enable/enable.go @@ -0,0 +1,29 @@ +package enable + +import ( + "github.com/MakeNowJust/heredoc/v2" + "github.com/OctopusDeploy/cli/pkg/cmd" + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/OctopusDeploy/cli/pkg/factory" + "github.com/OctopusDeploy/cli/pkg/machinescommon" + "github.com/OctopusDeploy/cli/pkg/usage" + "github.com/spf13/cobra" +) + +func NewCmdEnable(f factory.Factory) *cobra.Command { + return &cobra.Command{ + Args: usage.MaximumNArgs(1), + Use: "enable [ | ]", + Short: "Enable a worker", + Long: "Enable a worker in Octopus Deploy", + Example: heredoc.Docf(` + %[1]s worker enable Workers-100 + %[1]s worker enable 'build-worker' + `, constants.ExecutableName), + RunE: func(c *cobra.Command, args []string) error { + dependencies := cmd.NewDependencies(f, c) + opts := machinescommon.NewSetDisabledStateOptions(args, dependencies, machinescommon.NewWorkerKind(dependencies), false) + return machinescommon.SetDisabledState(opts) + }, + } +} diff --git a/pkg/cmd/worker/enable/enable_test.go b/pkg/cmd/worker/enable/enable_test.go new file mode 100644 index 00000000..59ca5af9 --- /dev/null +++ b/pkg/cmd/worker/enable/enable_test.go @@ -0,0 +1,110 @@ +package enable_test + +import ( + "bytes" + "testing" + + "github.com/AlecAivazis/survey/v2" + cmdRoot "github.com/OctopusDeploy/cli/pkg/cmd/root" + "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/test/fixtures" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/resources" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" +) + +var rootResource = testutil.NewRootResource() + +const spaceID = "Spaces-1" + +func TestWorkerEnable(t *testing.T) { + space1 := fixtures.NewSpace(spaceID, "Default Space") + + tests := []struct { + name string + run func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) + }{ + {"enables a worker identified on the command line", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "enable", "Workers-100", "--no-prompt"}) + 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/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", true)) + + updateRequest := api.ExpectRequest(t, "PUT", "/api/Spaces-1/workers/Workers-100") + updated, err := testutil.ReadJson[machines.Worker](updateRequest.Request.Body) + assert.Nil(t, err) + assert.Equal(t, false, updated.IsDisabled) + updateRequest.RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", false)) + + _, err = testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "Successfully enabled worker 'build-worker'") + assert.Equal(t, "", stdErr.String()) + }}, + + {"does not update a worker which is already enabled", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "enable", "Workers-100", "--no-prompt"}) + 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/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", false)) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "is already enabled") + assert.Equal(t, "", stdErr.String()) + }}, + + {"prompts for the worker when none was supplied", func(t *testing.T, api *testutil.MockHttpServer, qa *testutil.AskMocker, rootCmd *cobra.Command, stdOut *bytes.Buffer, stdErr *bytes.Buffer) { + cmdReceiver := testutil.GoBegin2(func() (*cobra.Command, error) { + defer api.Close() + rootCmd.SetArgs([]string{"worker", "enable"}) + 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/workers?isDisabled=true&take=2147483647"). + RespondWith(resources.Resources[*machines.Worker]{Items: []*machines.Worker{ + fixtures.NewWorker(spaceID, "Workers-100", "build-worker", true), + fixtures.NewWorker(spaceID, "Workers-200", "test-worker", true), + }}) + + _ = qa.ExpectQuestion(t, &survey.Select{ + Message: "Select the worker you wish to enable:", + Options: []string{"build-worker", "test-worker"}, + }).AnswerWith("build-worker") + + api.ExpectRequest(t, "PUT", "/api/Spaces-1/workers/Workers-100").RespondWith(fixtures.NewWorker(spaceID, "Workers-100", "build-worker", false)) + + _, err := testutil.ReceivePair(cmdReceiver) + assert.Nil(t, err) + assert.Contains(t, stdOut.String(), "Successfully enabled worker 'build-worker'") + assert.Equal(t, "", stdErr.String()) + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + api, qa := testutil.NewMockServerAndAsker() + askProvider := question.NewAskProvider(qa.AsAsker()) + fac := testutil.NewMockFactoryWithSpaceAndPrompt(api, space1, askProvider) + rootCmd := cmdRoot.NewCmdRoot(fac, nil, askProvider) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + test.run(t, api, qa, rootCmd, stdout, stderr) + }) + } +} diff --git a/pkg/cmd/worker/listening-tentacle/create/create.go b/pkg/cmd/worker/listening-tentacle/create/create.go index e629eca3..72688de7 100644 --- a/pkg/cmd/worker/listening-tentacle/create/create.go +++ b/pkg/cmd/worker/listening-tentacle/create/create.go @@ -31,6 +31,7 @@ type CreateFlags struct { *machinescommon.CreateTargetMachinePolicyFlags *shared.WorkerPoolFlags *machinescommon.WebFlags + *machinescommon.CreateTargetDisabledFlags } type CreateOptions struct { @@ -49,6 +50,7 @@ func NewCreateFlags() *CreateFlags { CreateTargetProxyFlags: machinescommon.NewCreateTargetProxyFlags(), CreateTargetMachinePolicyFlags: machinescommon.NewCreateTargetMachinePolicyFlags(), WorkerPoolFlags: shared.NewWorkerPoolFlags(), + CreateTargetDisabledFlags: machinescommon.NewCreateTargetDisabledFlags(), WebFlags: machinescommon.NewWebFlags(), } } @@ -87,6 +89,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { machinescommon.RegisterCreateTargetProxyFlags(cmd, createFlags.CreateTargetProxyFlags, "Listening Tentacle") machinescommon.RegisterCreateTargetMachinePolicyFlags(cmd, createFlags.CreateTargetMachinePolicyFlags) shared.RegisterCreateWorkerWorkerPoolFlags(cmd, createFlags.WorkerPoolFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "worker") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd @@ -126,6 +129,8 @@ func createRun(opts *CreateOptions) error { } worker.MachinePolicyID = machinePolicy.GetID() + worker.IsDisabled = opts.Disabled.Value + createdWorker, err := opts.Client.Workers.Add(worker) if err != nil { return err @@ -133,7 +138,7 @@ func createRun(opts *CreateOptions) error { fmt.Fprintf(opts.Out, "Successfully created Listening Tentacle worker '%s'.\n", worker.Name) if !opts.NoPrompt { - autoCmd := flag.GenerateAutomationCmd(opts.CmdPath, opts.GetSpaceNameOrEmpty(), opts.Name, opts.URL, opts.Thumbprint, opts.Proxy, opts.MachinePolicy, opts.WorkerPools) + autoCmd := flag.GenerateAutomationCmd(opts.CmdPath, opts.GetSpaceNameOrEmpty(), opts.Name, opts.URL, opts.Thumbprint, opts.Proxy, opts.MachinePolicy, opts.WorkerPools, opts.Disabled) fmt.Fprintf(opts.Out, "\nAutomation Command: %s\n", autoCmd) } diff --git a/pkg/cmd/worker/ssh/create/create.go b/pkg/cmd/worker/ssh/create/create.go index 2028b409..f3ad8e81 100644 --- a/pkg/cmd/worker/ssh/create/create.go +++ b/pkg/cmd/worker/ssh/create/create.go @@ -26,6 +26,7 @@ type CreateFlags struct { *shared.WorkerPoolFlags *machinescommon.WebFlags *machinescommon.SshCommonFlags + *machinescommon.CreateTargetDisabledFlags } type CreateOptions struct { @@ -44,6 +45,7 @@ func NewCreateFlags() *CreateFlags { CreateTargetProxyFlags: machinescommon.NewCreateTargetProxyFlags(), CreateTargetMachinePolicyFlags: machinescommon.NewCreateTargetMachinePolicyFlags(), WorkerPoolFlags: shared.NewWorkerPoolFlags(), + CreateTargetDisabledFlags: machinescommon.NewCreateTargetDisabledFlags(), WebFlags: machinescommon.NewWebFlags(), } } @@ -80,6 +82,7 @@ func NewCmdCreate(f factory.Factory) *cobra.Command { machinescommon.RegisterCreateTargetProxyFlags(cmd, createFlags.CreateTargetProxyFlags, "SSH worker") machinescommon.RegisterCreateTargetMachinePolicyFlags(cmd, createFlags.CreateTargetMachinePolicyFlags) shared.RegisterCreateWorkerWorkerPoolFlags(cmd, createFlags.WorkerPoolFlags) + machinescommon.RegisterCreateTargetDisabledFlags(cmd, createFlags.CreateTargetDisabledFlags, "worker") machinescommon.RegisterWebFlag(cmd, createFlags.WebFlags) return cmd @@ -129,6 +132,8 @@ func createRun(opts *CreateOptions) error { } worker.MachinePolicyID = machinePolicy.GetID() + worker.IsDisabled = opts.Disabled.Value + createdWorker, err := opts.Client.Workers.Add(worker) if err != nil { return err @@ -136,7 +141,7 @@ func createRun(opts *CreateOptions) error { fmt.Fprintf(opts.Out, "Successfully created SSH worker '%s'.\n", createdWorker.Name) if !opts.NoPrompt { - autoCmd := flag.GenerateAutomationCmd(opts.CmdPath, opts.GetSpaceNameOrEmpty(), opts.Name, opts.HostName, opts.Port, opts.Fingerprint, opts.Runtime, opts.Platform, opts.WorkerPools, opts.Account, opts.Proxy, opts.MachinePolicy) + autoCmd := flag.GenerateAutomationCmd(opts.CmdPath, opts.GetSpaceNameOrEmpty(), opts.Name, opts.HostName, opts.Port, opts.Fingerprint, opts.Runtime, opts.Platform, opts.WorkerPools, opts.Account, opts.Proxy, opts.MachinePolicy, opts.Disabled) fmt.Fprintf(opts.Out, "\nAutomation Command: %s\n", autoCmd) } diff --git a/pkg/cmd/worker/worker.go b/pkg/cmd/worker/worker.go index 6198d173..49f5c8b0 100644 --- a/pkg/cmd/worker/worker.go +++ b/pkg/cmd/worker/worker.go @@ -3,6 +3,8 @@ package worker import ( "github.com/MakeNowJust/heredoc/v2" cmdDelete "github.com/OctopusDeploy/cli/pkg/cmd/worker/delete" + cmdDisable "github.com/OctopusDeploy/cli/pkg/cmd/worker/disable" + cmdEnable "github.com/OctopusDeploy/cli/pkg/cmd/worker/enable" cmdList "github.com/OctopusDeploy/cli/pkg/cmd/worker/list" listeningTentacle "github.com/OctopusDeploy/cli/pkg/cmd/worker/listening-tentacle" pollingTentacle "github.com/OctopusDeploy/cli/pkg/cmd/worker/polling-tentacle" @@ -31,8 +33,10 @@ func NewCmdWorker(f factory.Factory) *cobra.Command { cmd.AddCommand(listeningTentacle.NewCmdListeningTentacle(f)) cmd.AddCommand(pollingTentacle.NewCmdPollingTentacle(f)) cmd.AddCommand(ssh.NewCmdSsh(f)) - cmd.AddCommand(cmdList.NewCmdList(f)) cmd.AddCommand(cmdDelete.NewCmdDelete(f)) + cmd.AddCommand(cmdEnable.NewCmdEnable(f)) + cmd.AddCommand(cmdDisable.NewCmdDisable(f)) + cmd.AddCommand(cmdList.NewCmdList(f)) cmd.AddCommand(cmdView.NewCmdView(f)) return cmd diff --git a/pkg/cmd/worker/worker_test.go b/pkg/cmd/worker/worker_test.go new file mode 100644 index 00000000..80d5f239 --- /dev/null +++ b/pkg/cmd/worker/worker_test.go @@ -0,0 +1,47 @@ +package worker_test + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/cmd/worker" + "github.com/OctopusDeploy/cli/pkg/machinescommon" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWorkerHasEnableAndDisableCommands(t *testing.T) { + cmd := worker.NewCmdWorker(testutil.NewMockFactory(testutil.NewMockHttpServer())) + + assert.NotNil(t, findCommand(cmd, "enable")) + assert.NotNil(t, findCommand(cmd, "disable")) +} + +func TestEveryWorkerCreateCommandSupportsDisabled(t *testing.T) { + root := worker.NewCmdWorker(testutil.NewMockFactory(testutil.NewMockHttpServer())) + + workerTypes := []string{"listening-tentacle", "ssh"} + for _, workerType := range workerTypes { + t.Run(workerType, func(t *testing.T) { + typeCmd := findCommand(root, workerType) + require.NotNil(t, typeCmd) + + createCmd := findCommand(typeCmd, "create") + require.NotNil(t, createCmd) + + disabled := createCmd.Flags().Lookup(machinescommon.FlagDisabled) + require.NotNil(t, disabled) + assert.Equal(t, "false", disabled.DefValue) + }) + } +} + +func findCommand(parent *cobra.Command, name string) *cobra.Command { + for _, c := range parent.Commands() { + if c.Name() == name { + return c + } + } + return nil +} diff --git a/test/fixtures/projects.go b/test/fixtures/projects.go index 4172ea0d..ac06a1e4 100644 --- a/test/fixtures/projects.go +++ b/test/fixtures/projects.go @@ -250,6 +250,14 @@ func NewDeploymentTarget(spaceID string, targetID string, name string, isDisable return result } +func NewWorker(spaceID string, workerID string, name string, isDisabled bool) *machines.Worker { + result := machines.NewWorker(name, machines.NewListeningTentacleEndpoint(&url.URL{Scheme: "https", Host: "worker:10933"}, "0123456789ABCDEF0123456789ABCDEF01234567")) + result.ID = workerID + result.SpaceID = spaceID + result.IsDisabled = isDisabled + return result +} + func NewRunbook(spaceID string, projectID string, runbookID string, name string) *runbooks.Runbook { result := runbooks.NewRunbook(name, projectID) result.ID = runbookID diff --git a/test/integration/worker_test.go b/test/integration/worker_test.go new file mode 100644 index 00000000..f607657e --- /dev/null +++ b/test/integration/worker_test.go @@ -0,0 +1,169 @@ +package integration_test + +import ( + "fmt" + "testing" + + "github.com/OctopusDeploy/cli/test/integration" + "github.com/OctopusDeploy/cli/test/testutil" + octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/machines" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// listening tentacles are the cheapest worker to create for real: the server +// records the endpoint without checking it can be reached +func createListeningTentacleWorker(t *testing.T, apiClient *octopusApiClient.Client, poolName string, name string, extraArgs ...string) *machines.Worker { + args := append([]string{ + "worker", "listening-tentacle", "create", + "--name", name, + "--worker-pool", poolName, + "--machine-policy", "Default Machine Policy", + "--thumbprint", "0123456789ABCDEF0123456789ABCDEF01234567", + "--url", fmt.Sprintf("https://%s.invalid:10933", name), + }, extraArgs...) + + stdOut, stdErr, err := integration.RunCli("Default", args...) + if !testutil.AssertSuccess(t, err, stdOut, stdErr) { + return nil + } + + worker, err := apiClient.Workers.GetByIdentifier(name) + testutil.RequireSuccess(t, err) + t.Cleanup(func() { assert.Nil(t, apiClient.Workers.DeleteByID(worker.GetID())) }) + return worker +} + +// settings a toggle must not disturb. HealthStatus, Status and StatusSummary are +// excluded on purpose: the server derives them from IsDisabled. +type workerSettings struct { + Name string + WorkerPoolIDs []string + MachinePolicyID string + Thumbprint string + URI string + EndpointType string +} + +func workerSettingsOf(worker *machines.Worker) workerSettings { + return workerSettings{ + Name: worker.Name, + WorkerPoolIDs: worker.WorkerPoolIDs, + MachinePolicyID: worker.MachinePolicyID, + Thumbprint: worker.Thumbprint, + URI: worker.URI, + EndpointType: fmt.Sprintf("%T", worker.Endpoint), + } +} + +func TestWorkerEnableDisable(t *testing.T) { + runId := uuid.New() + apiClient, err := integration.GetApiClient(space1ID) + testutil.RequireSuccess(t, err) + + pools, err := apiClient.WorkerPools.GetAll() + testutil.RequireSuccess(t, err) + + // dynamic pools provision their own workers, so only a static pool will take one + poolName := "" + for _, pool := range pools { + if pool.CanAddWorkers { + poolName = pool.Name + break + } + } + require.NotEmpty(t, poolName, "the space needs a worker pool that accepts workers") + + t.Run("create --disabled", func(t *testing.T) { + worker := createListeningTentacleWorker(t, apiClient, poolName, fmt.Sprintf("wkr-disabled-%s", runId), "--disabled") + require.NotNil(t, worker) + assert.True(t, worker.IsDisabled) + }) + + t.Run("create without --disabled", func(t *testing.T) { + worker := createListeningTentacleWorker(t, apiClient, poolName, fmt.Sprintf("wkr-enabled-%s", runId)) + require.NotNil(t, worker) + assert.False(t, worker.IsDisabled) + }) + + t.Run("enable and disable change nothing else", func(t *testing.T) { + worker := createListeningTentacleWorker(t, apiClient, poolName, fmt.Sprintf("wkr-toggle-%s", runId), "--disabled") + require.NotNil(t, worker) + before := workerSettingsOf(worker) + + stdOut, stdErr, err := integration.RunCli("Default", "worker", "enable", worker.Name) + if !testutil.AssertSuccess(t, err, stdOut, stdErr) { + return + } + assert.Contains(t, stdOut, fmt.Sprintf("Successfully enabled worker '%s'", worker.Name)) + + enabled, err := apiClient.Workers.GetByIdentifier(worker.GetID()) + testutil.RequireSuccess(t, err) + assert.False(t, enabled.IsDisabled) + + // the update is a read-modify-write of the whole worker, so the rest of + // its settings have to survive the round trip + assert.Equal(t, before, workerSettingsOf(enabled)) + + // and back again, by ID this time + stdOut, stdErr, err = integration.RunCli("Default", "worker", "disable", worker.GetID()) + if !testutil.AssertSuccess(t, err, stdOut, stdErr) { + return + } + assert.Contains(t, stdOut, fmt.Sprintf("Successfully disabled worker '%s'", worker.Name)) + + disabled, err := apiClient.Workers.GetByIdentifier(worker.GetID()) + testutil.RequireSuccess(t, err) + assert.True(t, disabled.IsDisabled) + assert.Equal(t, before, workerSettingsOf(disabled)) + }) + + t.Run("already in the requested state", func(t *testing.T) { + worker := createListeningTentacleWorker(t, apiClient, poolName, fmt.Sprintf("wkr-noop-%s", runId)) + require.NotNil(t, worker) + + stdOut, stdErr, err := integration.RunCli("Default", "worker", "enable", worker.Name) + if !testutil.AssertSuccess(t, err, stdOut, stdErr) { + return + } + assert.Contains(t, stdOut, fmt.Sprintf("Worker '%s' (%s) is already enabled.", worker.Name, worker.GetID())) + + unchanged, err := apiClient.Workers.GetByIdentifier(worker.GetID()) + testutil.RequireSuccess(t, err) + assert.False(t, unchanged.IsDisabled) + assert.Equal(t, worker.ModifiedOn, unchanged.ModifiedOn, "no update should have been sent") + }) + + t.Run("errors", func(t *testing.T) { + for _, tc := range []struct { + name string + args []string + expected string + }{ + {"unknown name", []string{"enable", "no-such-worker"}, "cannot find worker with name or ID of 'no-such-worker'"}, + {"no identifier without prompting", []string{"disable"}, "worker identifier is required but was not provided"}, + } { + t.Run(tc.name, func(t *testing.T) { + args := append([]string{"worker"}, tc.args...) + stdOut, stdErr, err := integration.RunCli("Default", args...) + assert.Error(t, err, stdOut) + assert.Contains(t, stdOut+stdErr, tc.expected) + }) + } + + t.Run("a deployment target is not a worker", func(t *testing.T) { + targets, err := apiClient.Machines.Get(machines.MachinesQuery{Take: 1}) + testutil.RequireSuccess(t, err) + if len(targets.Items) == 0 { + t.Skip("no deployment targets in this space") + } + targetID := targets.Items[0].GetID() + + stdOut, stdErr, err := integration.RunCli("Default", "worker", "enable", targetID) + assert.Error(t, err, stdOut) + assert.Contains(t, stdOut+stdErr, fmt.Sprintf("cannot find worker with name or ID of '%s'", targetID)) + }) + }) +} diff --git a/test/testutil/fakeoctopusserver.go b/test/testutil/fakeoctopusserver.go index ac337484..89ba4189 100644 --- a/test/testutil/fakeoctopusserver.go +++ b/test/testutil/fakeoctopusserver.go @@ -228,6 +228,7 @@ func NewRootResource() *octopusApiClient.RootResource { root.Links[constants.LinkPackages] = "/api/Spaces-1/packages{/id}{?nuGetPackageId,filter,latest,skip,take,includeNotes}" root.Links[constants.LinkLifecycles] = "/api/Spaces-1/lifecycles{/id}{?skip,take,ids,partialName}" root.Links[constants.LinkMachines] = "/api/Spaces-1/machines{/id}{?skip,take,name,ids,partialName,roles,isDisabled,healthStatuses,commStyles,tenantIds,tenantTags,environmentIds,thumbprint,deploymentId,shellNames}" + root.Links[constants.LinkWorkers] = "/api/Spaces-1/workers{/id}{?skip,take,name,ids,partialName,isDisabled,healthStatuses,commStyles,shellNames}" root.Links[constants.LinkProjectGroups] = "/api/Spaces-1/projectgroups{/id}{?skip,take,ids,partialName}" root.Links[constants.LinkUsers] = "/api/users" root.Links[constants.LinkCurrentUser] = "/api/users/me"