From 35a6fbaeeaa57001028a1ce94915f4a0499ecebb Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Tue, 19 Mar 2024 11:37:24 +0100 Subject: [PATCH] replace `CommandContext` param with `RestartOptions` type inspired by the 'GetOptions' type in the [kubectl get command](https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/get/get.go#L56-L86), mostly because `CommandContext` is confusing (I know, I contributed to it!) Signed-off-by: Xavier Coulon --- pkg/cmd/adm/register_member.go | 25 ++++++------ pkg/cmd/adm/register_member_test.go | 21 ++++------ pkg/cmd/adm/restart.go | 55 ++++++++++++++------------ pkg/cmd/adm/restart_test.go | 61 +++++++++++++++++------------ pkg/cmd/adm/unregister_member.go | 2 +- 5 files changed, 88 insertions(+), 76 deletions(-) diff --git a/pkg/cmd/adm/register_member.go b/pkg/cmd/adm/register_member.go index 2ec04b6..c9d1dbf 100644 --- a/pkg/cmd/adm/register_member.go +++ b/pkg/cmd/adm/register_member.go @@ -37,11 +37,10 @@ func NewRegisterMemberCmd() *cobra.Command { Long: `Downloads the 'add-cluster.sh' script from the 'toolchain-cicd' repo and calls it twice: once to register the Host cluster in the Member cluster and once to register the Member cluster in the host cluster.`, RunE: func(cmd *cobra.Command, args []string) error { term := ioutils.NewTerminal(cmd.InOrStdin, cmd.OutOrStdout) - ctx := clicontext.NewCommandContext(term, client.DefaultNewClient, client.DefaultNewRESTClient) newCommand := func(name string, args ...string) *exec.Cmd { return exec.Command(name, args...) } - return registerMemberCluster(ctx, newCommand, hostKubeconfig, memberKubeconfig) + return registerMemberCluster(context.Background(), term, client.DefaultNewClient, newCommand, hostKubeconfig, memberKubeconfig) }, } defaultKubeconfigPath := "" @@ -55,15 +54,15 @@ func NewRegisterMemberCmd() *cobra.Command { return cmd } -func registerMemberCluster(ctx *clicontext.CommandContext, newCommand client.CommandCreator, hostKubeconfig, memberKubeconfig string) error { - ctx.AskForConfirmation(ioutils.WithMessagef("register member cluster from kubeconfig %s. Be aware that the ksctl disables automatic approval to prevent new users being provisioned to the new member cluster. "+ +func registerMemberCluster(ctx context.Context, term ioutils.Terminal, newClient clicontext.NewClientFunc, newCommand client.CommandCreator, hostKubeconfig, memberKubeconfig string) error { + term.AskForConfirmation(ioutils.WithMessagef("register member cluster from kubeconfig %s. Be aware that the ksctl disables automatic approval to prevent new users being provisioned to the new member cluster. "+ "You will need to enable it again manually.", memberKubeconfig)) - hostClusterConfig, err := configuration.LoadClusterConfig(ctx, configuration.HostName) + hostClusterConfig, err := configuration.LoadClusterConfig(term, configuration.HostName) if err != nil { return err } - hostClusterClient, err := ctx.NewClient(hostClusterConfig.Token, hostClusterConfig.ServerAPI) + hostClusterClient, err := newClient(hostClusterConfig.Token, hostClusterConfig.ServerAPI) if err != nil { return err } @@ -72,23 +71,23 @@ func registerMemberCluster(ctx *clicontext.CommandContext, newCommand client.Com return err } - if err := runAddClusterScript(ctx, newCommand, configuration.Host, hostKubeconfig, memberKubeconfig); err != nil { + if err := runAddClusterScript(term, newCommand, configuration.Host, hostKubeconfig, memberKubeconfig); err != nil { return err } - if err := runAddClusterScript(ctx, newCommand, configuration.Member, hostKubeconfig, memberKubeconfig); err != nil { + if err := runAddClusterScript(term, newCommand, configuration.Member, hostKubeconfig, memberKubeconfig); err != nil { return err } warningMessage := "The automatic approval was disabled!\n Configure the new member cluster in ToolchainConfig and apply the changes to the cluster." - if err := restartHostOperator(ctx, hostClusterClient, hostClusterConfig); err != nil { + if err := restartHostOperator(ctx, term, hostClusterClient, hostClusterConfig); err != nil { return fmt.Errorf("%w\nIn Additon, there is another warning you should be aware of:\n%s", err, warningMessage) } - ctx.Printlnf("!!!!!!!!!!!!!!!") - ctx.Printlnf("!!! WARNING !!!") - ctx.Printlnf("!!!!!!!!!!!!!!!") - ctx.Printlnf(warningMessage) + term.Printlnf("!!!!!!!!!!!!!!!") + term.Printlnf("!!! WARNING !!!") + term.Printlnf("!!!!!!!!!!!!!!!") + term.Printlnf(warningMessage) return nil } diff --git a/pkg/cmd/adm/register_member_test.go b/pkg/cmd/adm/register_member_test.go index 0b87ea8..b258808 100644 --- a/pkg/cmd/adm/register_member_test.go +++ b/pkg/cmd/adm/register_member_test.go @@ -11,7 +11,6 @@ import ( "github.com/codeready-toolchain/toolchain-common/pkg/test/config" _ "github.com/kubesaw/ksctl/pkg/client" "github.com/kubesaw/ksctl/pkg/configuration" - clicontext "github.com/kubesaw/ksctl/pkg/context" . "github.com/kubesaw/ksctl/pkg/test" "github.com/h2non/gock" @@ -55,14 +54,13 @@ func TestRegisterMember(t *testing.T) { t.Run("When automatic approval is enabled", func(t *testing.T) { term := NewFakeTerminalWithResponse("Y") toolchainConfig := config.NewToolchainConfigObj(t, config.AutomaticApproval().Enabled(true)) - newClient, newRESTClient, fakeClient := NewFakeClients(t, toolchainConfig, deployment) + newClient, _, fakeClient := NewFakeClients(t, toolchainConfig, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = whenDeploymentThenUpdated(t, fakeClient, hostDeploymentName, 1, &numberOfUpdateCalls) - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) counter = 0 // when - err := registerMemberCluster(ctx, ocCommandCreator, hostKubeconfig, memberKubeconfig) + err := registerMemberCluster(context.Background(), term, newClient, ocCommandCreator, hostKubeconfig, memberKubeconfig) // then require.NoError(t, err) @@ -87,18 +85,17 @@ func TestRegisterMember(t *testing.T) { t.Run("When toolchainConfig is not present", func(t *testing.T) { term := NewFakeTerminalWithResponse("Y") - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) fakeClient.MockUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { if _, ok := obj.(*toolchainv1alpha1.ToolchainConfig); ok { return fmt.Errorf("should not be called") } return fakeClient.Client.Update(ctx, obj, opts...) } - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) counter = 0 // when - err := registerMemberCluster(ctx, ocCommandCreator, hostKubeconfig, memberKubeconfig) + err := registerMemberCluster(context.Background(), term, newClient, ocCommandCreator, hostKubeconfig, memberKubeconfig) // then require.NoError(t, err) @@ -113,18 +110,17 @@ func TestRegisterMember(t *testing.T) { t.Run("When automatic approval is disabled", func(t *testing.T) { term := NewFakeTerminalWithResponse("Y") toolchainConfig := config.NewToolchainConfigObj(t, config.AutomaticApproval().Enabled(false)) - newClient, newRESTClient, fakeClient := NewFakeClients(t, toolchainConfig, deployment) + newClient, _, fakeClient := NewFakeClients(t, toolchainConfig, deployment) fakeClient.MockUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { if _, ok := obj.(*toolchainv1alpha1.ToolchainConfig); ok { return fmt.Errorf("should not be called") } return fakeClient.Client.Update(ctx, obj, opts...) } - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) counter = 0 // when - err := registerMemberCluster(ctx, ocCommandCreator, hostKubeconfig, memberKubeconfig) + err := registerMemberCluster(context.Background(), term, newClient, ocCommandCreator, hostKubeconfig, memberKubeconfig) // then require.NoError(t, err) @@ -143,15 +139,14 @@ func TestRegisterMember(t *testing.T) { toolchainConfig := config.NewToolchainConfigObj(t, config.AutomaticApproval().Enabled(false)) toolchainConfig2 := config.NewToolchainConfigObj(t, config.AutomaticApproval().Enabled(true)) toolchainConfig2.Name = "config2" - newClient, newRESTClient, fakeClient := NewFakeClients(t, toolchainConfig, toolchainConfig2, deployment) + newClient, _, fakeClient := NewFakeClients(t, toolchainConfig, toolchainConfig2, deployment) fakeClient.MockUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { return fmt.Errorf("should not be called") } - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) counter = 0 // when - err := registerMemberCluster(ctx, ocCommandCreator, hostKubeconfig, memberKubeconfig) + err := registerMemberCluster(context.Background(), term, newClient, ocCommandCreator, hostKubeconfig, memberKubeconfig) // then require.Error(t, err) diff --git a/pkg/cmd/adm/restart.go b/pkg/cmd/adm/restart.go index cd10361..36e9578 100644 --- a/pkg/cmd/adm/restart.go +++ b/pkg/cmd/adm/restart.go @@ -28,9 +28,11 @@ func NewRestartCmd() *cobra.Command { If no deployment name is provided, then it lists all existing deployments in the namespace.`, Args: cobra.RangeArgs(0, 1), RunE: func(cmd *cobra.Command, args []string) error { - term := ioutils.NewTerminal(cmd.InOrStdin, cmd.OutOrStdout) - ctx := clicontext.NewCommandContext(term, client.DefaultNewClient, client.DefaultNewRESTClient) - return restart(ctx, targetCluster, args...) + o := &RestartOptions{ + term: ioutils.NewTerminal(cmd.InOrStdin, cmd.OutOrStdout), + newClient: client.DefaultNewClient, + } + return o.restart(cmd.Context(), targetCluster, args...) }, } command.Flags().StringVarP(&targetCluster, "target-cluster", "t", "", "The target cluster") @@ -38,58 +40,63 @@ If no deployment name is provided, then it lists all existing deployments in the return command } -func restart(ctx *clicontext.CommandContext, clusterName string, deployments ...string) error { - cfg, err := configuration.LoadClusterConfig(ctx, clusterName) +type RestartOptions struct { + term ioutils.Terminal + newClient clicontext.NewClientFunc +} + +func (o *RestartOptions) restart(ctx context.Context, clusterName string, deployments ...string) error { + cfg, err := configuration.LoadClusterConfig(o.term, clusterName) if err != nil { return err } - cl, err := ctx.NewClient(cfg.Token, cfg.ServerAPI) + cl, err := o.newClient(cfg.Token, cfg.ServerAPI) if err != nil { return err } if len(deployments) == 0 { - err := printExistingDeployments(ctx.Terminal, cl, cfg) + err := printExistingDeployments(o.term, cl, cfg) if err != nil { - ctx.Terminal.Printlnf("\nERROR: Failed to list existing deployments\n :%s", err.Error()) + o.term.Printlnf("\nERROR: Failed to list existing deployments\n :%s", err.Error()) } return fmt.Errorf("at least one deployment name is required, include one or more of the above deployments to restart") } deploymentName := deployments[0] - if !ctx.AskForConfirmation( + if !o.term.AskForConfirmation( ioutils.WithMessagef("restart the deployment '%s' in namespace '%s'", deploymentName, cfg.SandboxNamespace)) { return nil } - return restartDeployment(ctx, cl, cfg, deploymentName) + return restartDeployment(ctx, o.term, cl, cfg, deploymentName) } -func restartDeployment(ctx *clicontext.CommandContext, cl runtimeclient.Client, cfg configuration.ClusterConfig, deploymentName string) error { +func restartDeployment(ctx context.Context, term ioutils.Terminal, cl runtimeclient.Client, cfg configuration.ClusterConfig, deploymentName string) error { namespacedName := types.NamespacedName{ Namespace: cfg.SandboxNamespace, Name: deploymentName, } - originalReplicas, err := scaleToZero(cl, namespacedName) + originalReplicas, err := scaleToZero(ctx, cl, namespacedName) if err != nil { if apierrors.IsNotFound(err) { - ctx.Printlnf("\nERROR: The given deployment '%s' wasn't found.", deploymentName) - return printExistingDeployments(ctx, cl, cfg) + term.Printlnf("\nERROR: The given deployment '%s' wasn't found.", deploymentName) + return printExistingDeployments(term, cl, cfg) } return err } - ctx.Println("The deployment was scaled to 0") - if err := scaleBack(ctx, cl, namespacedName, originalReplicas); err != nil { - ctx.Printlnf("Scaling the deployment '%s' in namespace '%s' back to '%d' replicas wasn't successful", originalReplicas) - ctx.Println("Please, try to contact administrators to scale the deployment back manually") + term.Println("The deployment was scaled to 0") + if err := scaleBack(term, cl, namespacedName, originalReplicas); err != nil { + term.Printlnf("Scaling the deployment '%s' in namespace '%s' back to '%d' replicas wasn't successful", originalReplicas) + term.Println("Please, try to contact administrators to scale the deployment back manually") return err } - ctx.Printlnf("The deployment was scaled back to '%d'", originalReplicas) + term.Printlnf("The deployment was scaled back to '%d'", originalReplicas) return nil } -func restartHostOperator(ctx *clicontext.CommandContext, hostClient runtimeclient.Client, hostConfig configuration.ClusterConfig) error { +func restartHostOperator(ctx context.Context, term ioutils.Terminal, hostClient runtimeclient.Client, hostConfig configuration.ClusterConfig) error { deployments := &appsv1.DeploymentList{} if err := hostClient.List(context.TODO(), deployments, runtimeclient.InNamespace(hostConfig.SandboxNamespace), @@ -101,7 +108,7 @@ func restartHostOperator(ctx *clicontext.CommandContext, hostClient runtimeclien "It's not possible to restart the Host Operator deployment", hostConfig.SandboxNamespace, len(deployments.Items)) } - return restartDeployment(ctx, hostClient, hostConfig, deployments.Items[0].Name) + return restartDeployment(ctx, term, hostClient, hostConfig, deployments.Items[0].Name) } func printExistingDeployments(term ioutils.Terminal, cl runtimeclient.Client, cfg configuration.ClusterConfig) error { @@ -117,11 +124,11 @@ func printExistingDeployments(term ioutils.Terminal, cl runtimeclient.Client, cf return nil } -func scaleToZero(cl runtimeclient.Client, namespacedName types.NamespacedName) (int32, error) { +func scaleToZero(ctx context.Context, cl runtimeclient.Client, namespacedName types.NamespacedName) (int32, error) { // get the deployment deployment := &appsv1.Deployment{} - if err := cl.Get(context.TODO(), namespacedName, deployment); err != nil { + if err := cl.Get(ctx, namespacedName, deployment); err != nil { return 0, err } // keep original number of replicas so we can bring it back @@ -130,7 +137,7 @@ func scaleToZero(cl runtimeclient.Client, namespacedName types.NamespacedName) ( deployment.Spec.Replicas = &zero // update the deployment so it scales to zero - return originalReplicas, cl.Update(context.TODO(), deployment) + return originalReplicas, cl.Update(ctx, deployment) } func scaleBack(term ioutils.Terminal, cl runtimeclient.Client, namespacedName types.NamespacedName, originalReplicas int32) error { diff --git a/pkg/cmd/adm/restart_test.go b/pkg/cmd/adm/restart_test.go index 10f21e7..3b4e56b 100644 --- a/pkg/cmd/adm/restart_test.go +++ b/pkg/cmd/adm/restart_test.go @@ -7,7 +7,6 @@ import ( "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/kubesaw/ksctl/pkg/configuration" - clicontext "github.com/kubesaw/ksctl/pkg/context" . "github.com/kubesaw/ksctl/pkg/test" "github.com/stretchr/testify/assert" @@ -37,13 +36,16 @@ func TestRestartDeployment(t *testing.T) { t.Run("restart is successful for "+clusterName, func(t *testing.T) { // given deployment := newDeployment(namespacedName, 3) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 3, &numberOfUpdateCalls) - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) + o := RestartOptions{ + term: term, + newClient: newClient, + } // when - err := restart(ctx, clusterName, "cool-deployment") + err := o.restart(context.Background(), clusterName, "cool-deployment") // then require.NoError(t, err) @@ -55,14 +57,17 @@ func TestRestartDeployment(t *testing.T) { t.Run("list deployments when no deployment name is provided for "+clusterName, func(t *testing.T) { // given deployment := newDeployment(namespacedName, 3) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 3, &numberOfUpdateCalls) term := NewFakeTerminalWithResponse("Y") - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) + o := RestartOptions{ + term: term, + newClient: newClient, + } // when - err := restart(ctx, clusterName) + err := o.restart(context.Background(), clusterName) // then require.EqualError(t, err, "at least one deployment name is required, include one or more of the above deployments to restart") @@ -75,16 +80,19 @@ func TestRestartDeployment(t *testing.T) { t.Run("restart fails - cannot get the deployment for "+clusterName, func(t *testing.T) { // given deployment := newDeployment(namespacedName, 3) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 3, &numberOfUpdateCalls) fakeClient.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { return fmt.Errorf("some error") } - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) + o := RestartOptions{ + term: term, + newClient: newClient, + } // when - err := restart(ctx, clusterName, "cool-deployment") + err := o.restart(context.Background(), clusterName, "cool-deployment") // then require.Error(t, err) @@ -96,14 +104,17 @@ func TestRestartDeployment(t *testing.T) { t.Run("restart fails - deployment not found for "+clusterName, func(t *testing.T) { // given deployment := newDeployment(namespacedName, 3) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 3, &numberOfUpdateCalls) term := NewFakeTerminalWithResponse("Y") - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) + o := RestartOptions{ + term: term, + newClient: newClient, + } // when - err := restart(ctx, clusterName, "wrong-deployment") + err := o.restart(context.Background(), clusterName, "wrong-deployment") // then require.NoError(t, err) @@ -131,14 +142,17 @@ func TestRestartDeploymentWithInsufficientPermissions(t *testing.T) { Name: "cool-deployment", } deployment := newDeployment(namespacedName, 3) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + newClient, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 3, &numberOfUpdateCalls) term := NewFakeTerminalWithResponse("Y") - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) + o := RestartOptions{ + term: term, + newClient: newClient, + } // when - err := restart(ctx, clusterName, "cool-deployment") + err := o.restart(context.Background(), clusterName, "cool-deployment") // then require.Error(t, err) @@ -162,13 +176,12 @@ func TestRestartHostOperator(t *testing.T) { // given deployment := newDeployment(namespacedName, 1) deployment.Labels = map[string]string{"olm.owner.namespace": "toolchain-host-operator"} - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + _, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 1, &numberOfUpdateCalls) - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) // when - err := restartHostOperator(ctx, fakeClient, cfg) + err := restartHostOperator(context.Background(), term, fakeClient, cfg) // then require.NoError(t, err) @@ -179,13 +192,12 @@ func TestRestartHostOperator(t *testing.T) { t.Run("host deployment with the label is not present - restart fails", func(t *testing.T) { // given deployment := newDeployment(namespacedName, 1) - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment) + _, _, fakeClient := NewFakeClients(t, deployment) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 1, &numberOfUpdateCalls) - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) // when - err := restartHostOperator(ctx, fakeClient, cfg) + err := restartHostOperator(context.Background(), term, fakeClient, cfg) // then require.Error(t, err) @@ -199,13 +211,12 @@ func TestRestartHostOperator(t *testing.T) { deployment.Labels = map[string]string{"olm.owner.namespace": "toolchain-host-operator"} deployment2 := deployment.DeepCopy() deployment2.Name = "another" - newClient, newRESTClient, fakeClient := NewFakeClients(t, deployment, deployment2) + _, _, fakeClient := NewFakeClients(t, deployment, deployment2) numberOfUpdateCalls := 0 fakeClient.MockUpdate = requireDeploymentBeingUpdated(t, fakeClient, namespacedName, 1, &numberOfUpdateCalls) - ctx := clicontext.NewCommandContext(term, newClient, newRESTClient) // when - err := restartHostOperator(ctx, fakeClient, cfg) + err := restartHostOperator(context.Background(), term, fakeClient, cfg) // then require.Error(t, err) diff --git a/pkg/cmd/adm/unregister_member.go b/pkg/cmd/adm/unregister_member.go index 674a0cf..fbf1fe5 100644 --- a/pkg/cmd/adm/unregister_member.go +++ b/pkg/cmd/adm/unregister_member.go @@ -62,5 +62,5 @@ func UnregisterMemberCluster(ctx *clicontext.CommandContext, clusterName string) } ctx.Printlnf("\nThe deletion of the Toolchain member cluster from the Host cluster has been triggered") - return restartHostOperator(ctx, hostClusterClient, hostClusterConfig) + return restartHostOperator(ctx.Context, ctx.Terminal, hostClusterClient, hostClusterConfig) }