✨ Moved MachineDeployment Cluster Label Name to webhook

[hpandeycodeit]

What this PR does / why we need it:
Moved MachineDeployment Cluster Label Name to webhook as per #2302

Which issue(s) this PR fixes :
Fixes #2302

[prankul88]

@hpandeycodeit You probably need to remove clusterv1.ClusterLabelName and add ClusterLabelName instead.

[hpandeycodeit]

thanks! changed it :)

[vincepri]

/approve

Linter is failing with

controllers/machinedeployment_controller.go:151: File is not `gofmt`-ed with `-s` (gofmt)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hpandeycodeit, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

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

[detiber]

Specifying the Selector here is causing the test failure: line 49 is checking for the MachineDeploymentLabelName key which is only set if there is not a Selector already defined.

It might be good to add a separate test that specifies md.Spec.Selector.MatchLabels and md.Spec.Template.Labels and verifies that the MachineDeployment labels are not added, while the ClusterLabelName ones are.

[vincepri]

/milestone v0.3.0-rc.1

[vincepri]

@hpandeycodeit Are still interested to get this change merged?

[hpandeycodeit]

@hpandeycodeit Are still interested to get this change merged?

yes. will make the suggested changes today.

[vincepri]

@hpandeycodeit LGTM pending squash

[vincepri]

/milestone v0.3.0

[hpandeycodeit]

/test pull-cluster-api-test

[chuckha]

There were some assumptions in the tests that this change breaks. The tests have two choices. Either implement webhooks in the envtest set of tests or ensure that when we create machine deployments they always have the right label/selectors applied. Here is a diff required to get these tests to passing:

diff --git a/controllers/machinedeployment_controller_test.go b/controllers/machinedeployment_controller_test.go
index b947d3b7e..ccf1fc86a 100644
--- a/controllers/machinedeployment_controller_test.go
+++ b/controllers/machinedeployment_controller_test.go
@@ -62,19 +62,29 @@ var _ = Describe("MachineDeployment Reconciler", func() {
        })
 
        It("Should reconcile a MachineDeployment", func() {
-               labels := map[string]string{"foo": "bar"}
+               labels := map[string]string{
+                       "foo":                      "bar",
+                       clusterv1.ClusterLabelName: testCluster.Name,
+               }
                version := "1.10.3"
                deployment := &clusterv1.MachineDeployment{
                        ObjectMeta: metav1.ObjectMeta{
                                GenerateName: "md-",
                                Namespace:    namespace.Name,
+                               Labels: map[string]string{
+                                       clusterv1.ClusterLabelName: testCluster.Name,
+                               },
                        },
                        Spec: clusterv1.MachineDeploymentSpec{
                                ClusterName:          testCluster.Name,
                                MinReadySeconds:      pointer.Int32Ptr(0),
                                Replicas:             pointer.Int32Ptr(2),
                                RevisionHistoryLimit: pointer.Int32Ptr(0),
-                               Selector:             metav1.LabelSelector{},
+                               Selector: metav1.LabelSelector{
+                                       MatchLabels: map[string]string{
+                                               clusterv1.ClusterLabelName: testCluster.Name,
+                                       },
+                               },
                                Strategy: &clusterv1.MachineDeploymentStrategy{
                                        Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
                                        RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{
@@ -306,7 +316,8 @@ var _ = Describe("MachineDeployment Reconciler", func() {
                oldLabels[clusterv1.MachineDeploymentLabelName] = deployment.Name
 
                newLabels := map[string]string{
-                       "new-key": "new-value",
+                       "new-key":                  "new-value",
+                       clusterv1.ClusterLabelName: testCluster.Name,
                }
 
                By("Updating MachineDeployment label")

[chuckha]

if you apply that patch or any similar patch that gets the tests passing @hpandeycodeit, this lgtm.

[hpandeycodeit]

if you apply that patch or any similar patch that gets the tests passing @hpandeycodeit, this lgtm.

Thanks @chuckha I have applied this patch!

[chuckha]

/lgtm

[chuckha]

/hold

[chuckha]

Sorry, just noticed this wasn't squashed, can you squash this down to 1 commit please?

[vincepri]

/lgtm

@chuckha to unhold

[chuckha]

/hold cancel

[hpandeycodeit]

Thank you @vincepri @detiber @chuckha!

[k8s-ci-robot]

@hpandeycodeit: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-capd-e2e	dab38c8	link	/test pull-cluster-api-capd-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.