OCM-9917 | fix: Bug with min/max replicas (edit machinepool)

cmd/edit/machinepool/cmd_test.go

@@ -41,7 +41,6 @@ var _ = Describe("Edit Machinepool", func() {
 			c.State(cmv1.ClusterStateReady)
 			c.Hypershift(cmv1.NewHypershift().Enabled(false))
 		})
-		classicClusterReady := test.FormatClusterList([]*cmv1.Cluster{mockClassicClusterReady})
 
 		version := cmv1.NewVersion().ID("4.12.24").RawID("openshift-4.12.24")
 		awsNodePool := cmv1.NewAWSNodePool().InstanceType("m5.xlarge")
@@ -69,9 +68,8 @@ var _ = Describe("Edit Machinepool", func() {
 			t.SetCluster("", nil)
 		})
 
-		Describe("Machinepools", func() {
+		Describe("Machinepools", Ordered, func() {
 			It("Able to edit machinepool with no issues", func() {
-				t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, classicClusterReady))
 				// First get
 				t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, mpResponse))
 				// Edit
@@ -87,7 +85,6 @@ var _ = Describe("Edit Machinepool", func() {
 					nodePoolId, "--min-replicas", "2", "--enable-autoscaling", "true", "-y"})).To(Succeed())
 			})
 			It("Machinepool ID passed in without flag in random location", func() {
-				t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, classicClusterReady))
 				// First get
 				t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, mpResponse))
 				// Edit
@@ -135,6 +132,8 @@ var _ = Describe("Edit Machinepool", func() {
 				args := NewEditMachinepoolUserOptions()
 				args.machinepool = nodePoolId
 				args.autoscalingEnabled = true
+				args.maxReplicas = 10
+				args.minReplicas = 2
 				runner := EditMachinePoolRunner(args)
 				cmd := NewEditMachinePoolCommand()
 				Expect(cmd.Flag("cluster").Value.Set(clusterId)).To(Succeed())

cmd/edit/machinepool/options.go

@@ -53,7 +53,7 @@ func (m *EditMachinepoolOptions) Bind(args *EditMachinepoolUserOptions, argv []s
 	}
 
 	if m.args.machinepool == "" {
-		return fmt.Errorf("you need to specify a machine pool name")
+		return fmt.Errorf("You need to specify a machine pool name")
 	}
 
 	if m.args.labels != "" {
@@ -63,5 +63,17 @@ func (m *EditMachinepoolOptions) Bind(args *EditMachinepoolUserOptions, argv []s
 		}
 	}
 
+	if m.args.autoscalingEnabled {
+		if m.args.minReplicas <= 0 {
+			return fmt.Errorf("Min replicas must be greater than zero when autoscaling is enabled")
+		}
+		if m.args.maxReplicas <= 0 {
+			return fmt.Errorf("Max replicas must be greater than zero when autoscaling is enabled")
+		}
+		if m.args.minReplicas > m.args.maxReplicas {
+			return fmt.Errorf("Min replicas must be less than max replicas")
+		}
+	}
+
 	return nil
 }

cmd/edit/machinepool/options_test.go

@@ -15,6 +15,9 @@ var _ = Describe("Test edit machinepool options", func() {
 	})
 	Context("Edit Machinepool Options", func() {
 		var options EditMachinepoolOptions
+		BeforeEach(func() {
+			args = NewEditMachinepoolUserOptions()
+		})
 		It("Create args from options using Bind (also tests MachinePool func)", func() {
 			// Set value then bind
 			testMachinepool := "test"
@@ -25,7 +28,7 @@ var _ = Describe("Test edit machinepool options", func() {
 		It("Fail to bind args due to empty machinepool name", func() {
 			args.machinepool = ""
 			err := options.Bind(args, []string{})
-			Expect(err).To(MatchError("you need to specify a machine pool name"))
+			Expect(err).To(MatchError("You need to specify a machine pool name"))
 		})
 		It("Test Bind with argv instead of normal args (single arg, no flag for machinepool)", func() {
 			argv := []string{"test-id"}
@@ -48,5 +51,29 @@ var _ = Describe("Test edit machinepool options", func() {
 			err := options.Bind(args, []string{})
 			Expect(err).To(MatchError("Expected key=value format for labels"))
 		})
+		It("Test min replicas negative value (fail)", func() {
+			args.minReplicas = -1
+			args.maxReplicas = 1
+			args.autoscalingEnabled = true
+			args.machinepool = "test"
+			err := options.Bind(args, []string{})
+			Expect(err).To(MatchError("Min replicas must be greater than zero when autoscaling is enabled"))
+		})
+		It("Test max replicas negative value (fail)", func() {
+			args.maxReplicas = -1
+			args.minReplicas = 1
+			args.autoscalingEnabled = true
+			args.machinepool = "test"
+			err := options.Bind(args, []string{})
+			Expect(err).To(MatchError("Max replicas must be greater than zero when autoscaling is enabled"))
+		})
+		It("Test min replicas > max replicas (fail)", func() {
+			args.maxReplicas = 1
+			args.minReplicas = 5
+			args.autoscalingEnabled = true
+			args.machinepool = "test"
+			err := options.Bind(args, []string{})
+			Expect(err).To(MatchError("Min replicas must be less than max replicas"))
+		})
 	})
 })

pkg/machinepool/machinepool.go

@@ -1321,8 +1321,7 @@ func (m *machinePool) EditMachinePool(cmd *cobra.Command, machinePoolId string,
 	if cluster.Hypershift().Enabled() {
 		return editNodePool(cmd, machinePoolId, clusterKey, cluster, r)
 	}
-	editMachinePool(cmd, machinePoolId, clusterKey, cluster, r)
-	return nil
+	return editMachinePool(cmd, machinePoolId, clusterKey, cluster, r)
 }
 
 // fillAutoScalingAndReplicas is filling either autoscaling or replicas value in the builder
@@ -1531,16 +1530,10 @@ func editMachinePool(cmd *cobra.Command, machinePoolId string,
 		return fmt.Errorf("Multi AZ clusters require that the number of MachinePool replicas be a multiple of 3")
 	}
 
-	labels, err := cmd.Flags().GetString("labels")
-	if err != nil {
-		return fmt.Errorf("Failed to get inputted labels: '%s'", err)
-	}
+	labels := cmd.Flags().Lookup("labels").Value.String()
 	labelMap := mpHelpers.GetLabelMap(cmd, r, machinePool.Labels(), labels)
 
-	taints, err := cmd.Flags().GetString("taints")
-	if err != nil {
-		return fmt.Errorf("Failed to get inputted taints: '%s'", err)
-	}
+	taints := cmd.Flags().Lookup("taints").Value.String()
 	taintBuilders := mpHelpers.GetTaints(cmd, r, machinePool.Taints(), taints)
 
 	mpBuilder := cmv1.NewMachinePool().
@@ -1628,7 +1621,17 @@ func editNodePool(cmd *cobra.Command, nodePoolID string,
 	}
 
 	if autoscaling && cmd.Flags().Changed("min-replicas") && minReplicas < 1 {
-		return fmt.Errorf("The number of machine pool min-replicas needs to be greater than zero")
+		return fmt.Errorf("The number of machine pool min-replicas needs to be a non-negative integer")
+	}
+
+	if autoscaling && cmd.Flags().Changed("max-replicas") && maxReplicas < 1 {
+		return fmt.Errorf("The number of machine pool max-replicas needs to be a non-negative integer")
+	}
+
+	if autoscaling && cmd.Flags().Changed("max-replicas") && cmd.Flags().Changed(
+		"min-replicas") && minReplicas > maxReplicas {
+		return fmt.Errorf("The number of machine pool min-replicas needs to be less than the number of " +
+			"machine pool max-replicas")
 	}
 
 	labels := cmd.Flags().Lookup("labels").Value.String()