🐛 fixes issue when we update the clustermanager with new labels. #1047
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: suvaanshkumar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
=======================================
Coverage 63.98% 63.98%
=======================================
Files 198 198
Lines 19631 19631
=======================================
Hits 12561 12561
Misses 6020 6020
Partials 1050 1050
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
suvaanshkumar
changed the title
Signed-off-by: suvaanshkumar <[email protected]>
|
PR needs rebase. 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-sigs/prow repository. |
WalkthroughThis change updates several cluster manager deployment manifests to use dynamic label values based on the cluster manager name and removes dynamic user-provided labels from deployment selectors to address immutability issues. Test utilities and integration tests were also updated to align with the new label logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClusterManager Template Engine
participant Kubernetes API Server
User->>ClusterManager Template Engine: Deploy ClusterManager with custom labels
ClusterManager Template Engine->>Kubernetes API Server: Create/Update Deployment with selector.matchLabels (only fixed labels, no user labels)
Kubernetes API Server-->>ClusterManager Template Engine: Deployment created/updated (no immutable selector error)
User->>Kubernetes API Server: Update Deployment labels (metadata.labels, not selector)
Kubernetes API Server-->>User: Labels updated successfully
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
manifests/cluster-manager/management/cluster-manager-addon-manager-deployment.yaml (1)
40-40: Update pod affinity rules to use dynamic values.The pod affinity rules still reference the hardcoded value "clustermanager-addon-manager-controller" instead of using the dynamic template. This creates inconsistency with the updated labels.
values: - - clustermanager-addon-manager-controller + - {{ .ClusterManagerName }}-addon-manager-controllerApply the same change to both pod affinity rules (lines 40 and 49).
Also applies to: 49-49
manifests/cluster-manager/management/cluster-manager-registration-deployment.yaml (1)
40-40: Update pod affinity rules to use dynamic values.The pod affinity rules still reference the hardcoded value "clustermanager-registration-controller" instead of using the dynamic template, creating inconsistency.
values: - - clustermanager-registration-controller + - {{ .ClusterManagerName }}-registration-controllerApply this change to both pod affinity rules.
Also applies to: 49-49
manifests/cluster-manager/management/cluster-manager-placement-deployment.yaml (1)
41-41: Update pod affinity rules to use dynamic values.The pod affinity rules still reference the hardcoded value "clustermanager-placement-controller" instead of using the dynamic template.
values: - - clustermanager-placement-controller + - {{ .ClusterManagerName }}-placement-controllerApply this change to both pod affinity rules.
Also applies to: 50-50
🧹 Nitpick comments (1)
test/integration/util/helpers.go (1)
3-4: Consider improving the function name and documentation.The function name
MapConsistsis grammatically awkward. Consider renaming it toMapContains,IsSubsetOf, orMapIsSubsetOffor better clarity.-// MapConsists checks if the initialMap is a subset of the entireMap. -func MapConsists(initialMap, entireMap map[string]string) bool { +// MapContains checks if the entireMap contains all key-value pairs from the initialMap. +func MapContains(entireMap, initialMap map[string]string) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
manifests/cluster-manager/management/cluster-manager-addon-manager-deployment.yaml(2 hunks)manifests/cluster-manager/management/cluster-manager-manifestworkreplicaset-deployment.yaml(0 hunks)manifests/cluster-manager/management/cluster-manager-placement-deployment.yaml(2 hunks)manifests/cluster-manager/management/cluster-manager-registration-deployment.yaml(2 hunks)manifests/cluster-manager/management/cluster-manager-registration-webhook-deployment.yaml(0 hunks)manifests/cluster-manager/management/cluster-manager-work-webhook-deployment.yaml(0 hunks)test/integration/operator/clustermanager_test.go(3 hunks)test/integration/util/helpers.go(1 hunks)
💤 Files with no reviewable changes (3)
- manifests/cluster-manager/management/cluster-manager-registration-webhook-deployment.yaml
- manifests/cluster-manager/management/cluster-manager-manifestworkreplicaset-deployment.yaml
- manifests/cluster-manager/management/cluster-manager-work-webhook-deployment.yaml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.778Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
manifests/cluster-manager/management/cluster-manager-registration-deployment.yaml (2)
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.778Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrolebinding.yaml:5-10
Timestamp: 2025-06-26T00:33:09.303Z
Learning: The OCM (Open Cluster Management) project uses Go templates for Kubernetes manifest generation, not Helm templates. Functions like `toYaml` that are available in Helm are not available in Go templates. The correct Go template syntax for iterating over labels is `{{ range $key, $value := .Labels }}` followed by `"{{ $key }}": "{{ $value }}"`.
manifests/cluster-manager/management/cluster-manager-addon-manager-deployment.yaml (2)
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.778Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrolebinding.yaml:5-10
Timestamp: 2025-06-26T00:33:09.303Z
Learning: The OCM (Open Cluster Management) project uses Go templates for Kubernetes manifest generation, not Helm templates. Functions like `toYaml` that are available in Helm are not available in Go templates. The correct Go template syntax for iterating over labels is `{{ range $key, $value := .Labels }}` followed by `"{{ $key }}": "{{ $value }}"`.
manifests/cluster-manager/management/cluster-manager-placement-deployment.yaml (2)
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.778Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrolebinding.yaml:5-10
Timestamp: 2025-06-26T00:33:09.303Z
Learning: The OCM (Open Cluster Management) project uses Go templates for Kubernetes manifest generation, not Helm templates. Functions like `toYaml` that are available in Helm are not available in Go templates. The correct Go template syntax for iterating over labels is `{{ range $key, $value := .Labels }}` followed by `"{{ $key }}": "{{ $value }}"`.
test/integration/operator/clustermanager_test.go (2)
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrole.yaml:5-10
Timestamp: 2025-06-26T00:34:09.778Z
Learning: The open-cluster-management-io/ocm codebase uses Go templates (text/template), not Helm templates. The standard pattern for dynamic labels in manifests is: `{{ if gt (len .Labels) 0 }}{{ range $key, $value := .Labels }}"{{ $key }}": "{{ $value }}"{{ end }}{{ end }}`. Do not suggest Helm-specific functions like `toYaml` for this codebase.
Learnt from: zhiweiyin318
PR: open-cluster-management-io/ocm#1046
File: manifests/cluster-manager/hub/cluster-manager-clusterprofiles-clusterrolebinding.yaml:5-10
Timestamp: 2025-06-26T00:33:09.303Z
Learning: The OCM (Open Cluster Management) project uses Go templates for Kubernetes manifest generation, not Helm templates. Functions like `toYaml` that are available in Helm are not available in Go templates. The correct Go template syntax for iterating over labels is `{{ range $key, $value := .Labels }}` followed by `"{{ $key }}": "{{ $value }}"`.
🧬 Code Graph Analysis (1)
test/integration/operator/clustermanager_test.go (2)
test/integration/util/helpers.go (1)
MapConsists(4-17)vendor/github.com/onsi/gomega/matchers.go (1)
BeNil(51-53)
🔇 Additional comments (5)
manifests/cluster-manager/management/cluster-manager-addon-manager-deployment.yaml (1)
7-7: LGTM! Dynamic label templating is correctly applied.The replacement of hardcoded label values with
{{ .ClusterManagerName }}-addon-manager-controllerproperly implements dynamic labeling based on the cluster manager name.Also applies to: 18-18, 22-22
manifests/cluster-manager/management/cluster-manager-registration-deployment.yaml (1)
7-7: LGTM! Dynamic label templating is correctly applied.The replacement of hardcoded label values with
{{ .ClusterManagerName }}-registration-controllerproperly implements dynamic labeling.Also applies to: 18-18, 22-22
manifests/cluster-manager/management/cluster-manager-placement-deployment.yaml (1)
7-7: LGTM! Dynamic label templating is correctly applied.The replacement of hardcoded label values with
{{ .ClusterManagerName }}-placement-controllerproperly implements dynamic labeling.Also applies to: 18-18, 23-23
test/integration/operator/clustermanager_test.go (2)
1186-1186: Consider removing the generic "app": "clustermanager" label.The removal of the generic "app": "clustermanager" label from the expected labels map aligns with the shift to more specific, dynamic labeling using cluster manager names.
1234-1234: LGTM! Improved variable naming for clarity.The updated variable names like
k8sHubWorkWebhookDeployment,k8sHubPlacementDeployment, etc., make the test more readable and less confusing than reusing generic names.Also applies to: 1244-1244, 1254-1254, 1264-1264
| // 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") |
There was a problem hiding this comment.
🛠️ 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 trueAlso 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 fmt.Errorf("expected registration-controller labels to be %v, but got %v", labels, registrationDeployment.GetLabels()) | ||
| } | ||
| registrationLabels := registrationDeployment.GetLabels() | ||
| util.MapConsists(registrationLabels, labels) |
There was a problem hiding this comment.
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.
Summary
This PR removes the3 additional tags in the label selectors of deployments that are created by clustermanager operator in open-cluster-management-hub namespace.
Related issue(s)
Fixes #1046
Summary by CodeRabbit
Refactor
Tests
Chores