🐛 Allow empty string for AgentInstallNamespace to support template namespaces #400
Conversation
…spaces This change updates the AgentInstallNamespace field validation pattern to allow empty strings, enabling template-type addons to use the namespace defined in their addonTemplate. Key changes: - Updated validation pattern from ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ to ^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$ to allow empty strings - Added documentation clarifying behavior for template vs non-template addons - Regenerated CRD manifests - Added comprehensive integration tests for validation This allows users to explicitly set agentInstallNamespace to "" to use their template's namespace, while maintaining backward compatibility with the default "open-cluster-management-agent-addon" namespace. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: zhujian <[email protected]>
WalkthroughMakes agentInstallNamespace optional by updating validation to allow empty string, removes implicit defaulting behavior in API comments, expands CRD field description, and adds integration tests covering valid, invalid, and empty namespace cases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
|
/cc @qiujian16 |
![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 (2)
test/integration/api/addondeploymentconfig_test.go (2)
163-256: Excellent test coverage for the new validation behavior.The integration tests comprehensively validate the updated
agentInstallNamespacefield:
- Empty string acceptance (lines 163-180): validates template namespace support
- Valid namespace (lines 182-199): confirms standard namespace format works
- Invalid formats (lines 201-237): correctly reject leading hyphens and uppercase
- Length constraint (lines 239-256): validates 63-char maximum
Optional: Make the max-length test more precise.
At line 246,
rand.String(64)generates a random 64-character string that may contain non-lowercase characters, potentially causing the test to fail for pattern mismatch rather than length. Consider generating a valid but too-long namespace string for clearer test intent:- AgentInstallNamespace: rand.String(64), // max is 63 + AgentInstallNamespace: "a" + rand.String(62) + "b", // 64 chars total, max is 63This ensures the test fails specifically due to length violation. However, since the test already validates that invalid configs are rejected (regardless of the specific reason), the current implementation is acceptable.
163-180: Consider adding an explicit test for default behavior.While existing tests at lines 31-89 implicitly verify the default namespace behavior (by omitting
agentInstallNamespace), consider adding a dedicated test that explicitly verifies the default value is applied when the field is omitted. This would make the test coverage more obvious and document the default behavior more clearly.Example test:
ginkgo.It("Should apply default agentInstallNamespace when field is omitted", func() { addOnDeploymentConfig := &addonv1alpha1.AddOnDeploymentConfig{ ObjectMeta: metav1.ObjectMeta{ Name: addOnDeploymentConfigName, Namespace: testNamespace, }, Spec: addonv1alpha1.AddOnDeploymentConfigSpec{ // AgentInstallNamespace intentionally omitted }, } created, err := hubAddonClient.AddonV1alpha1().AddOnDeploymentConfigs(testNamespace).Create( context.TODO(), addOnDeploymentConfig, metav1.CreateOptions{}, ) gomega.Expect(err).ToNot(gomega.HaveOccurred()) gomega.Expect(created.Spec.AgentInstallNamespace).To(gomega.Equal("open-cluster-management-agent-addon")) })This test would explicitly document and verify that the default namespace is applied when the field is omitted, distinguishing it from the empty string case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml(1 hunks)addon/v1alpha1/types_addondeploymentconfig.go(1 hunks)test/integration/api/addondeploymentconfig_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/integration/api/addondeploymentconfig_test.go (1)
addon/v1alpha1/types_addondeploymentconfig.go (2)
AddOnDeploymentConfig(14-21)AddOnDeploymentConfigSpec(23-72)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (2)
addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yaml (1)
44-50: LGTM! Validation pattern and documentation correctly support empty string for template namespaces.The pattern change from
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$to^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$correctly makes the field optional by wrapping the entire expression in an outer optional group. This allows:
- Empty string: for template-type addons to use
addonTemplatenamespace- Valid namespace strings: lowercase alphanumeric, hyphens (not leading/trailing), up to 63 chars
- Default behavior: preserved for backward compatibility when field is omitted
The expanded description clearly documents the intended behavior for both template-type and non-template addons.
addon/v1alpha1/types_addondeploymentconfig.go (1)
56-63: LGTM! Type definition changes are consistent with CRD updates.The validation pattern and documentation comments correctly reflect the API change to support empty string for template-type addons. The
+kubebuilder:defaultmarker ensures backward compatibility by applying the default namespace when the field is omitted, while allowing explicit empty string values to pass through for template namespace resolution.
|
/approve |
|
[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 f042545
into
open-cluster-management-io:main
Updates open-cluster-management.io/api to include changes from open-cluster-management-io/api#400 which allows empty string for AgentInstallNamespace to support template-type addons. Signed-off-by: zhujian <[email protected]>
2 participants
Summary
This PR implements the API repository changes for ocm#1209, allowing template-type addons to use the namespace defined in their
addonTemplateby settingagentInstallNamespaceto an empty string.Changes
1. API Type Changes
AgentInstallNamespacevalidation pattern intypes_addondeploymentconfig.gofrom^[a-z0-9]([-a-z0-9]*[a-z0-9])?$to^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$to allow empty strings2. CRD Updates
3. Integration Tests
AgentInstallNamespacevalidation:Behavior
For template-type addons:
agentInstallNamespace: ""to use the namespace defined in theaddonTemplateFor non-template addons:
"open-cluster-management-agent-addon"if not specifiedBackward Compatibility
✅ Fully backward compatible - existing configs continue to work with the default namespace
Related issue(s)
open-cluster-management-io/ocm#1209
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests