🐛 dont set negative addon conds if no addon ops were performed #64
Conversation
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
WalkthroughhandleAddonConfig and handleHubAddons now return (bool, error) to signal whether reconciliation changed state; hub.go captures these flags, adjusts error handling and FleetConfig condition setting based on change signals, and updates InstalledHubAddOns/status accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed Checks (1 warning)
✅ Passed Checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✨ 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. Comment |
TylerGillson
left a comment
TylerGillson
left a comment
There was a problem hiding this comment.
This will work, but can we also log a useful warning message with level 1 if the crds are not found?
![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 (2)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (2)
186-205: Fix nil-pointer deref when AddOnTemplate is NotFoundIf Get() returns NotFound, addon is nil, but code later dereferences addon.Spec.AddonName. This will panic.
addon, err := addonC.AddonV1alpha1().AddOnTemplates().Get(ctx, addonName, metav1.GetOptions{}) if err != nil && !kerrs.IsNotFound(err) { errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) continue } // 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 + if addon == nil { + // Template already gone; base AddOn name unknown. Skip purge evaluation for this entry. + logger.V(5).Info("addon template not found; skipping purge evaluation", "AddOnTemplate", addonName) + continue + } + baseAddonName := addon.Spec.AddonNameIf you prefer to still attempt purge without the template, we can derive the base name from our naming convention, but that’s lossy; above is safer.
411-469: Return “changed” only when install/uninstall ops are neededSame issue as handleAddonConfig: this currently returns changed=true even on no-ops. Compute changed from the diff and propagate it in error paths and the final return.
// do uninstalls first, then installs - err := handleHubAddonUninstall(ctx, addonsToUninstall, fc) - if err != nil { - return true, err - } - - err = handleHubAddonInstall(ctx, addonC, addonsToInstall, bundleVersion, fc) - if err != nil { - return true, err - } + changed := len(addonsToUninstall) > 0 || len(addonsToInstall) > 0 + if err := handleHubAddonUninstall(ctx, addonsToUninstall, fc); err != nil { + return changed, err + } + if err := handleHubAddonInstall(ctx, addonC, addonsToInstall, bundleVersion, fc); err != nil { + return changed, err + } // build the new installed addons list // ... fc.Status.InstalledHubAddOns = newInstalledAddOns - return true, nil + return changed, nil
🧹 Nitpick comments (2)
fleetconfig-controller/internal/controller/v1alpha1/hub.go (2)
89-96: Log suppressed errors when no addon ops were plannedWhen err != nil and addonConfigChanged==false, we intentionally don’t surface a negative condition. Add a low-verbosity log to aid triage.
- addonConfigChanged, err := handleAddonConfig(ctx, kClient, addonC, fc) + addonConfigChanged, err := handleAddonConfig(ctx, kClient, addonC, fc) if err != nil && addonConfigChanged { fc.SetConditions(true, v1alpha1.NewCondition( err.Error(), v1alpha1.FleetConfigAddonsConfigured, metav1.ConditionFalse, metav1.ConditionTrue, )) return err } + if err != nil && !addonConfigChanged { + log.FromContext(ctx).V(5).Info("ignoring addon-config error (no addon ops planned)", "error", err) + }
97-104: Same: log suppressed hub-addon errors when no ops were planned- hubAddonChanged, err := handleHubAddons(ctx, addonC, fc) + hubAddonChanged, err := handleHubAddons(ctx, addonC, fc) if err != nil && hubAddonChanged { fc.SetConditions(true, v1alpha1.NewCondition( err.Error(), v1alpha1.FleetConfigAddonsConfigured, metav1.ConditionFalse, metav1.ConditionTrue, )) return err } + if err != nil && !hubAddonChanged { + log.FromContext(ctx).V(5).Info("ignoring hub-addon error (no addon ops planned)", "error", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/controller/v1alpha1/addon.go(6 hunks)fleetconfig-controller/internal/controller/v1alpha1/hub.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
fleetconfig-controller/internal/controller/v1alpha1/hub.go (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (1)
NewCondition(146-157)fleetconfig-controller/api/v1alpha1/constants.go (1)
FleetConfigAddonsConfigured(16-16)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (3)
fleetconfig-controller/vendor/open-cluster-management.io/api/client/addon/clientset/versioned/clientset.go (1)
Clientset(21-24)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (1)
FleetConfig(732-738)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). (2)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/internal/controller/v1alpha1/addon.go (1)
42-47: Good: no negative condition when CRD list fails and no addons are configuredReturning (false, err) when requestedAddOns is empty ensures we don’t publish a negative condition for a no-op scenario (race with CRD creation). Looks aligned with the PR objective.
fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
105-111: Condition-setting relies on correct changed flags—verify after handler fixesOnce the handlers return accurate changed flags (see addon.go comments), this block will align with the PR objective. Without those fixes, this may still set success on no-ops.
Would you like me to open a follow-up PR after we merge the handler fixes to add an integration test ensuring we set AddonsConfigured only when actual create/delete/install/uninstall occurs?
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
|
[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 7b2a16d
into
open-cluster-management-io:main
2 participants
Prior to this fix, it was possible for a race condition to occur where the
handleAddonConfigsfunction would be executed before theAddOnTemplateCRD was created. This would cause an error which would be set as a negative condition on the FC, regardless of if any addons were actually configured. Once the CRD was available and the function returned a success, the negative condition would persist and not be removed, causing the FC to remainUnhealthy. This fix ensures that both positive and negative conditions are only set if there are addons being created or deleted.Summary by CodeRabbit
New Features
Bug Fixes
Refactor