Skip to content

🐛 fixes issue when we update the clustermanager with new labels. #1047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛 fixes issue when we update the clustermanager with new labels. #1047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions manifests/cluster-manager/management/cluster-manager-addon-manager-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: {{ .ClusterManagerName }}-addon-manager-controller
namespace: {{ .ClusterManagerNamespace }}
labels:
app: clustermanager-controller
app: {{ .ClusterManagerName }}-addon-manager-controller
createdByClusterManager: {{ .ClusterManagerName }}
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
Expand All @@ -15,16 +15,11 @@ spec:
replicas: {{ .Replica }}
selector:
matchLabels:
app: clustermanager-addon-manager-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
app: {{ .ClusterManagerName }}-addon-manager-controller
template:
metadata:
labels:
app: clustermanager-addon-manager-controller
app: {{ .ClusterManagerName }}-addon-manager-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
Expand Down
5 changes: 0 additions & 5 deletions manifests/cluster-manager/management/cluster-manager-manifestworkreplicaset-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ spec:
selector:
matchLabels:
app: {{ .ClusterManagerName }}-work-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
template:
metadata:
labels:
Expand Down
12 changes: 4 additions & 8 deletions manifests/cluster-manager/management/cluster-manager-placement-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: {{ .ClusterManagerName }}-placement-controller
namespace: {{ .ClusterManagerNamespace }}
labels:
app: clustermanager-controller
app: {{ .ClusterManagerName }}-placement-controller
createdByClusterManager: {{ .ClusterManagerName }}
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
Expand All @@ -15,16 +15,12 @@ spec:
replicas: {{ .Replica }}
selector:
matchLabels:
app: clustermanager-placement-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
app: {{ .ClusterManagerName }}-placement-controller

template:
metadata:
labels:
app: clustermanager-placement-controller
app: {{ .ClusterManagerName }}-placement-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
Expand Down
11 changes: 3 additions & 8 deletions manifests/cluster-manager/management/cluster-manager-registration-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: {{ .ClusterManagerName }}-registration-controller
namespace: {{ .ClusterManagerNamespace }}
labels:
app: clustermanager-controller
app: {{ .ClusterManagerName }}-registration-controller
createdByClusterManager: {{ .ClusterManagerName }}
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
Expand All @@ -15,16 +15,11 @@ spec:
replicas: {{ .Replica }}
selector:
matchLabels:
app: clustermanager-registration-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
app: {{ .ClusterManagerName }}-registration-controller
template:
metadata:
labels:
app: clustermanager-registration-controller
app: {{ .ClusterManagerName }}-registration-controller
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{$key}}": "{{$value}}"
Expand Down
5 changes: 0 additions & 5 deletions manifests/cluster-manager/management/cluster-manager-registration-webhook-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ spec:
selector:
matchLabels:
app: {{ .ClusterManagerName }}-registration-webhook
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
template:
metadata:
labels:
Expand Down
5 changes: 0 additions & 5 deletions manifests/cluster-manager/management/cluster-manager-work-webhook-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ spec:
selector:
matchLabels:
app: {{ .ClusterManagerName }}-work-webhook
{{ if gt (len .Labels) 0 }}
{{ range $key, $value := .Labels }}
"{{ $key }}": "{{ $value }}"
{{ end }}
{{ end }}
template:
metadata:
labels:
Expand Down
36 changes: 13 additions & 23 deletions test/integration/operator/clustermanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,7 @@ var _ = ginkgo.Describe("ClusterManager Default Mode", ginkgo.Ordered, func() {

ginkgo.It("should have labels on resources created by clustermanager", func() {

labels := map[string]string{"app": "clustermanager", "createdByClusterManager": "hub", "test-label": "test-value", "test-label2": "test-value2"}
labels := map[string]string{"createdByClusterManager": "hub", "test-label": "test-value", "test-label2": "test-value2"}
gomega.Eventually(func() error {
clusterManager, err := operatorClient.OperatorV1().ClusterManagers().Get(context.Background(), clusterManagerName, metav1.GetOptions{})
if err != nil {
Expand All @@ -1202,9 +1202,8 @@ var _ = ginkgo.Describe("ClusterManager Default Mode", ginkgo.Ordered, func() {
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected registration-controller labels to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
registrationLabels := registrationDeployment.GetLabels()
util.MapConsists(registrationLabels, labels)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix MapConsists usage - missing assertions.

The util.MapConsists calls are not being used properly for testing. The function returns a boolean, but you're not asserting on the result. This means the test will pass even if the label subset check fails.

-				util.MapConsists(registrationLabels, labels)
+				if !util.MapConsists(labels, registrationLabels) {
+					return fmt.Errorf("expected labels not found in deployment labels")
+				}

Apply similar fixes to all MapConsists calls in this test. Note that I've also swapped the parameter order based on the function's expected behavior (initialMap should be the expected labels, entireMap should be the actual deployment labels).

Also applies to: 1228-1228, 1238-1238, 1248-1248, 1258-1258, 1269-1269

🤖 Prompt for AI Agents 
In test/integration/operator/clustermanager_test.go at line 1206 and similarly
at lines 1228, 1238, 1248, 1258, and 1269, the calls to util.MapConsists are
missing assertions on their boolean results, causing tests to pass regardless of
the check outcome. Fix this by wrapping each util.MapConsists call in an
assertion function (e.g., require.True or assert.True) to verify the result is
true. Also, ensure the parameter order matches the function's expected behavior,
passing the expected labels as the first argument and the actual deployment
labels as the second.

return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())

Expand All @@ -1226,57 +1225,48 @@ var _ = ginkgo.Describe("ClusterManager Default Mode", ginkgo.Ordered, func() {
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected registration-webhook labels to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
util.MapConsists(registrationDeployment.GetLabels(), labels)
return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())

// Compare labels on work-webhook
gomega.Eventually(func() error {
registrationDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubWorkWebhookDeployment, metav1.GetOptions{})
k8sHubWorkWebhookDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubWorkWebhookDeployment, metav1.GetOptions{})
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected work-webhook labels to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
util.MapConsists(k8sHubWorkWebhookDeployment.GetLabels(), labels)
return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())

// Compare labels on placement-controller
gomega.Eventually(func() error {
registrationDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubPlacementDeployment, metav1.GetOptions{})
k8sHubPlacementDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubPlacementDeployment, metav1.GetOptions{})
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected placement-controller labels to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
util.MapConsists(k8sHubPlacementDeployment.GetLabels(), labels)
return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())

// Compare labels on addon-manager-controller
gomega.Eventually(func() error {
registrationDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubAddOnManagerDeployment, metav1.GetOptions{})
k8sHubAddOnManagerDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubAddOnManagerDeployment, metav1.GetOptions{})
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected labels addon-manager-controller to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
util.MapConsists(k8sHubAddOnManagerDeployment.GetLabels(), labels)
return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())

// Compare labels on work-controller
gomega.Eventually(func() error {
registrationDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).Get(context.Background(), hubWorkControllerDeployment, metav1.GetOptions{})
k8sHubWorkControllerDeployment, err := kubeClient.AppsV1().Deployments(hubNamespace).
Get(context.Background(), hubWorkControllerDeployment, metav1.GetOptions{})
if err != nil {
return err
}
if !helpers.MapCompare(registrationDeployment.GetLabels(), labels) {
return fmt.Errorf("expected work-controller labels to be %v, but got %v", labels, registrationDeployment.GetLabels())
}
util.MapConsists(k8sHubWorkControllerDeployment.GetLabels(), labels)
return nil
}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeNil())
})
Expand Down
17 changes: 17 additions & 0 deletions test/integration/util/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package util

// MapConsists checks if the initialMap is a subset of the entireMap.
func MapConsists(initialMap, entireMap map[string]string) bool {
if len(initialMap) == 0 {
println("Initial map is empty, returning true")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove debug print statements from library code.

The println statements are inappropriate for library/utility code as they produce unwanted output during testing and production use. Consider using a proper logging framework or removing them entirely.

 	if len(initialMap) == 0 {
-		println("Initial map is empty, returning true")
 		return true
 	}
 	for key, value := range initialMap {
 		if entireMap[key] != value {
-			println("Map does not consist, returning false")
 			return false
 		}
 	}
-	println("Map consists of the required values, returning true")
 	return true

Also applies to: 11-11, 15-15

🤖 Prompt for AI Agents 
In test/integration/util/helpers.go at lines 6, 11, and 15, remove the println
debug statements as they produce unwanted output in tests and production.
Replace them with proper logging calls if necessary, or remove them entirely to
keep the utility code clean and silent during normal operation.

return true
}
for key, value := range initialMap {
if entireMap[key] != value {
println("Map does not consist, returning false")
return false
}
}
println("Map consists of the required values, returning true")
return true
}
Loading