✨ allow installing and uninstalling hub-addons #50
Conversation
WalkthroughAdds hub add-on support: a new InstalledHubAddOn API type and status field, CRD/schema and Helm values/templates/docs updates for hubAddOns, validation enhancements, expanded RBAC verbs, and controller logic to reconcile (install/uninstall) hub add-ons and update status. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fleetconfig-controller/internal/controller/hub.go (1)
210-252: Fix inverted upgrade logic in hubNeedsUpgradeThe final return in hubNeedsUpgrade currently uses
==, causing upgrades to trigger when the active bundle version already matches the desired version. It should use!=so that an upgrade is only flagged when the versions differ.• File: fleetconfig-controller/internal/controller/hub.go (line 251)
• Change the equality check to inequality:- return activeBundleVersion == fc.Spec.Hub.ClusterManager.Source.BundleVersion, nil + return activeBundleVersion != fc.Spec.Hub.ClusterManager.Source.BundleVersion, nilfleetconfig-controller/api/v1alpha1/fleetconfig_types.go (1)
80-87: GetCondition returns pointer to the range variable copy (not the slice element)Returning &c yields a pointer to the iteration variable, which is a copy. Mutating the returned pointer will not update s.Conditions. Return a pointer to the slice element instead.
Apply this diff:
func (s *FleetConfigStatus) GetCondition(cType string) *Condition { - for _, c := range s.Conditions { - if c.Type == cType { - return &c - } - } + for i := range s.Conditions { + if s.Conditions[i].Type == cType { + return &s.Conditions[i] + } + } return nil }
🧹 Nitpick comments (7)
fleetconfig-controller/charts/fleetconfig-controller/README.md (1)
84-85: Document allowed hubAddOns and options; include a quick example.To help users configure this correctly, list the allowed built-in add-on names and the available per-item fields, and add a short example. Based on the CRD, valid names include "argocd" and "governance-policy-framework", and optional fields include installNamespace (default: "open-cluster-management-addon") and createNamespace (default: false).
Example values snippet (for docs only):
fleetConfig: hub: hubAddOns: - name: argocd installNamespace: openshift-gitops createNamespace: true - name: governance-policy-frameworkfleetconfig-controller/charts/fleetconfig-controller/templates/fleetconfig.yaml (1)
114-115: Gate hubAddOns rendering for consistency and to avoid emitting empty arrays.addOnConfigs is conditionally rendered; do the same for hubAddOns to reduce churn in rendered manifests and retain compatibility with older CRDs during upgrades.
Apply this diff:
- hubAddOns: {{- toYaml .Values.fleetConfig.hub.hubAddOns | nindent 4 }} + {{- if .Values.fleetConfig.hub.hubAddOns }} + hubAddOns: {{- toYaml .Values.fleetConfig.hub.hubAddOns | nindent 4 }} + {{- end }}fleetconfig-controller/internal/controller/hub.go (1)
105-109: Optionally set AddonsConfigured=False when no addons are specified.Currently, the condition is only set to True when there are any AddOnConfigs or HubAddOns; when there are none, no condition is set, which may leave stale state from previous reconciliations. Consider explicitly setting AddonsConfigured to False when the combined count is zero.
Apply this diff:
- if len(fc.Spec.AddOnConfigs)+len(fc.Spec.HubAddOns) > 0 { - fc.SetConditions(true, v1alpha1.NewCondition( - v1alpha1.FleetConfigAddonsConfigured, v1alpha1.FleetConfigAddonsConfigured, metav1.ConditionTrue, metav1.ConditionTrue, - )) - } + if len(fc.Spec.AddOnConfigs)+len(fc.Spec.HubAddOns) > 0 { + fc.SetConditions(true, v1alpha1.NewCondition( + v1alpha1.FleetConfigAddonsConfigured, v1alpha1.FleetConfigAddonsConfigured, metav1.ConditionTrue, metav1.ConditionTrue, + )) + } else { + fc.SetConditions(true, v1alpha1.NewCondition( + "no add-ons specified", v1alpha1.FleetConfigAddonsConfigured, metav1.ConditionFalse, metav1.ConditionTrue, + )) + }fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml (1)
2672-2691: CRD status.installedHubAddOns schema looks good; consider list typing metadata.If you plan to enforce uniqueness or treat the list semantically, you might add x-kubernetes-list-type (e.g., "atomic") and, if keying by name+bundleVersion, x-kubernetes-list-map-keys in future to help clients merge strategically. Not required now, but worth noting for future evolutions.
fleetconfig-controller/internal/controller/addon.go (2)
627-639: Consider using a more robust namespace creation approachThe workaround for ArgoCD namespace creation should be temporary. Consider implementing a more generic solution that handles namespace creation for all hub add-ons that require it.
Would you like me to help implement a more generic namespace creation solution that could handle this requirement for all hub add-ons, not just ArgoCD? This would make the code more maintainable when the upstream fix is available.
667-676: Add more specific error handling for add-on detectionThe function assumes that finding a ClusterManagementAddOn means it was installed by
install hub-addon. Consider adding additional checks or labels to ensure the add-on was actually installed as a hub add-on versus a regular add-on.func isAddonInstalled(ctx context.Context, addonC *addonapi.Clientset, addonName string) (bool, error) { - if _, err := addonC.AddonV1alpha1().ClusterManagementAddOns().Get(ctx, addonName, metav1.GetOptions{}); err != nil { + cma, err := addonC.AddonV1alpha1().ClusterManagementAddOns().Get(ctx, addonName, metav1.GetOptions{}) + if err != nil { return false, client.IgnoreNotFound(err) } + // Check if the addon has labels or annotations indicating it's a hub addon + // This provides more certainty about the addon's origin + if cma.Labels != nil { + if source, ok := cma.Labels["addon.open-cluster-management.io/source"]; ok && source == "hub-addon" { + return true, nil + } + } + // we enforce unique names between hubAddOns and addOnConfigs, // and handle deleting addOnConfigs first // so if the addon is found here, we can assume it was previously installed by `install hub-addon` return true, nil }fleetconfig-controller/api/v1alpha1/validation.go (1)
206-211: Minor: Inconsistent error message formattingThe error message ends with a period, while other similar error messages in the file don't. Consider removing the period for consistency.
if _, found := addOnConfigNames[ha.Name]; found { errs = append(errs, field.Invalid(field.NewPath("hubAddOns").Index(i), ha.Name, - fmt.Sprintf("hubAddOn name %s clashes with an existing addOnConfig name.", ha.Name))) + fmt.Sprintf("hubAddOn name %s clashes with an existing addOnConfig name", ha.Name))) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(2 hunks)fleetconfig-controller/api/v1alpha1/validation.go(3 hunks)fleetconfig-controller/api/v1alpha1/zz_generated.deepcopy.go(2 hunks)fleetconfig-controller/charts/fleetconfig-controller/README.md(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/fleetconfig.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/rbac/manager-rbac.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/values.yaml(1 hunks)fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(1 hunks)fleetconfig-controller/internal/controller/addon.go(2 hunks)fleetconfig-controller/internal/controller/hub.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
fleetconfig-controller/internal/controller/hub.go (1)
fleetconfig-controller/api/v1alpha1/constants.go (1)
FleetConfigAddonsConfigured(16-16)
fleetconfig-controller/internal/controller/addon.go (2)
fleetconfig-controller/vendor/open-cluster-management.io/api/client/addon/clientset/versioned/clientset.go (1)
Clientset(21-24)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)
🔇 Additional comments (12)
fleetconfig-controller/api/v1alpha1/zz_generated.deepcopy.go (2)
222-226: LGTM: status.InstalledHubAddOns deep-copy is correct for value-only elements.Shallow-copying a slice of structs with only string fields is safe. No action.
315-328: LGTM: InstalledHubAddOn DeepCopy methods added.Generated methods match the new type and are consistent with controller-gen output.
fleetconfig-controller/internal/controller/hub.go (1)
97-104: Good integration: hub add-on handling errors flow into AddonsConfigured condition.Error propagation and condition setting mirror the existing AddOnConfigs path. No functional concerns here.
fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml (1)
2661-2681: LGTM! New status field for tracking installed hub add-onsThe addition of the
installedHubAddOnsarray in the status section is well-structured with all necessary fields (bundleVersion,name, and optionalnamespace) properly defined with required constraints.fleetconfig-controller/charts/fleetconfig-controller/templates/rbac/manager-rbac.yaml (1)
136-140: Verify necessity of write verbs for specific resourcesI didn’t find any direct use of
Create,Update,Patch, orDeleteonAddonPlacementScore,ManagedClusterSetBinding, orPlacementin the fleetconfig-controller codebase. The only mentions of those types came from the dashboard/apiserver module; there were no calls to the controller-runtime client or dynamic client operating on these resources in the operator’s controllers.• Please manually review the controller code—both typed (e.g.
r.Client.Create(ctx, obj, opts)) and any unstructured/dynamic client usage—to confirm whether the operator ever writes to these CRDs.
• If no write operations are performed, removecreate,update,patch, anddeletefrom the RBAC rules for these resources to adhere to least privilege.fleetconfig-controller/internal/controller/addon.go (2)
29-44: LGTM! Well-organized constants for hub add-on managementThe constants are properly organized and follow Go naming conventions.
509-567: Well-structured hub add-on reconciliation logicThe function properly handles:
- Finding add-ons to uninstall (version mismatches or removed add-ons)
- Finding add-ons to install (new or version upgrades)
- Performing operations in the correct order (uninstalls before installs)
- Updating the status with the new installed add-ons list
fleetconfig-controller/api/v1alpha1/validation.go (3)
88-98: LGTM! Proper validation for AddOnConfig uniquenessThe validation correctly enforces uniqueness of AddOnConfig entries using the composite key (name-version) and provides clear error messages with conflicting indices.
175-194: LGTM! Comprehensive cross-field validationThe validation properly checks that spoke add-ons reference existing configurations in either AddOnConfigs or HubAddOns, with an improved error message that mentions both possible sources.
213-222: LGTM! Proper validation for HubAddOn name uniquenessThe validation correctly enforces uniqueness of HubAddOn names and provides clear error messages with conflicting indices.
fleetconfig-controller/charts/fleetconfig-controller/values.yaml (1)
71-75: LGTM! Well-documented hub add-ons configurationThe new
hubAddOnsconfiguration is properly documented with clear examples and constraints:
- Restricted to specific add-on names ("argocd", "governance-policy-framework")
- Clear namespace behavior for each add-on type
- Optional namespace creation flag
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (1)
64-68: Our searches indicate:
- The
Equalmethod infleetconfig_types.gocurrently only comparesConditionsand ignoresPhaseandInstalledHubAddOns.- We found no direct invocations of
Status().PatchorStatus().Updateinfleetconfig_controller.go, but the reconciler uses a genericpatchHelper.Patch(ctx, fc)call at the end ofReconcile, which patches the full object (including status) using its diff from the original, based on fields modified onfcduring reconciliation.- Since
PhaseandInstalledHubAddOnsare part offc.Statusand are mutated in the reconciler beforepatchHelper.Patch(...), the client library will detect their changes in the in-memory object and include them in the patch—even ifEqualdoesn’t consider them.Conclusion:
Because the controller does not rely on
FleectConfigStatus.Equalto gate status writes—rather, it unconditionally patches any changes detected by the helper—the omission ofPhaseandInstalledHubAddOnsinEqualdoes not prevent updates to those fields.Action
You can preserve the existingEqualimplementation unchanged, or if you prefer to maintain a comprehensive, order-insensitive comparison for tests or other purposes, add comparisons forPhaseandInstalledHubAddOnsas proposed. However, no critical fix is required to ensure status updates work correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/addon.go (1)
502-507: Normalize namespace matching for ArgoCD to avoid false mismatches. Also: prior BundleVersion typo appears resolved.Because ArgoCD ignores the install namespace (chart hardcodes it), comparing desired.InstallNamespace to installed.Namespace can drift and trigger unnecessary uninstall/reinstall cycles if the user toggles or sets a non-effective namespace. Normalize the expected namespace before comparison.
Also confirming the earlier “BundleVerion” typo is fixed here; thanks for addressing it.
Apply this diff:
func isHubAddOnMatching(installed v1alpha1.InstalledHubAddOn, desired v1alpha1.HubAddOn, bundleVersion string) bool { - return installed.Name == desired.Name && - installed.Namespace == desired.InstallNamespace && - installed.BundleVersion == bundleVersion + expNS := effectiveInstallNamespace(desired) + return installed.Name == desired.Name && + installed.Namespace == expNS && + installed.BundleVersion == bundleVersion }Add this helper (place near other helpers):
// effectiveInstallNamespace returns the real namespace that will be used for the hub add-on. // Some hub add-ons (e.g., ArgoCD) ignore the chosen namespace and deploy into a fixed one. func effectiveInstallNamespace(desired v1alpha1.HubAddOn) string { if desired.Name == hubAddOnArgoCD { return argocdNamespace } return desired.InstallNamespace }
🧹 Nitpick comments (5)
fleetconfig-controller/internal/controller/addon.go (5)
39-44: Fix typos and hyphenation in comment.Minor doc polish.
Apply this diff:
-// accepetd hub addon names +// accepted hub add-on names
546-554: Pass FleetConfig base args to hub-addon operations.Other clusteradm calls use fc.BaseArgs(); hub-addon install/uninstall should be consistent (kubeconfig/context/registry, etc.). That requires threading fc into the helper calls.
Apply this diff:
- err := handleHubAddonUninstall(ctx, addonsToUninstall) + err := handleHubAddonUninstall(ctx, fc, addonsToUninstall) if err != nil { return err } - err = handleHubAddonInstall(ctx, kClient, addonC, addonsToInstall, bundleVersion) + err = handleHubAddonInstall(ctx, kClient, addonC, fc, addonsToInstall, bundleVersion)
556-565: Record the effective namespace in status (reflect ArgoCD reality).Status should reflect where the add-on is actually installed. For ArgoCD, that’s always “argocd”.
Apply this diff:
for _, d := range desiredAddOns { newInstalledAddOns = append(newInstalledAddOns, v1alpha1.InstalledHubAddOn{ Name: d.Name, - Namespace: d.InstallNamespace, + Namespace: effectiveInstallNamespace(d), BundleVersion: bundleVersion, }) }
569-605: Make uninstall resilient and consistent: include BaseArgs; treat “not found” as success.
- Include fc.BaseArgs so the command runs in the correct context/with the right registry.
- Uninstall should be idempotent; treat “not found” conditions as success (similar to disable).
Apply this diff:
-func handleHubAddonUninstall(ctx context.Context, addons []v1alpha1.InstalledHubAddOn) error { +func handleHubAddonUninstall(ctx context.Context, fc *v1alpha1.FleetConfig, addons []v1alpha1.InstalledHubAddOn) error { @@ for _, addon := range addons { - args := []string{ - uninstall, - hubAddon, - fmt.Sprintf("--names=%s", addon.Name), - } + args := append([]string{ + uninstall, + hubAddon, + fmt.Sprintf("--names=%s", addon.Name), + }, fc.BaseArgs()...) if addon.Namespace != "" { args = append(args, fmt.Sprintf("--namespace=%s", addon.Namespace)) } @@ stdout, stderr, err := exec_utils.CmdWithLogs(ctx, cmd, "waiting for 'clusteradm uninstall hub-addon' to complete...") if err != nil { out := append(stdout, stderr...) outStr := string(out) + // idempotence: consider "not found" as success + if strings.Contains(outStr, "not found") { + logger.V(5).Info("hub addon already uninstalled (not found)", "name", addon.Name, "namespace", addon.Namespace, "output", outStr) + continue + } errs = append(errs, fmt.Errorf("failed to uninstall hubAddon %s: %v, output: %s", addon.Name, err, outStr)) continue }
667-676: Now-unused helper; consider removing or making it version-aware.With the “installed?” short-circuit removed, this function is either unused or insufficient because it ignores bundle version. Suggest removing to avoid dead code or extend it to read the installed bundle version (if clusteradm/CMA exposes it).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(2 hunks)fleetconfig-controller/internal/controller/addon.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/api/v1alpha1/fleetconfig_types.go
🧰 Additional context used
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/addon.go (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
InstalledHubAddOn(493-500)HubAddOn(786-800)FleetConfig(719-725)Hub(172-204)ClusterManager(244-280)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/internal/controller/addon.go (2)
35-38: Good: clear, centralized constants for hub-addon subcommands.This improves readability and avoids stringly-typed usage scattered across the code.
606-665: Ensure version-aware upgrades, include FleetConfig base args, and correctly handle the--create-namespaceflag– Do not short-circuit purely on “already installed” status. To support upgrades, compare against
fc.Status.InstalledHubAddOnsfor matching name + bundleVersion, or remove theisAddonInstalledcheck entirely.
– Update the signature to acceptfc *v1alpha1.FleetConfig, then build your args with itsBaseArgs()for registry/context consistency.
– For namespace creation, use the presence flag syntax—only append--create-namespacewhenaddon.CreateNamespaceis true (omit--create-namespace=false). The correct clusteradm invocation is:clusteradm install hub-addon \ --names <addon-name> \ --bundle-version <version> \ --namespace <install-namespace> \ --create-namespace– Example diff:
func handleHubAddonInstall( ctx context.Context, kClient client.Client, addonC *addonapi.Clientset, - addons []v1alpha1.HubAddOn, + fc *v1alpha1.FleetConfig, + addons []v1alpha1.HubAddOn, bundleVersion string, ) error { for _, addon := range addons { - // (removed defensive isAddonInstalled short-circuit) + // Version-aware check: compare against fc.Status.InstalledHubAddOns (same name & bundleVersion) args := append([]string{ install, hubAddon, fmt.Sprintf("--names=%s", addon.Name), fmt.Sprintf("--bundle-version=%s", bundleVersion), + }, fc.BaseArgs()...) if addon.CreateNamespace { args = append(args, "--create-namespace") } if addon.InstallNamespace != "" { args = append(args, fmt.Sprintf("--namespace=%s", addon.InstallNamespace)) }Please verify:
- That
FleetConfig.BaseArgs()exists and returns the intended[]string.- Your upgrade logic against
fc.Status.InstalledHubAddOns(or removeisAddonInstalled) correctly handles same-version skips.- The resulting CLI flags align with the
clusteradm install hub-addonsemantics, using--create-namespaceonly when needed.
TylerGillson
left a comment
TylerGillson
left a comment
There was a problem hiding this comment.
Looks good! Just a couple of nits around whitespace.
fleetconfig-controller/charts/fleetconfig-controller/values.yaml
Outdated
Show resolved
Hide resolved
arturshadnik
force-pushed
the
feat/hub-addon-2
branch
from
096cab9 to
f2550ef
Compare
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Co-authored-by: Tyler Gillson <tyler.gillson@gmail.com> Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
arturshadnik
force-pushed
the
feat/hub-addon-2
branch
from
f2550ef to
e53e12a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arturshadnik, TylerGillson 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 |
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml (2)
2671-2691: Make installedHubAddOns a keyed list to prevent duplicate entriesAs this array represents a set keyed by addon identity, consider declaring it as a map list keyed by name. This enforces uniqueness at the API level and simplifies merges/patches.
Apply this diff:
installedHubAddOns: items: description: InstalledHubAddOn tracks metadata for each hubAddon that is successfully installed on the hub. properties: bundleVersion: description: BundleVersion is the bundle version used when installing the addon. type: string name: description: Name is the name of the addon. type: string namespace: description: Namespace is the namespace that the addon was installed into. type: string required: - bundleVersion - name type: object type: array + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - nameNotes:
- If you foresee multiple entries with the same name in different namespaces, keying on ["name","namespace"] would work, but then the controller must always set namespace (non-empty).
--- `2671-2691`: **Optional: add a printer column for the count of installed hub add-ons** Improves UX when listing FleetConfigs. Not required for correctness. Example (placed under versions[].additionalPrinterColumns): ```diff - jsonPath: .status.phase name: PHASE type: string + - jsonPath: .status.installedHubAddOns + name: HUB-ADDONS + type: integer + priority: 1 + description: Number of installed hub add-ons + format: int32fleetconfig-controller/charts/fleetconfig-controller/templates/fleetconfig.yaml (2)
114-114: Render hubAddOns only when set (avoid emitting an empty array) and mirror addOnConfigs gating.Currently this always renders
hubAddOns:and will output[]by default. That’s inconsistent with the conditional aroundaddOnConfigs, and it’s nicer to omit empty fields from the CR to reduce churn and keep the spec clean.Apply this diff to gate rendering on non-empty values:
- hubAddOns: {{- toYaml .Values.fleetConfig.hub.hubAddOns | nindent 4 }} + {{- with .Values.fleetConfig.hub.hubAddOns }} + hubAddOns: {{- toYaml . | nindent 4 }} + {{- end }}
114-114: Guard against nil values passed to toYaml to prevent surprises.If
fleetConfig.hub.hubAddOnsis ever omitted from values (e.g., in a consumer’s custom values),toYamlcould receive<no value>. While your chart appears to default this to[], adding a defensive default keeps the template robust.Alternative single-line tweak if you prefer always rendering the field:
- hubAddOns: {{- toYaml .Values.fleetConfig.hub.hubAddOns | nindent 4 }} + hubAddOns: {{- toYaml (.Values.fleetConfig.hub.hubAddOns | default (list)) | nindent 4 }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(2 hunks)fleetconfig-controller/api/v1alpha1/validation.go(3 hunks)fleetconfig-controller/api/v1alpha1/zz_generated.deepcopy.go(2 hunks)fleetconfig-controller/charts/fleetconfig-controller/README.md(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/fleetconfig.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/rbac/manager-rbac.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/values.yaml(1 hunks)fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(1 hunks)fleetconfig-controller/internal/controller/addon.go(2 hunks)fleetconfig-controller/internal/controller/hub.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- fleetconfig-controller/charts/fleetconfig-controller/README.md
- fleetconfig-controller/internal/controller/addon.go
- fleetconfig-controller/charts/fleetconfig-controller/values.yaml
- fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml
- fleetconfig-controller/api/v1alpha1/validation.go
- fleetconfig-controller/charts/fleetconfig-controller/templates/rbac/manager-rbac.yaml
- fleetconfig-controller/internal/controller/hub.go
- fleetconfig-controller/api/v1alpha1/fleetconfig_types.go
- fleetconfig-controller/api/v1alpha1/zz_generated.deepcopy.go
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (3)
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml (2)
2671-2691: Status field addition looks correct and consistent with the API intentThe new FleetConfig.status.installedHubAddOns schema is well-formed: required keys are appropriate, optional namespace is fine, and the placement under status with subresources.status declared will keep it controller-owned.
2671-2691: InstalledHubAddOns schema is identical in both base and chart CRDs—no action needed.Both
config/crd/bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml(lines 2659–2676) andcharts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml(lines 2669–2686) declare the sameinstalledHubAddOnsobject/array structure with matchingitems.properties.fleetconfig-controller/charts/fleetconfig-controller/templates/fleetconfig.yaml (1)
114-114: CRD & Go types alignment confirmedAll checks pass—
hubAddOnsis defined at the top level ofspec, not nested underspec.hub. No mismatches found.
- FleetConfigSpec in Go declares
at the same level asHubAddOns []HubAddOn `json:"hubAddOns,omitempty"`HubandSpokes.- CRD schema under
speclistshubAddOns:(sibling tohub:andspokes:), with no nestedspec.hub.hubAddOns.- Helm template’s
correctly emits the array ashubAddOns: {{- toYaml .Values.fleetConfig.hub.hubAddOns | nindent 4 }}spec.hubAddOns.No changes required.
openshift-merge-bot
bot
merged commit f6340bb
into
open-cluster-management-io:main
3 participants
This PR adds the ability for FleetConfig controller to handle installing and uninstalling
hub-addons.Notes:
argocdaddon will currently fail to uninstall, until a new release ofclusteradmcontaining 🐛 tweak argocd hub-addon install and uninstall clusteradm#510 is cut.argocdnamespace on the hub, due to the same issue ^.namespacefield has no effect when usingargocdaddon. The namespace is hardcoded in the helm chart andclusteradm.createNamespacedoes have an effect, and can be used to create theargocdnamespace if needed.hubAddOns) and custom (addOnConfigs), if either of the 2hubAddOnsis installed, the same name cannot be reused for anaddOnConfig.managedClusterSetBindings. manager ClusterRole was updated to allow this.Sanity testing
hubAddOns:governance-policy-frameworknamespace created and thegovernance-policy-frameworkcontrollers installed in it. FleetConfig is unhealthy due to argocd failing to install (argocdnamespace is missing)hubAddOnsto setcreateNamespace: trueforargocd. The argocd-pull-integration controller is installed on the hub.spokecluster.argocdnamespace.governance-policy-frameworkinopen-cluster-management-addonnamespace (currently not configurable, see Support adding extra config to addOns during enable clusteradm#501, installNamespace not respected when using AddonTemplate addon-framework#327).hubAddOnfrom the hub -> denied by webhookhubAddOn: Invalid value: []string{"argocd"}: cannot remove hubAddOns [argocd] as they are still in use by managedclusteraddonsSummary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation