🌱 Add default namespace support to AgentInstallNamespaceFromDeploymentConfigFunc#349
Conversation
…onfigFunc This enhancement allows callers to specify a fallback namespace when the addon deployment config doesn't specify an agent install namespace. The function now accepts an optional defaultNs parameter that will be returned when config.Spec.AgentInstallNamespace is empty. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe PR updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
/hold |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/utils/addon_config.go (1)
55-59: Missing default namespace fallback when config is nil.When
config == nil, the function returns an empty string without checking thedefaultNsparameter. However, the comment at lines 50-54 states: "we can use the default namespace" when the addon deployment config is not configured. This creates inconsistent behavior:
- When config is nil → returns empty string (ignores default)
- When config exists but AgentInstallNamespace is empty → returns default (lines 61-63)
Both cases should likely use the default namespace fallback.
Apply this diff to add the missing fallback:
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) + if len(defaultNs) > 0 { + return defaultNs[0], nil + } return "", nil }
🧹 Nitpick comments (1)
pkg/utils/addon_config.go (1)
61-63: LGTM with documentation update needed.The logic correctly returns the first default namespace when
config.Spec.AgentInstallNamespaceis empty and a default is provided.Consider updating the function documentation (lines 34-36) to describe the new default namespace behavior:
// AgentInstallNamespaceFromDeploymentConfigFunc returns an agent install namespace helper function which will get the // namespace from the addon deployment config. If the addon does not support addon deployment config or there is no -// matched addon deployment config, it will return an empty string. +// matched addon deployment config, it will return an empty string. If a default namespace is provided via defaultNs +// and the config's AgentInstallNamespace is empty, it will return the default namespace. func AgentInstallNamespaceFromDeploymentConfigFunc(Optional API clarity improvement:
The variadic parameter accepts multiple strings but only uses the first. Consider using a more explicit signature:
func AgentInstallNamespaceFromDeploymentConfigFunc( adcgetter AddOnDeploymentConfigGetter, defaultNs string, ) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error)Or if you want to keep the optional nature without variadic:
func AgentInstallNamespaceFromDeploymentConfigFunc( adcgetter AddOnDeploymentConfigGetter, opts ...Option, ) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error)where
Optionis a functional option pattern. However, the current variadic approach is acceptable if simplicity is preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/utils/addon_config.go(2 hunks)
⏰ 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: unit
- GitHub Check: e2e-cloudevents
- GitHub Check: e2e
- GitHub Check: integration
- GitHub Check: e2e-hosted
- GitHub Check: build
- GitHub Check: verify
| func AgentInstallNamespaceFromDeploymentConfigFunc( | ||
| adcgetter AddOnDeploymentConfigGetter, | ||
| adcgetter AddOnDeploymentConfigGetter, defaultNs ...string, | ||
| ) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP -C3 '\bAgentInstallNamespaceFromDeploymentConfigFunc\s*\(' --type=goRepository: open-cluster-management-io/addon-framework
Length of output: 228
Address inconsistent nil config handling and update documentation.
Lines 55-59 return an empty string when config == nil without checking defaultNs, while lines 61-63 correctly apply the default namespace when config != nil but AgentInstallNamespace is empty. This inconsistency violates the function's intent. Additionally, the function documentation (lines 34-36) doesn't mention the new default namespace behavior.
Fix the nil config case to also check defaultNs (e.g., return first element if available), and update the function comment to document the fallback behavior.
🤖 Prompt for AI Agents
In pkg/utils/addon_config.go around lines 34 to 63, the function comment and
nil-config branch are inconsistent: update the function comment to state that if
the AddOnDeploymentConfig is nil or its AgentInstallNamespace is empty, the
function will fall back to the first element of the optional defaultNs (if
provided); then change the nil-config handling so it returns defaultNs[0] (when
defaultNs is non-empty) instead of an empty string, preserving the existing
behavior for when no defaultNs is provided and keeping the existing error return
path unchanged.
1 participant
Summary
This enhancement adds optional default namespace support to
AgentInstallNamespaceFromDeploymentConfigFunc. When the addon deployment config doesn't specify anAgentInstallNamespace, callers can now provide a fallback default namespace.Changes
AgentInstallNamespaceFromDeploymentConfigFuncsignature to accept optionaldefaultNs ...stringparameterconfig.Spec.AgentInstallNamespaceis empty and a default is providedTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.