Skip to content

Commit

Permalink
Merge pull request #2490 from hunterkepley/ocm-11364
Browse files Browse the repository at this point in the history 
OCM-11364 | fix: Regression, HCP nodepool creation validation used for classic
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Sep 23, 2024
2 parents e041f05 + 7464a08 commit 19b7ec7
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 23 deletions.
8 changes: 6 additions & 2 deletions pkg/machinepool/helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,15 +205,19 @@ func maxReplicaValidator(minReplicas int, multiAZMachinePool bool) interactive.V
}
}

func minReplicaValidator(multiAZMachinePool bool, autoscaling bool) interactive.Validator {
func minReplicaValidator(multiAZMachinePool bool, autoscaling bool, isHypershift bool) interactive.Validator {
return func(val interface{}) error {
minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
if err != nil {
return err
}
if autoscaling && minReplicas < 1 {
if autoscaling && minReplicas < 1 && isHypershift {
return fmt.Errorf("min-replicas must be greater than zero")
}
if autoscaling && minReplicas < 0 && !isHypershift {
return fmt.Errorf("min-replicas must be a number that is 0 or greater when autoscaling is" +
" enabled")
}
if !autoscaling && minReplicas < 0 {
return fmt.Errorf("Replicas must be a non-negative integer")
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/machinepool/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package machinepool
import (
"fmt"

gomock "go.uber.org/mock/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"
Expand Down Expand Up @@ -417,7 +417,7 @@ var _ = Describe("getSecurityGroupsOption", func() {
var _ = Describe("Machine pool min/max replicas validation", func() {
DescribeTable("Machine pool min replicas validation",
func(minReplicas int, autoscaling bool, multiAZ bool, hasError bool) {
err := minReplicaValidator(multiAZ, autoscaling)(minReplicas)
err := minReplicaValidator(multiAZ, autoscaling, false)(minReplicas)
if hasError {
Expect(err).To(HaveOccurred())
} else {
Expand All @@ -440,7 +440,7 @@ var _ = Describe("Machine pool min/max replicas validation", func() {
0,
true,
false,
true,
false,
),
Entry("One replicas - autoscaling",
1,
Expand Down
22 changes: 12 additions & 10 deletions pkg/machinepool/machinepool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,8 @@ func (m *machinePool) CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, clu
}
}

minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool)
minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool,
cluster.Hypershift().Enabled())
if err != nil {
return err
}
Expand Down Expand Up @@ -588,7 +589,8 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust
}
}

minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, false)
minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, false,
cluster.Hypershift().Enabled())
if err != nil {
return err
}
Expand Down Expand Up @@ -1323,7 +1325,7 @@ func getMachinePoolReplicas(cmd *cobra.Command,
Default: minReplicas,
Required: replicasRequired,
Validators: []interactive.Validator{
minReplicaValidator(multiAZ, false),
minReplicaValidator(multiAZ, false, false),
},
})
if err != nil {
Expand Down Expand Up @@ -1879,7 +1881,7 @@ func getNodePoolReplicas(cmd *cobra.Command,
Default: existingAutoscaling.MinReplica(),
Required: replicasRequired,
Validators: []interactive.Validator{
minReplicaValidator(false, true),
minReplicaValidator(false, true, true),
},
})
if err != nil {
Expand Down Expand Up @@ -1918,7 +1920,7 @@ func getNodePoolReplicas(cmd *cobra.Command,
Default: replicas,
Required: true,
Validators: []interactive.Validator{
minReplicaValidator(false, false),
minReplicaValidator(false, false, true),
},
})
if err != nil {
Expand Down Expand Up @@ -1956,7 +1958,7 @@ func editAutoscaling(nodePool *cmv1.NodePool, minReplicas int, maxReplicas int)
return nil
}
func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions,
multiAZMachinePool bool) (minReplicas, maxReplicas, replicas int, autoscaling bool, err error) {
multiAZMachinePool bool, isHypershift bool) (minReplicas, maxReplicas, replicas int, autoscaling bool, err error) {
isMinReplicasSet := cmd.Flags().Changed("min-replicas")
isMaxReplicasSet := cmd.Flags().Changed("max-replicas")
isAutoscalingSet := cmd.Flags().Changed("enable-autoscaling")
Expand Down Expand Up @@ -1996,15 +1998,15 @@ func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOption
Default: minReplicas,
Required: true,
Validators: []interactive.Validator{
minReplicaValidator(multiAZMachinePool, autoscaling),
minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift),
},
})
if err != nil {
return minReplicas, maxReplicas, replicas, autoscaling,
fmt.Errorf("Expected a valid number of min replicas: %s", err)
}
}
err = minReplicaValidator(multiAZMachinePool, autoscaling)(minReplicas)
err = minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift)(minReplicas)
if err != nil {
return minReplicas, maxReplicas, replicas, autoscaling, err
}
Expand Down Expand Up @@ -2044,14 +2046,14 @@ func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOption
Default: replicas,
Required: true,
Validators: []interactive.Validator{
minReplicaValidator(multiAZMachinePool, autoscaling),
minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift),
},
})
if err != nil {
return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of replicas: %s", err)
}
}
err = minReplicaValidator(multiAZMachinePool, autoscaling)(replicas)
err = minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift)(replicas)
if err != nil {
return minReplicas, maxReplicas, replicas, autoscaling, err
}
Expand Down
16 changes: 8 additions & 8 deletions pkg/machinepool/machinepool_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import (
"io"
"net/http"
"os"
reflect "reflect"
"reflect"
"time"

gomock "go.uber.org/mock/gomock"
"go.uber.org/mock/gomock"

ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -515,7 +515,7 @@ var _ = Describe("Utility Functions", func() {
var validator interactive.Validator

BeforeEach(func() {
validator = minReplicaValidator(true, false) // or false for non-multiAZ
validator = minReplicaValidator(true, false, false) // or false for non-multiAZ
})

When("input is non-integer", func() {
Expand Down Expand Up @@ -1532,7 +1532,7 @@ var _ = Describe("ManageReplicas", func() {
args.AutoscalingEnabled = true
cmd.Flags().Int32("replicas", 1, "Replicas of the machine pool")
cmd.Flags().Set("replicas", "1")
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool)
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Replicas can't be set when autoscaling is enabled"))
Expect(autoscaling).To(BeTrue())
Expand All @@ -1545,7 +1545,7 @@ var _ = Describe("ManageReplicas", func() {
cmd.Flags().Set("max-replicas", "6")
args.MinReplicas = 3
args.MaxReplicas = 6
_, _, _, _, err := manageReplicas(cmd, args, multiAZMachinePool)
_, _, _, _, err := manageReplicas(cmd, args, multiAZMachinePool, false)
Expect(err).ToNot(HaveOccurred())
})
})
Expand All @@ -1559,7 +1559,7 @@ var _ = Describe("ManageReplicas", func() {
cmd.Flags().Set("max-replicas", "3")
args.MinReplicas = 1
args.MaxReplicas = 3
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool)
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Autoscaling must be enabled in order to set min and max replicas"))
Expect(autoscaling).To(BeFalse())
Expand All @@ -1568,7 +1568,7 @@ var _ = Describe("ManageReplicas", func() {
args.AutoscalingEnabled = false
cmd.Flags().Int32("replicas", 1, "Replicas of the machine pool")
cmd.Flags().Set("replicas", "1")
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool)
_, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false)
Expect(err).ToNot(HaveOccurred())
Expect(autoscaling).To(BeFalse())
})
Expand All @@ -1594,7 +1594,7 @@ var _ = Describe("Utility Functions", func() {
var validator interactive.Validator

BeforeEach(func() {
validator = minReplicaValidator(true, false) // or false for non-multiAZ
validator = minReplicaValidator(true, false, false) // or false for non-multiAZ
})

It("should return error for non-integer input", func() {
Expand Down

0 comments on commit 19b7ec7

Please sign in to comment.