Review fixes v4

Author: chrischdi

internal/controllers/machine/machine_controller_noderef_test.go

@@ -85,40 +85,44 @@ func TestReconcileNode(t *testing.T) {
 	}
 
 	testCases := []struct {
-		name               string
-		machine            *clusterv1.Machine
-		node               *corev1.Node
-		nodeGetErr         bool
-		expectResult       ctrl.Result
-		expectError        bool
-		expected           func(g *WithT, m *clusterv1.Machine)
-		expectNodeGetError bool
-		expectedNode       func(g *WithT, m *corev1.Node)
+		name                            string
+		machine                         *clusterv1.Machine
+		node                            *corev1.Node
+		featureGateMachineTaintsEnabled bool
+		nodeGetErr                      bool
+		expectResult                    ctrl.Result
+		expectError                     bool
+		expected                        func(g *WithT, m *clusterv1.Machine)
+		expectNodeGetError              bool
+		expectedNode                    func(g *WithT, m *corev1.Node)
 	}{
 		{
-			name:         "No op if provider ID is not set",
-			machine:      &clusterv1.Machine{},
-			node:         nil,
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			name:                            "No op if provider ID is not set",
+			machine:                         &clusterv1.Machine{},
+			node:                            nil,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 		},
 		{
-			name:               "err reading node (something different than not found), it should return error",
-			machine:            defaultMachine.DeepCopy(),
-			node:               nil,
-			nodeGetErr:         true,
-			expectResult:       ctrl.Result{},
-			expectError:        true,
-			expectNodeGetError: true,
+			name:                            "err reading node (something different than not found), it should return error",
+			machine:                         defaultMachine.DeepCopy(),
+			node:                            nil,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      true,
+			expectResult:                    ctrl.Result{},
+			expectError:                     true,
+			expectNodeGetError:              true,
 		},
 		{
-			name:         "waiting for the node to exist, no op",
-			machine:      defaultMachine.DeepCopy(),
-			node:         nil,
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			name:                            "waiting for the node to exist, no op",
+			machine:                         defaultMachine.DeepCopy(),
+			node:                            nil,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 		},
 		{
 			name:    "node found, should surface info",
@@ -146,9 +150,10 @@ func TestReconcileNode(t *testing.T) {
 					},
 				},
 			},
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.NodeRef.Name).To(Equal("test-node-1"))
 				g.Expect(m.Status.NodeInfo).ToNot(BeNil())
@@ -174,10 +179,11 @@ func TestReconcileNode(t *testing.T) {
 					},
 				},
 			},
-			node:         nil,
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  true,
+			node:                            nil,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     true,
 		},
 		{
 			name: "node not found is tolerated when machine is deleting",
@@ -200,10 +206,11 @@ func TestReconcileNode(t *testing.T) {
 					},
 				},
 			},
-			node:         nil,
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			node:                            nil,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 		},
 		{
 			name:    "node found, should propagate taints",
@@ -227,9 +234,10 @@ func TestReconcileNode(t *testing.T) {
 					},
 				},
 			},
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			featureGateMachineTaintsEnabled: true,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 			expectedNode: func(g *WithT, n *corev1.Node) {
 				g.Expect(n.Spec.Taints).To(BeComparableTo([]corev1.Taint{
 					{
@@ -248,7 +256,7 @@ func TestReconcileNode(t *testing.T) {
 		},
 		{
 			name:    "node found, should not add taints annotation if taints feature gate is disabled",
-			machine: defaultMachine.DeepCopy(), // The test only enables the feature gate if machine has taints.
+			machine: defaultMachineWithTaints.DeepCopy(), // The test only enables the feature gate if machine has taints.
 			node: &corev1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-node-1",
@@ -268,9 +276,10 @@ func TestReconcileNode(t *testing.T) {
 					},
 				},
 			},
-			nodeGetErr:   false,
-			expectResult: ctrl.Result{},
-			expectError:  false,
+			featureGateMachineTaintsEnabled: false,
+			nodeGetErr:                      false,
+			expectResult:                    ctrl.Result{},
+			expectError:                     false,
 			expectedNode: func(g *WithT, n *corev1.Node) {
 				g.Expect(n.Spec.Taints).To(BeEmpty())
 				g.Expect(n.Annotations).ToNot(HaveKey(clusterv1.TaintsFromMachineAnnotation))
@@ -282,7 +291,7 @@ func TestReconcileNode(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			if tc.machine.Spec.Taints != nil {
+			if tc.featureGateMachineTaintsEnabled {
 				utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachineTaintPropagation, true)
 			}
 

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -422,7 +422,7 @@ func TestMachineDeploymentReconciler(t *testing.T) {
 
 		// Update the taints of the MachineDeployment,
 		// expect the Reconcile to be called and the MachineSet to be updated in-place.
-		t.Log("Updating deletion.order on the MachineDeployment")
+		t.Log("Updating template.spec.taints on the MachineDeployment")
 		additionalTaint := clusterv1.MachineTaint{
 			Key:         "additional-taint-key",
 			Value:       "additional-taint-value",

internal/webhooks/machinepool.go

@@ -192,7 +192,7 @@ func (webhook *MachinePool) validate(oldObj, newObj *clusterv1.MachinePool) erro
 	if len(newObj.Spec.Template.Spec.Taints) > 0 {
 		allErrs = append(allErrs, field.Forbidden(specPath.Child("taints"), "taints feature for MachinePools is not yet implemented"))
 		if !feature.Gates.Enabled(feature.MachineTaintPropagation) {
-			allErrs = append(allErrs, field.Forbidden(specPath.Child("taints"), "taints are not allowed to be set  when the feature gate MachineTaintPropagation is disabled"))
+			allErrs = append(allErrs, field.Forbidden(specPath.Child("taints"), "taints are not allowed to be set when the feature gate MachineTaintPropagation is disabled"))
 		}
 	}
 

internal/webhooks/test/machine_test.go

@@ -178,7 +178,7 @@ func TestMachineTaintValidation(t *testing.T) {
 			expectErr: true,
 		},
 		{
-			name:           "should block taint key with multuple slashes",
+			name:           "should block taint key with multiple slashes",
 			featureEnabled: true,
 			machine: m.DeepCopy().WithTaints(clusterv1.MachineTaint{
 				Key: "one/two/with-prefix", Effect: corev1.TaintEffectNoSchedule, Propagation: clusterv1.MachineTaintPropagationAlways,