fix review findings

Author: sbueringer

test/e2e/clusterclass_changes.go

@@ -244,12 +244,11 @@ func modifyControlPlaneViaClusterClassAndWait(ctx context.Context, input modifyC
 		// Get the ControlPlane.
 		controlPlaneRef := input.Cluster.Spec.ControlPlaneRef
 		controlPlaneTopology := input.Cluster.Spec.Topology.ControlPlane
-		controlPlaneClass := input.ClusterClass.Spec.ControlPlane
 		controlPlane, err := external.Get(ctx, mgmtClient, controlPlaneRef, input.ClusterClass.Namespace)
 		g.Expect(err).ToNot(HaveOccurred())
 
 		// Verify that the fields from Cluster topology are set on the control plane.
-		assertControlPlaneFields(g, controlPlane, newControlPlaneTemplate, controlPlaneTopology, controlPlaneClass)
+		assertControlPlaneTopologyFields(g, controlPlane, controlPlaneTopology)
 
 		// Verify that ModifyControlPlaneFields have been set.
 		for fieldPath, expectedValue := range input.ModifyControlPlaneFields {
@@ -262,10 +261,10 @@ func modifyControlPlaneViaClusterClassAndWait(ctx context.Context, input modifyC
 	}, input.WaitForControlPlane...).Should(BeNil())
 }
 
-func assertControlPlaneFields(g Gomega, controlPlane, controlPlaneTemplate *unstructured.Unstructured, controlPlaneTopology clusterv1.ControlPlaneTopology, controlPlaneClass clusterv1.ControlPlaneClass) {
-	// Note: We only verify that all labels and annotations from the Cluster topology exist to keep it simple here.
-	// There could be additional labels and annotations from the ClusterClass or the ControlPlane template.
-	// This is fully covered by the ClusterClass rollout test.
+// assertControlPlaneTopologyFields asserts that all fields set in the ControlPlaneTopology have been set on the ControlPlane.
+// Note: We intentionally focus on the fields set in the ControlPlaneTopology and ignore the ones set through ClusterClass or
+// ControlPlane template as we want to validate that the fields of the ControlPlaneTopology have been propagated correctly.
+func assertControlPlaneTopologyFields(g Gomega, controlPlane *unstructured.Unstructured, controlPlaneTopology clusterv1.ControlPlaneTopology) {
 	metadata, err := contract.ControlPlane().MachineTemplate().Metadata().Get(controlPlane)
 	g.Expect(err).ToNot(HaveOccurred())
 	for k, v := range controlPlaneTopology.Metadata.Labels {
@@ -275,41 +274,23 @@ func assertControlPlaneFields(g Gomega, controlPlane, controlPlaneTemplate *unst
 		g.Expect(metadata.Annotations).To(HaveKeyWithValue(k, v))
 	}
 
-	nodeDrainTimeout, err := contract.ControlPlane().MachineTemplate().NodeDrainTimeout().Get(controlPlane)
-	g.Expect(err).ToNot(HaveOccurred())
-	expectedNodeDrainTimeout := controlPlaneTopology.NodeDrainTimeout
-	if expectedNodeDrainTimeout == nil {
-		expectedNodeDrainTimeout = controlPlaneClass.NodeDrainTimeout
-	}
-	if expectedNodeDrainTimeout == nil {
-		expectedNodeDrainTimeout, err = contract.ControlPlaneTemplate().Template().MachineTemplate().NodeDrainTimeout().Get(controlPlaneTemplate)
+	if controlPlaneTopology.NodeDrainTimeout != nil {
+		nodeDrainTimeout, err := contract.ControlPlane().MachineTemplate().NodeDrainTimeout().Get(controlPlane)
 		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(nodeDrainTimeout).To(Equal(controlPlaneTopology.NodeDrainTimeout))
 	}
-	g.Expect(nodeDrainTimeout).To(Equal(expectedNodeDrainTimeout))
 
-	nodeDeletionTimeout, err := contract.ControlPlane().MachineTemplate().NodeDeletionTimeout().Get(controlPlane)
-	g.Expect(err).ToNot(HaveOccurred())
-	expectedNodeDeletionTimeout := controlPlaneTopology.NodeDeletionTimeout
-	if expectedNodeDeletionTimeout == nil {
-		expectedNodeDeletionTimeout = controlPlaneClass.NodeDeletionTimeout
-	}
-	if expectedNodeDeletionTimeout == nil {
-		expectedNodeDeletionTimeout, err = contract.ControlPlaneTemplate().Template().MachineTemplate().NodeDeletionTimeout().Get(controlPlaneTemplate)
+	if controlPlaneTopology.NodeDeletionTimeout != nil {
+		nodeDeletionTimeout, err := contract.ControlPlane().MachineTemplate().NodeDeletionTimeout().Get(controlPlane)
 		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(nodeDeletionTimeout).To(Equal(controlPlaneTopology.NodeDeletionTimeout))
 	}
-	g.Expect(nodeDeletionTimeout).To(Equal(expectedNodeDeletionTimeout))
 
-	nodeVolumeDetachTimeout, err := contract.ControlPlane().MachineTemplate().NodeVolumeDetachTimeout().Get(controlPlane)
-	g.Expect(err).ToNot(HaveOccurred())
-	expectedNodeVolumeDetachTimeout := controlPlaneTopology.NodeVolumeDetachTimeout
-	if expectedNodeVolumeDetachTimeout == nil {
-		expectedNodeVolumeDetachTimeout = controlPlaneClass.NodeVolumeDetachTimeout
-	}
-	if expectedNodeVolumeDetachTimeout == nil {
-		expectedNodeVolumeDetachTimeout, err = contract.ControlPlaneTemplate().Template().MachineTemplate().NodeVolumeDetachTimeout().Get(controlPlaneTemplate)
+	if controlPlaneTopology.NodeVolumeDetachTimeout != nil {
+		nodeVolumeDetachTimeout, err := contract.ControlPlane().MachineTemplate().NodeVolumeDetachTimeout().Get(controlPlane)
 		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(nodeVolumeDetachTimeout).To(Equal(controlPlaneTopology.NodeVolumeDetachTimeout))
 	}
-	g.Expect(nodeVolumeDetachTimeout).To(Equal(expectedNodeVolumeDetachTimeout))
 }
 
 // modifyMachineDeploymentViaClusterClassAndWaitInput is the input type for modifyMachineDeploymentViaClusterClassAndWait.
@@ -400,7 +381,7 @@ func modifyMachineDeploymentViaClusterClassAndWait(ctx context.Context, input mo
 				md := mdList.Items[0]
 
 				// Verify that the fields from Cluster topology are set on the MachineDeployment.
-				assertMachineDeploymentFields(g, md, mdTopology, mdClass)
+				assertMachineDeploymentTopologyFields(g, md, mdTopology)
 
 				if mdClass.Template.Bootstrap.Ref != nil {
 					// Get the corresponding BootstrapConfigTemplate.
@@ -435,7 +416,10 @@ func modifyMachineDeploymentViaClusterClassAndWait(ctx context.Context, input mo
 	}
 }
 
-func assertMachineDeploymentFields(g Gomega, md clusterv1.MachineDeployment, mdTopology clusterv1.MachineDeploymentTopology, mdClass clusterv1.MachineDeploymentClass) {
+// assertMachineDeploymentTopologyFields asserts that all fields set in the MachineDeploymentTopology have been set on the MachineDeployment.
+// Note: We intentionally focus on the fields set in the MachineDeploymentTopology and ignore the ones set through ClusterClass
+// as we want to validate that the fields of the MachineDeploymentTopology have been propagated correctly.
+func assertMachineDeploymentTopologyFields(g Gomega, md clusterv1.MachineDeployment, mdTopology clusterv1.MachineDeploymentTopology) {
 	// Note: We only verify that all labels and annotations from the Cluster topology exist to keep it simple here.
 	// This is fully covered by the ClusterClass rollout test.
 	for k, v := range mdTopology.Metadata.Labels {
@@ -445,41 +429,29 @@ func assertMachineDeploymentFields(g Gomega, md clusterv1.MachineDeployment, mdT
 		g.Expect(md.Annotations).To(HaveKeyWithValue(k, v))
 	}
 
-	expectedNodeDrainTimeout := mdTopology.NodeDrainTimeout
-	if expectedNodeDrainTimeout == nil {
-		expectedNodeDrainTimeout = mdClass.NodeDrainTimeout
+	if mdTopology.NodeDrainTimeout != nil {
+		g.Expect(md.Spec.Template.Spec.NodeDrainTimeout).To(Equal(mdTopology.NodeDrainTimeout))
 	}
-	g.Expect(md.Spec.Template.Spec.NodeDrainTimeout).To(Equal(expectedNodeDrainTimeout))
 
-	expectedNodeDeletionTimeout := mdTopology.NodeDeletionTimeout
-	if expectedNodeDeletionTimeout == nil {
-		expectedNodeDeletionTimeout = mdClass.NodeDeletionTimeout
+	if mdTopology.NodeDeletionTimeout != nil {
+		g.Expect(md.Spec.Template.Spec.NodeDeletionTimeout).To(Equal(mdTopology.NodeDeletionTimeout))
 	}
-	g.Expect(md.Spec.Template.Spec.NodeDeletionTimeout).To(Equal(expectedNodeDeletionTimeout))
 
-	expectedNodeVolumeDetachTimeout := mdTopology.NodeVolumeDetachTimeout
-	if expectedNodeVolumeDetachTimeout == nil {
-		expectedNodeVolumeDetachTimeout = mdClass.NodeVolumeDetachTimeout
+	if mdTopology.NodeVolumeDetachTimeout != nil {
+		g.Expect(md.Spec.Template.Spec.NodeVolumeDetachTimeout).To(Equal(mdTopology.NodeVolumeDetachTimeout))
 	}
-	g.Expect(md.Spec.Template.Spec.NodeVolumeDetachTimeout).To(Equal(expectedNodeVolumeDetachTimeout))
 
-	expectedMinReadySeconds := mdTopology.MinReadySeconds
-	if expectedMinReadySeconds == nil {
-		expectedMinReadySeconds = mdClass.MinReadySeconds
+	if mdTopology.MinReadySeconds != nil {
+		g.Expect(md.Spec.MinReadySeconds).To(Equal(mdTopology.MinReadySeconds))
 	}
-	g.Expect(md.Spec.MinReadySeconds).To(Equal(expectedMinReadySeconds))
 
-	expectedStrategy := mdTopology.Strategy
-	if expectedStrategy == nil {
-		expectedStrategy = mdClass.Strategy
+	if mdTopology.Strategy != nil {
+		g.Expect(md.Spec.Strategy).To(Equal(mdTopology.Strategy))
 	}
-	g.Expect(md.Spec.Strategy).To(Equal(expectedStrategy))
 
-	expectedFailureDomain := mdTopology.FailureDomain
-	if expectedFailureDomain == nil {
-		expectedFailureDomain = mdClass.FailureDomain
+	if mdTopology.FailureDomain != nil {
+		g.Expect(md.Spec.Template.Spec.FailureDomain).To(Equal(mdTopology.FailureDomain))
 	}
-	g.Expect(md.Spec.Template.Spec.FailureDomain).To(Equal(expectedFailureDomain))
 }
 
 // rebaseClusterClassAndWaitInput is the input type for rebaseClusterClassAndWait.

test/e2e/clusterclass_rollout.go

@@ -1016,9 +1016,7 @@ func modifyControlPlaneViaClusterAndWait(ctx context.Context, input modifyContro
 		g.Expect(err).ToNot(HaveOccurred())
 
 		// Verify that the fields from Cluster topology are set on the control plane.
-		// Note: We don't have to pass in the ControlPlaneTemplate and the ControlPlaneClass as the
-		// timeouts are all set in the Cluster topology, so these objects are not used.
-		assertControlPlaneFields(g, controlPlane, nil, controlPlaneTopology, clusterv1.ControlPlaneClass{})
+		assertControlPlaneTopologyFields(g, controlPlane, controlPlaneTopology)
 	}, input.WaitForControlPlane...).Should(Succeed())
 }
 
@@ -1062,9 +1060,7 @@ func modifyMachineDeploymentViaClusterAndWait(ctx context.Context, input modifyM
 				md := mdList.Items[0]
 
 				// Verify that the fields from Cluster topology are set on the MachineDeployment.
-				// Note: We don't have to pass in the MachineDeploymentClass as the fields are
-				// all set in the Cluster topology, so the MachineDeploymentClass is not used.
-				assertMachineDeploymentFields(g, md, mdTopology, clusterv1.MachineDeploymentClass{})
+				assertMachineDeploymentTopologyFields(g, md, mdTopology)
 			}, input.WaitForMachineDeployments...).Should(BeNil())
 		}
 	}