OCM-7186 | feat: Adding unit tests for CreateMachinePool + CreateNode…

…Pool

cmd/create/machinepool/cmd.go

@@ -20,19 +20,15 @@ import (
 	"context"
 	"fmt"
 
-	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/spf13/cobra"
 
 	"github.com/openshift/rosa/pkg/aws"
-	mpHelpers "github.com/openshift/rosa/pkg/helper/machinepools"
 	"github.com/openshift/rosa/pkg/machinepool"
 	mpOpts "github.com/openshift/rosa/pkg/options/machinepool"
 	"github.com/openshift/rosa/pkg/properties"
 	"github.com/openshift/rosa/pkg/rosa"
 )
 
-var args machinepool.MachinePoolArgs
-
 type CreateMachinePoolSpec struct {
 	Service machinepool.MachinePoolService
 }
@@ -53,33 +49,26 @@ func NewCreateMachinePoolCommand() *cobra.Command {
 	return cmd
 }
 
-// Create machine pool based on cluster type
-func (m CreateMachinePool) createMachinePoolBasedOnClusterType(r *rosa.Runtime,
-	cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster,
-	options *mpOpts.CreateMachinepoolUserOptions) error {
-	if cluster.Hypershift().Enabled() {
-		return m.service.CreateNodePools(r, cmd, clusterKey, cluster, options)
-	}
-	return m.service.CreateMachinePool(r, cmd, clusterKey, cluster, options)
-}
-
 // Original function refactored to use the new helper functions
 func CreateMachinepoolRunner(userOptions *mpOpts.CreateMachinepoolUserOptions) rosa.CommandRunner {
 	return func(ctx context.Context, r *rosa.Runtime, cmd *cobra.Command, argv []string) error {
 		var err error
 		options := NewCreateMachinepoolOptions()
 		clusterKey := r.GetClusterKey()
 		options.args = userOptions
+		newService := NewCreateMachinePool(CreateMachinePoolSpec{
+			Service: machinepool.NewMachinePoolService(),
+		})
 
 		cluster := r.FetchCluster()
-		if err := validateClusterState(cluster, clusterKey); err != nil {
+		if err := machinepool.ValidateClusterState(cluster, clusterKey); err != nil {
 			return err
 		}
 
 		val, ok := cluster.Properties()[properties.UseLocalCredentials]
 		useLocalCredentials := ok && val == "true"
 
-		if err := validateLabels(cmd, options.args); err != nil {
+		if err := machinepool.ValidateLabels(cmd, options.args); err != nil {
 			return err
 		}
 
@@ -91,28 +80,8 @@ func CreateMachinepoolRunner(userOptions *mpOpts.CreateMachinepoolUserOptions) r
 		if err != nil {
 			return fmt.Errorf("Failed to create awsClient: %s", err)
 		}
-		service := NewCreateMachinePool(CreateMachinePoolSpec{
-			Service: machinepool.NewMachinePoolService(),
-		})
 
-		return service.createMachinePoolBasedOnClusterType(r, cmd, clusterKey, cluster, options.Machinepool())
-	}
-}
-
-// Validate the cluster's state is ready
-func validateClusterState(cluster *cmv1.Cluster, clusterKey string) error {
-	if cluster.State() != cmv1.ClusterStateReady {
-		return fmt.Errorf("Cluster '%s' is not yet ready", clusterKey)
-	}
-	return nil
-}
-
-// Parse labels if the 'labels' flag is set
-func validateLabels(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions) error {
-	if cmd.Flags().Changed("labels") {
-		if _, err := mpHelpers.ParseLabels(args.Labels); err != nil {
-			return fmt.Errorf("%s", err)
-		}
+		return newService.service.CreateMachinePoolBasedOnClusterType(r,
+			cmd, clusterKey, cluster, options.Machinepool())
 	}
-	return nil
 }

cmd/create/machinepool/cmd_test.go

@@ -28,22 +28,18 @@ var _ = Describe("Create machine pool", func() {
 		When("cluster is classic", func() {
 			It("should create machine pool", func() {
 				serviceMock := machinepool.NewMockMachinePoolService(ctrl)
-				serviceMock.EXPECT().CreateMachinePool(gomock.Any(), gomock.Any(),
+				serviceMock.EXPECT().CreateMachinePoolBasedOnClusterType(gomock.Any(), gomock.Any(),
 					gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1)
 
-				createMachinepool := NewCreateMachinePool(CreateMachinePoolSpec{
-					Service: serviceMock,
-				})
-
 				mockClassicClusterReady := test.MockCluster(func(c *cmv1.ClusterBuilder) {
 					c.AWS(cmv1.NewAWS().SubnetIDs("subnet-0b761d44d3d9a4663", "subnet-0f87f640e56934cbc"))
 					c.Region(cmv1.NewCloudRegion().ID("us-east-1"))
 					c.State(cmv1.ClusterStateReady)
 					c.Hypershift(cmv1.NewHypershift().Enabled(false))
 				})
-				err := createMachinepool.createMachinePoolBasedOnClusterType(rosa.NewRuntime(), NewCreateMachinePoolCommand(),
+				err := serviceMock.CreateMachinePoolBasedOnClusterType(rosa.NewRuntime(), NewCreateMachinePoolCommand(),
 					"82339823", mockClassicClusterReady, NewCreateMachinepoolUserOptions())
-				Expect(err).To(BeNil())
+				Expect(err).ToNot(HaveOccurred())
 
 			})
 		})
@@ -76,8 +72,8 @@ var _ = Describe("Validation functions", func() {
 
 	Context("validateClusterState", func() {
 		It("should return nil when the cluster state is ready", func() {
-			err := validateClusterState(mockClassicClusterReady, "test-cluster")
-			Expect(err).To(BeNil())
+			err := machinepool.ValidateClusterState(mockClassicClusterReady, "test-cluster")
+			Expect(err).ToNot(HaveOccurred())
 		})
 
 		It("should return an error when the cluster state is not ready", func() {
@@ -87,30 +83,29 @@ var _ = Describe("Validation functions", func() {
 				c.State(cmv1.ClusterStateInstalling)
 				c.Hypershift(cmv1.NewHypershift().Enabled(false))
 			})
-			err := validateClusterState(mockClassicClusterInstalling, "test-cluster")
+			err := machinepool.ValidateClusterState(mockClassicClusterInstalling, "test-cluster")
 			Expect(err).To(MatchError("Cluster 'test-cluster' is not yet ready"))
 		})
 	})
 
 	Context("validateLabels", func() {
 		It("should return nil when the labels flag has not changed", func() {
-			err := validateLabels(mockCmd, mockArgs)
-			Expect(err).To(BeNil())
+			err := machinepool.ValidateLabels(mockCmd, mockArgs)
+			Expect(err).ToNot(HaveOccurred())
 		})
 
 		It("should return nil when the labels flag is set with valid labels", func() {
 			mockArgs.Labels = "key1=value1,key2=value2"
 			mockCmd.Flags().Set("labels", "key1=value1,key2=value2")
-			err := validateLabels(mockCmd, mockArgs)
-			Expect(err).To(BeNil())
+			err := machinepool.ValidateLabels(mockCmd, mockArgs)
+			Expect(err).ToNot(HaveOccurred())
 		})
 
 		It("should return an error when the labels flag is set with invalid labels", func() {
 			mockArgs.Labels = "key1=value1,key2"
 			mockCmd.Flags().Set("labels", "key1=value1,key2")
-			err := validateLabels(mockCmd, mockArgs)
-			Expect(err).NotTo(BeNil())
+			err := machinepool.ValidateLabels(mockCmd, mockArgs)
+			Expect(err).To(HaveOccurred())
 		})
 	})
 })
-

cmd/create/machinepool/options.go

@@ -5,6 +5,8 @@ import (
 	"github.com/openshift/rosa/pkg/reporter"
 )
 
+const instanceType = "m5.xlarge"
+
 type CreateMachinepoolOptions struct {
 	reporter *reporter.Object
 
@@ -13,7 +15,7 @@ type CreateMachinepoolOptions struct {
 
 func NewCreateMachinepoolUserOptions() *mpOpts.CreateMachinepoolUserOptions {
 	return &mpOpts.CreateMachinepoolUserOptions{
-		InstanceType:          "m5.xlarge",
+		InstanceType:          instanceType,
 		AutoscalingEnabled:    false,
 		MultiAvailabilityZone: true,
 		Autorepair:            true,
@@ -30,11 +32,3 @@ func NewCreateMachinepoolOptions() *CreateMachinepoolOptions {
 func (m *CreateMachinepoolOptions) Machinepool() *mpOpts.CreateMachinepoolUserOptions {
 	return m.args
 }
-
-func (m *CreateMachinepoolOptions) Bind(args *mpOpts.CreateMachinepoolUserOptions, argv []string) error {
-	m.args = args
-	if len(argv) > 0 {
-		m.args.Name = argv[0]
-	}
-	return nil
-}

cmd/create/machinepool/options_test.go

@@ -19,41 +19,26 @@ var _ = Describe("CreateMachinepoolOptions", func() {
 		userOptions = NewCreateMachinepoolUserOptions()
 	})
 
-	Describe("NewCreateMachinepoolUserOptions", func() {
+	Context("NewCreateMachinepoolUserOptions", func() {
 		It("should create default user options", func() {
-			Expect(userOptions.InstanceType).To(Equal("m5.xlarge"))
+			Expect(userOptions.InstanceType).To(Equal(instanceType))
 			Expect(userOptions.AutoscalingEnabled).To(BeFalse())
 			Expect(userOptions.MultiAvailabilityZone).To(BeTrue())
 			Expect(userOptions.Autorepair).To(BeTrue())
 		})
 	})
 
-	Describe("NewCreateMachinepoolOptions", func() {
+	Context("NewCreateMachinepoolOptions", func() {
 		It("should create default machine pool options", func() {
 			Expect(machinepoolOptions.reporter).To(BeAssignableToTypeOf(&reporter.Object{}))
 			Expect(machinepoolOptions.args).To(BeAssignableToTypeOf(&mpOpts.CreateMachinepoolUserOptions{}))
 		})
 	})
 
-	Describe("Machinepool", func() {
+	Context("Machinepool", func() {
 		It("should return the args field", func() {
 			machinepoolOptions.args = userOptions
 			Expect(machinepoolOptions.Machinepool()).To(Equal(userOptions))
 		})
 	})
-
-	Describe("Bind", func() {
-		It("should bind the passed arguments", func() {
-			argv := []string{"test-pool"}
-			Expect(machinepoolOptions.Bind(userOptions, argv)).To(Succeed())
-			Expect(machinepoolOptions.args).To(Equal(userOptions))
-			Expect(machinepoolOptions.args.Name).To(Equal("test-pool"))
-		})
-
-		It("should not modify args.Name if no arguments passed", func() {
-			initialName := userOptions.Name
-			Expect(machinepoolOptions.Bind(userOptions, []string{})).To(Succeed())
-			Expect(machinepoolOptions.args.Name).To(Equal(initialName))
-		})
-	})
 })

cmd/create/machinepool/suite_test.go

@@ -0,0 +1,13 @@
+package machinepool
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestMachinepools(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Machinepool/nodepool suite")
+}

cmd/describe/autoscaler/cmd_test.go

@@ -196,4 +196,3 @@ var _ = Describe("rosa describe autoscaler", func() {
 		})
 	})
 })
-

pkg/machinepool/helper.go

@@ -2,7 +2,6 @@ package machinepool
 
 import (
 	"fmt"
-	"os"
 	"strconv"
 
 	awssdk "github.com/aws/aws-sdk-go-v2/aws"
@@ -12,12 +11,31 @@ import (
 	"github.com/spf13/cobra"
 
 	"github.com/openshift/rosa/pkg/aws"
+	mpHelpers "github.com/openshift/rosa/pkg/helper/machinepools"
 	"github.com/openshift/rosa/pkg/interactive"
 	interactiveSgs "github.com/openshift/rosa/pkg/interactive/securitygroups"
 	mpOpts "github.com/openshift/rosa/pkg/options/machinepool"
 	"github.com/openshift/rosa/pkg/rosa"
 )
 
+// Parse labels if the 'labels' flag is set
+func ValidateLabels(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions) error {
+	if cmd.Flags().Changed("labels") {
+		if _, err := mpHelpers.ParseLabels(args.Labels); err != nil {
+			return fmt.Errorf("%s", err)
+		}
+	}
+	return nil
+}
+
+// Validate the cluster's state is ready
+func ValidateClusterState(cluster *cmv1.Cluster, clusterKey string) error {
+	if cluster.State() != cmv1.ClusterStateReady {
+		return fmt.Errorf("Cluster '%s' is not yet ready", clusterKey)
+	}
+	return nil
+}
+
 func getSubnetFromUser(cmd *cobra.Command, r *rosa.Runtime, isSubnetSet bool,
 	cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) (string, error) {
 	var selectSubnet bool
@@ -215,7 +233,7 @@ func spotMaxPriceValidator(val interface{}) error {
 }
 
 func getSubnetFromAvailabilityZone(cmd *cobra.Command, r *rosa.Runtime, isAvailabilityZoneSet bool,
-	cluster *cmv1.Cluster, args MachinePoolArgs) (string, error) {
+	cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) (string, error) {
 
 	privateSubnets, err := r.AWSClient.GetVPCPrivateSubnets(cluster.AWS().SubnetIDs()[0])
 	if err != nil {
@@ -260,7 +278,10 @@ func getSubnetFromAvailabilityZone(cmd *cobra.Command, r *rosa.Runtime, isAvaila
 		}
 		r.Reporter.Infof("There are several subnets for availability zone '%s'", availabilityZone)
 		interactive.Enable()
-		subnet := getSubnetFromUser(cmd, r, false, cluster, args)
+		subnet, err := getSubnetFromUser(cmd, r, false, cluster, args)
+		if err != nil {
+			return "", err
+		}
 		return subnet, nil
 	}
 

pkg/machinepool/helper_test.go

@@ -1,6 +1,11 @@
 package machinepool
 
 import (
+	"fmt"
+
+	gomock "go.uber.org/mock/gomock"
+
+	awssdk "github.com/aws/aws-sdk-go-v2/aws"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -9,10 +14,10 @@ import (
 
 	mock "github.com/openshift/rosa/pkg/aws"
 	"github.com/openshift/rosa/pkg/helper/features"
+	mpOpts "github.com/openshift/rosa/pkg/options/machinepool"
 	"github.com/openshift/rosa/pkg/rosa"
 	"github.com/openshift/rosa/pkg/test"
 	. "github.com/openshift/rosa/pkg/test"
-	mpOpts "github.com/openshift/rosa/pkg/options/machinepool"
 )
 
 var _ = Describe("Machine pool helper", func() {
@@ -195,7 +200,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() {
 	var (
 		r              *rosa.Runtime
 		cmd            *cobra.Command
-		args           *MachinePoolArgs
+		args           *mpOpts.CreateMachinepoolUserOptions
 		mockClient     *mock.MockClient
 		az             string
 		subnetId1      string
@@ -211,7 +216,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() {
 		az = "us-east-1a"
 		subnetId1 = "subnet-123"
 		subnetId2 = "subnet-456"
-		args = &MachinePoolArgs{}
+		args = &mpOpts.CreateMachinepoolUserOptions{}
 	})
 
 	When("no availability zone is set", func() {
@@ -228,7 +233,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() {
 		})
 
 		It("returns the correct subnet when one subnet is expected", func() {
-			subnet, err := getSubnetFromAvailabilityZone(cmd, r, false, cluster, *args)
+			subnet, err := getSubnetFromAvailabilityZone(cmd, r, false, cluster, args)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(subnet).To(Equal(subnetId1))
 		})
@@ -249,7 +254,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() {
 		})
 
 		It("handles errors correctly when the availability zone does not match", func() {
-			subnet, err := getSubnetFromAvailabilityZone(cmd, r, true, cluster, *args)
+			subnet, err := getSubnetFromAvailabilityZone(cmd, r, true, cluster, args)
 			Expect(err).To(HaveOccurred())
 			Expect(subnet).To(Equal(""))
 		})