🐛 Support empty AgentInstallNamespace for template-type addons #337
Conversation
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]>
…e addons This commit adds a new function AgentInstallNamespaceFromDeploymentConfigRawFunc that returns the raw AgentInstallNamespace value from AddonDeploymentConfig, including empty string. This enables template-type addons to signal "use template namespace" by setting agentInstallNamespace: "". For backward compatibility, the existing AgentInstallNamespaceFromDeploymentConfigFunc now explicitly returns "open-cluster-management-agent-addon" when the config exists but AgentInstallNamespace is empty. This ensures existing addons continue to work as before. Changes: - Add AgentInstallNamespaceFromDeploymentConfigRawFunc for raw value access - Update AgentInstallNamespaceFromDeploymentConfigFunc to return default namespace when AgentInstallNamespace is empty (backward compatible) - Add comprehensive tests for both functions - Update documentation to clarify the behavior differences Fixes: open-cluster-management-io/ocm#1209 Signed-off-by: zhujian <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
WalkthroughThe PR updates the addon-framework dependency version and introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes are well-focused with straightforward logic. A single new function variant is introduced alongside the existing function modification, with comprehensive test coverage. The nil checks and conditional returns are clear and maintainable. No complex control flow or architectural changes. Possibly related issues
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (1)
pkg/utils/addon_config.go (1)
75-104: Consider extracting common logic to reduce duplication.The new function duplicates most of the logic from
AgentInstallNamespaceFromDeploymentConfigFunc(lines 84-98 mirror lines 44-62). Consider extracting the common retrieval logic into a private helper function and have both public functions call it with a flag indicating whether to apply the default.Example refactor:
func getAgentInstallNamespaceFromDeploymentConfig( addon *addonapiv1alpha1.ManagedClusterAddOn, adcgetter AddOnDeploymentConfigGetter, applyDefault bool, ) (string, error) { if addon == nil { return "", fmt.Errorf("failed to get addon install namespace, addon is nil") } config, err := GetDesiredAddOnDeploymentConfig(addon, adcgetter) if err != nil { return "", fmt.Errorf("failed to get deployment config for addon %s: %v", addon.Name, err) } if config == nil { klog.V(4).InfoS("Addon deployment config is nil, return an empty string for agent install namespace", "addonNamespace", addon.Namespace, "addonName", addon.Name) return "", nil } namespace := config.Spec.AgentInstallNamespace if applyDefault && len(namespace) == 0 { return "open-cluster-management-agent-addon", nil } return namespace, nil } func AgentInstallNamespaceFromDeploymentConfigFunc( adcgetter AddOnDeploymentConfigGetter, ) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error) { return func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { return getAgentInstallNamespaceFromDeploymentConfig(addon, adcgetter, true) } } func AgentInstallNamespaceFromDeploymentConfigRawFunc( adcgetter AddOnDeploymentConfigGetter, ) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error) { return func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { return getAgentInstallNamespaceFromDeploymentConfig(addon, adcgetter, false) } }This would improve maintainability by consolidating the retrieval logic in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/0000_02_addon.open-cluster-management.io_addondeploymentconfigs.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/addon/v1alpha1/types_addondeploymentconfig.gois excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1/types.gois excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yamlis excluded by!vendor/**vendor/open-cluster-management.io/api/work/v1alpha1/types_manifestworkreplicaset.gois excluded by!vendor/**
📒 Files selected for processing (3)
go.mod(1 hunks)pkg/utils/addon_config.go(2 hunks)pkg/utils/addon_config_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/utils/addon_config.go (2)
vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.go (1)
ManagedClusterAddOn(19-32)vendor/k8s.io/klog/v2/klog.go (2)
V(1353-1355)InfoS(1530-1532)
pkg/utils/addon_config_test.go (4)
vendor/open-cluster-management.io/api/addon/v1alpha1/types_addondeploymentconfig.go (2)
AddOnDeploymentConfig(14-21)AddOnDeploymentConfigSpec(23-72)vendor/open-cluster-management.io/api/addon/v1alpha1/types_managedclusteraddon.go (3)
ManagedClusterAddOn(19-32)ManagedClusterAddOnStatus(100-161)ConfigReference(184-205)vendor/open-cluster-management.io/api/addon/v1alpha1/types_clustermanagementaddon.go (3)
ConfigGroupResource(104-115)ConfigReferent(118-129)ConfigSpecHash(132-138)pkg/utils/addon_config.go (2)
AddOnDeploymentConfigGetter(16-18)AgentInstallNamespaceFromDeploymentConfigRawFunc(81-104)
⏰ 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: verify
- GitHub Check: e2e-hosted
- GitHub Check: build
- GitHub Check: unit
- GitHub Check: e2e
- GitHub Check: e2e-cloudevents
- GitHub Check: integration
🔇 Additional comments (5)
go.mod (1)
25-25: LGTM! Dependency update aligns with PR objectives.The update to
open-cluster-management.io/apienables support for emptyAgentInstallNamespacefield, which is required for the template-type addon functionality introduced in this PR.pkg/utils/addon_config.go (2)
37-39: Clear documentation of backward compatibility.The added documentation effectively explains the behavioral difference between the two functions, making it clear when developers should use each variant.
64-69: LGTM! Backward compatibility preserved.The logic correctly returns the default namespace when
AgentInstallNamespaceis empty, ensuring existing addons continue to work without modification.pkg/utils/addon_config_test.go (2)
131-165: LGTM! Test case validates backward compatibility.The test case correctly verifies that when
AgentInstallNamespaceis empty, the function returns the default namespace "open-cluster-management-agent-addon". The spec hash44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8aappears to be the SHA256 hash of an emptyAddOnDeploymentConfigSpecwith only theAgentInstallNamespacefield set to empty string.
212-331: LGTM! Comprehensive test coverage for the raw function.The test function thoroughly validates the new raw variant's behavior:
- Nil addon handling
- Missing config reference handling
- Empty
AgentInstallNamespacereturning empty string (key difference from the non-raw function)- Non-empty
AgentInstallNamespacereturning the configured valueThe test cases correctly verify that the raw function preserves empty strings instead of applying defaults, which is the intended behavior for template-type addons.
zhujian7
changed the title
|
/cc @qiujian16 |
| // defined in the template manifest itself, rather than applying a default namespace. | ||
| // If the addon does not support addon deployment config or there is no matched addon deployment config, | ||
| // it will return an empty string. | ||
| func AgentInstallNamespaceFromDeploymentConfigRawFunc( |
There was a problem hiding this comment.
should this be defined in ocm given it is only used for addonTemplate?
There was a problem hiding this comment.
Not 100% sure if this could be used by other cases.
There was a problem hiding this comment.
AgentInstallNamespaceFromDeploymentConfigRawFunc the name is a big confused. Why RawFunc?
There was a problem hiding this comment.
This is compared with AgentInstallNamespaceFromDeploymentConfigFunc, AgentInstallNamespaceFromDeploymentConfigFunc returns the default open-cluster-management-agent-addon if the addondeploymentconfig.spec.installnamespace is empty, AgentInstallNamespaceFromDeploymentConfigRawFunc returns empty directly.
|
/hold |
2 participants
Description
This PR adds support for template-type addons to use the namespace defined in their template manifest by setting
agentInstallNamespace: ""in the AddonDeploymentConfig.Changes
1. API Upgrade
open-cluster-management.io/apito include 🐛 Allow empty string for AgentInstallNamespace to support template namespaces api#400AgentInstallNamespacefield2. New Function
Added
AgentInstallNamespaceFromDeploymentConfigRawFuncthat:AgentInstallNamespacevalue including empty stringsagentInstallNamespace: ""3. Backward Compatibility
Updated existing
AgentInstallNamespaceFromDeploymentConfigFuncto:"open-cluster-management-agent-addon"whenAgentInstallNamespaceis empty4. Tests
AgentInstallNamespacereturning default namespaceRelated Issues
open-cluster-management-io/ocm#1209
Testing
All unit tests pass:
```
go test ./pkg/utils/... -v -run TestAgentInstallNamespace
```
Backward Compatibility
✅ This change is fully backward compatible. Existing addons using
AgentInstallNamespaceFromDeploymentConfigFuncwill continue to receive the default namespace whenAgentInstallNamespaceis empty or unset.Template-type addons that want the new behavior should migrate to use
AgentInstallNamespaceFromDeploymentConfigRawFunc.Summary by CodeRabbit
New Features
Chores
Tests