🌱 Set explicit default namespace for addon registration #338
Conversation
- Set default Status.Namespace to "open-cluster-management-agent-addon" - Clarify namespace priority order with explicit default value - Add test case for default namespace behavior - Improve test error messages to show actual vs expected values This ensures a consistent default namespace even when InstallNamespace is deprecated and agentInstallNamespace is empty. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: zhujian <[email protected]>
WalkthroughThe change reworks namespace resolution logic in addon registration synchronization, establishing a priority-based namespace determination flow with defaults, explicit overrides, and conditional AgentInstallNamespace handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Reasoning: The controller change introduces a priority-based namespace resolution flow requiring careful verification of the override sequence and edge case handling. The test additions validate default behavior but are straightforward. Mixed complexity with single affected file demands deliberate review of logic correctness rather than high file heterogeneity. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zhujian7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/addonmanager/controllers/registration/controller.go (3)
188-191: Use a named constant and fix typos in the comment.Good default. Recommend a package-level const to avoid string drift, and correct “deprceated”/grammar.
Apply within this block:
- // explicitly set the default namespace value, since the mca.spec.installNamespace is deprceated and - // the addonDeploymentConfig.spec.agentInstallNamespace could be empty - managedClusterAddonCopy.Status.Namespace = "open-cluster-management-agent-addon" + // Set explicit default namespace. Spec.installNamespace is deprecated and + // addonDeploymentConfig.spec.agentInstallNamespace may be empty. + managedClusterAddonCopy.Status.Namespace = defaultAddonAgentNamespaceAdd near the top of this file (after imports):
+const defaultAddonAgentNamespace = "open-cluster-management-agent-addon"
192-199: Clarify comment to reflect override semantics.Comment implies defaulting to registrationOption.Namespace. It’s actually an override when set. Update for accuracy.
- // Set the default namespace to registrationOption.Namespace + // Override with registrationOption.Namespace if provided
201-209: Tighten comment; consider status condition on error.
- Comment mentions InstallNamespace here, but this block only handles AgentInstallNamespace; adjust wording.
- Optional: when AgentInstallNamespace returns an error, mirror the earlier PermissionConfig failure path by setting RegistrationApplied=False with a specific reason before returning, improving observability.
- // Override if agentInstallNamespace or InstallNamespace is specified + // Override with AgentInstallNamespace if it returns a non-empty valueOptionally set a condition on error (pattern already used above):
// On err, set a ManagedClusterAddOnRegistrationApplied=False condition // with a dedicated Reason (e.g., RegistrationAppliedResolveNamespaceFailed) // and PatchStatus before returning.pkg/addonmanager/controllers/registration/controller_test.go (1)
220-252: LGTM: covers defaulting path; add one more edge test.Test correctly verifies default namespace when nothing is specified. Suggest adding a case where AgentInstallNamespace returns an empty string to confirm it does not clobber the default.
Example case to add to cases[]:
{ name: "agentInstallNamespace returns empty -> keep default", cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")}, addon: []runtime.Object{ func() *addonapiv1alpha1.ManagedClusterAddOn { addon := addontesting.NewAddon("test", "cluster1", metav1.OwnerReference{Kind: "ClusterManagementAddOn", Name: "test"}) return addon }(), }, validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { addontesting.AssertActions(t, actions, "patch") actual := actions[0].(clienttesting.PatchActionImpl).Patch addOn := &addonapiv1alpha1.ManagedClusterAddOn{} if err := json.Unmarshal(actual, addOn); err != nil { t.Fatal(err) } if addOn.Status.Namespace != "open-cluster-management-agent-addon" { t.Errorf("Namespace %s in status is not correct, expected open-cluster-management-agent-addon", addOn.Status.Namespace) } }, testaddon: &testAgent{ name: "test", registrations: []addonapiv1alpha1.RegistrationConfig{{SignerName: "test"}}, agentInstallNamespace: func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { return "", nil }, }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/addonmanager/controllers/registration/controller.go(1 hunks)pkg/addonmanager/controllers/registration/controller_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/addonmanager/controllers/registration/controller_test.go (2)
pkg/addonmanager/addontesting/helpers.go (3)
NewManagedCluster(208-214)NewAddon(81-89)AssertActions(292-301)vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.go (2)
ManagedClusterAddOn(19-32)RegistrationConfig(56-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-hosted
- GitHub Check: e2e-cloudevents
- GitHub Check: integration
- GitHub Check: e2e
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: verify
🔇 Additional comments (2)
pkg/addonmanager/controllers/registration/controller_test.go (2)
172-177: LGTM: clearer assertion message.Including actual Namespace improves failure diagnostics.
206-211: LGTM: clearer assertion message.Better error context with the actual value.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit c18f49f
into
open-cluster-management-io:main
2 participants
Description
This PR sets an explicit default namespace for addon registration to ensure consistent behavior when
InstallNamespaceis deprecated andagentInstallNamespaceis empty.Changes
Status.Namespaceto"open-cluster-management-agent-addon"Namespace Priority Order
"open-cluster-management-agent-addon"registrationOption.Namespaceif setSpec.InstallNamespaceif setAgentInstallNamespacecallback result if set and non-emptyTesting
Added new test case
"default namespace when no namespace is specified"to verify the default namespace is correctly set.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
open-cluster-management-agent-addon, then applies overrides from registration options, installation specifications, and agent namespace settings with proper fallback handling.Tests