Set default clusterID labels only on machine by enxebre · Pull Request #659 · openshift/machine-api-operator

enxebre · 2020-07-27T12:26:32Z

This tries to fix the following scenario:
We set ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] if it's not present. It's not present and we set it to the correct value. If there happens to be a bad label in ms.Spec.Template.Labels this would result in a miss match.

Follow up for
#608,
#644 and
#653.

This tries to fix the following scenario: We set ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] if it's not present. It's not present and we set it to the correct value. If there happens to be a bad label in `ms.Spec.Template.Labels` this would result in a miss match. Follow for openshift#608, openshift#644 and openshift#653.

michaelgugino · 2020-07-27T14:57:41Z

@@ -124,24 +124,6 @@ func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, ut
 	if ok, err := h.webhookOperations(m, h.clusterID); !ok {
 		errs = append(errs, err.Errors()...)
 	} else {


Do we need this else statement anymore?

yes, this is the original logic to set the defaulted spec regardless the labels.

We're only appending errors in one place. It seems we should just exit early there and get rid of the else statement.

michaelgugino · 2020-07-27T14:58:12Z

-		}
-
 		// Restore the defaulted template
 		ms.Spec.Template.Spec = m.Spec


Is this line doing anything useful now?

yes, this is the original logic to set the defaulted spec regardless the labels.

Okay, the comment for this code doesn't really make sense. It seems like it should say
"Update the template to the defaulted one" or similar. "Restore" implies we're undoing something.

enxebre · 2020-07-27T15:03:06Z

/retest

enxebre · 2020-07-28T08:19:32Z

/retest

enxebre · 2020-07-29T08:07:53Z

/retest

Danil-Grigorev · 2020-07-29T08:36:53Z

-				if tc.presetClusterID {
-					gs.Expect(ms.Spec.Selector.MatchLabels[MachineClusterIDLabel]).To(BeIdenticalTo(presetClusterID))
-					gs.Expect(ms.Spec.Template.Labels[MachineClusterIDLabel]).To(BeIdenticalTo(presetClusterID))
-
-				} else {
-					gs.Expect(ms.Spec.Selector.MatchLabels[MachineClusterIDLabel]).To(BeIdenticalTo(tc.clusterID))
-					gs.Expect(ms.Spec.Template.Labels[MachineClusterIDLabel]).To(BeIdenticalTo(tc.clusterID))
-				}


I think it needs to test the opposite is true - the ClusterID label didn't change.

Danil-Grigorev

/approve

Just a minor note

openshift-ci-robot · 2020-07-29T08:37:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Danil-Grigorev]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enxebre · 2020-07-30T08:16:15Z

/retest

michaelgugino · 2020-07-31T15:09:19Z

@@ -124,24 +124,6 @@ func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, ut
 	if ok, err := h.webhookOperations(m, h.clusterID); !ok {
 		errs = append(errs, err.Errors()...)
 	} else {


We're only appending errors in one place. It seems we should just exit early there and get rid of the else statement.

michaelgugino · 2020-07-31T15:10:39Z

-		}
-
 		// Restore the defaulted template
 		ms.Spec.Template.Spec = m.Spec


Okay, the comment for this code doesn't really make sense. It seems like it should say
"Update the template to the defaulted one" or similar. "Restore" implies we're undoing something.

JoelSpeed

/lgtm

enxebre · 2020-08-03T09:10:49Z

/test e2e-aws-operator

openshift-bot · 2020-08-03T10:24:39Z

/retest