✨ use clusteradm instead of custom code for labeling, ns creation #58
Conversation
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
WalkthroughBumps Helm chart image tag to v0.0.13. Refactors addon handling by removing addon client propagation, threading FleetConfig through hub/spoke addon flows, updating function signatures, adding labels to addon create/enable commands, and adjusting hub handling to call the revised handleHubAddons. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (1)
186-205: Nil dereference risk when AddOnTemplate is NotFoundIf Get() returns NotFound, addon is nil; dereferencing addon.Spec.AddonName will panic.
- // delete the addon template - if addon != nil { - err = addonC.AddonV1alpha1().AddOnTemplates().Delete(ctx, addonName, metav1.DeleteOptions{}) - if err != nil && !kerrs.IsNotFound(err) { - errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) - continue - } - } - - baseAddonName := addon.Spec.AddonName - // get the addon name without a version suffix, add it to purge list - purgeList = append(purgeList, baseAddonName) + // delete the addon template + if addon != nil { + err = addonC.AddonV1alpha1().AddOnTemplates().Delete(ctx, addonName, metav1.DeleteOptions{}) + if err != nil && !kerrs.IsNotFound(err) { + errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) + continue + } + // get the addon name without a version suffix, add it to purge list + purgeList = append(purgeList, addon.Spec.AddonName) + } else { + // Template missing; skip purge to avoid accidental CMA deletion for out-of-band templates + logger.V(3).Info("addon template not found, skipping purge candidate", "AddOnTemplate", addonName) + }
🧹 Nitpick comments (3)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (3)
115-123: Use %s for formatting labels (minor)Prefer %s over %v for clarity since ManagedBySelector.String() returns a string.
- fmt.Sprintf("--labels=%v", v1alpha1.ManagedBySelector.String()), + fmt.Sprintf("--labels=%s", v1alpha1.ManagedBySelector.String()),
374-401: Skip-paths on disable: consider one more benign case (optional)Some clusteradm versions return “No resources found” on disable; consider treating that as success as well.
- if strings.Contains(outStr, "add-on not found") { + if strings.Contains(outStr, "add-on not found") || strings.Contains(outStr, "No resources found") { logger.V(5).Info("addon already disabled (not found)", "managedcluster", spokeName, "addons", addons, "output", outStr) return nil }
316-364: Add minimal unit coverage for args construction (optional)Recommend tests that validate:
- baseArgs includes labels and kubeconfig/timeout from fc.BaseArgs
- --clusters used instead of --cluster
- --annotate omitted when no annotations
I can add table-driven tests for handleAddonEnable/handleAddonDisable to assert arg slices.
📜 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 (5)
fleetconfig-controller/charts/fleetconfig-controller/README.md(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/values.yaml(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/addon.go(13 hunks)fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)fleetconfig-controller/internal/controller/v1alpha1/spoke.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T17:55:52.159Z
Learnt from: TylerGillson
PR: open-cluster-management-io/lab#51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.159Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.
Applied to files:
fleetconfig-controller/charts/fleetconfig-controller/README.mdfleetconfig-controller/charts/fleetconfig-controller/values.yaml
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1alpha1/spoke.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
AddOnConfig(701-724)Spoke(349-391)AddOn(429-441)InstalledHubAddOn(504-513)HubAddOn(799-813)fleetconfig-controller/api/v1alpha1/constants.go (1)
ManagedBySelector(98-98)
⏰ 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) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (10)
fleetconfig-controller/charts/fleetconfig-controller/values.yaml (1)
279-281: Image tag updated to v0.0.13 — LGTMIn sync with README. Nothing else to change.
fleetconfig-controller/internal/controller/v1alpha1/addon.go (5)
89-92: Signature change to handleAddonCreate — OKCall site updated to pass fc; aligns with moving to fc.BaseArgs()/clusteradm flags.
411-469: HubAddOns logic assumes ClusterManager bundleVersion; verify singleton pathIf hub.singletonControlPlane is used (no ClusterManager), accessing fc.Spec.Hub.ClusterManager.Source.BundleVersion can nil-deref. Confirm singleton isn’t supported yet, or guard accordingly.
481-486: Uninstall hub-addon now uses fc.BaseArgs — LGTM
529-536: Install hub-addon with --create-namespace and BaseArgs — LGTMMatches PR goal to rely on clusteradm flags.
324-330: Correct syntax forclusteradm addon enable
- The command uses the
--clustersflag (plural) to specify one or more managed cluster names, not--cluster(github.com, open-cluster-management.io)- Supported flags for this subcommand are:
--names <addon-name>--namespace <namespace>--clusters <cluster1>,<cluster2>,…- Optional:
--annotate <key=value>(only include when non-empty)- Optional:
--context <kubeconfig-context>- There is no documented
--labelsflag foraddon enable; remove any--labelsusage or replace it with one of the supported flags above (github.com)Example:
clusteradm addon enable \ --names my-addon \ --namespace open-cluster-management-agent-addon \ --clusters cluster1,cluster2 \ [--annotate addon.open-cluster-management.io/on-multicluster-hub=true] \ [--context my-hub-context]Likely an incorrect or invalid review comment.
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (3)
178-201: Switched to handleSpokeAddons(ctx, spoke, fc) — LGTMUses fc-threaded flow; conditions updated correctly.
632-636: Passing fc to handleAddonDisable in deregister — LGTMEnsures BaseArgs are applied during cleanup.
546-565: All checks passed: No stale--cluster=flags or mismatched addon handler signaturesThe repository-wide grep confirmed:
- No occurrences of the singular
--cluster=flag in any addon enable paths.- Every call to
handleAddonEnablematches the signature(ctx, spokeName string, []v1alpha1.AddOn, *FleetConfig).- Every call to
handleAddonDisablematches the signature(ctx, spokeName string, []string, *FleetConfig).No further changes are necessary.
fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
97-103: Updated call to handleHubAddons(ctx, addonC, fc) — LGTM
arturshadnik
changed the title
|
[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 |
openshift-merge-bot
bot
merged commit fa19db2
into
open-cluster-management-io:main
2 participants
Description
--labelsflag for addon create and enable, remove custom labeling code--create-namespaceflagBaseArgsto all addon related clusteradm invokationsRegression testing
addon createandaddon enableresources still have theaddon.open-cluster-management.io/managedBy: fleetconfig-controllerlabelSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores