Skip to content

⚠️ remove cmamanagedby migration shim and drop addon-framework v0.9.3 support#383

Open
tesshuflower wants to merge 1 commit into
open-cluster-management-io:mainfrom
tesshuflower:drop-v0.9.3-support
Open

⚠️ remove cmamanagedby migration shim and drop addon-framework v0.9.3 support#383
tesshuflower wants to merge 1 commit into
open-cluster-management-io:mainfrom
tesshuflower:drop-v0.9.3-support

Conversation

@tesshuflower

@tesshuflower tesshuflower commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Removes all compatibility code that allowed addons built with addon-framework ≤v0.9.3
(using the deprecated WithInstallStrategy()) to coexist with the OCM addon-manager
via the lifecycle: self annotation. WithInstallStrategy() was deprecated in v0.9.0
and removed from addon-framework in v0.10.0. The migration period is now complete.

Changes

  • Delete pkg/addonmanager/controllers/cmamanagedby/ — the migration shim that cleared
    lifecycle: self annotations on ClusterManagementAddOn objects
  • Remove cmamanagedby controller instantiation from base_manager.go
  • Remove ManagedByAddonManager filter function and AddonManagementFilterFunc type
    from pkg/utils/helpers.go — no code reads the lifecycle annotation anymore
  • Remove lifecycle: addon-manager annotation from integration test CMA setup — now inert

Migration

Addon developers still using WithInstallStrategy() must migrate to declaring
spec.installStrategy in their ClusterManagementAddOn CR. See the
developer guide.

Ref: open-cluster-management-io/ocm#1428

Summary by CodeRabbit

  • Refactor

    • Removed legacy self-management flow so addons follow the unified management path.
    • Removed helper that inspected addon lifecycle annotations; lifecycle annotations are no longer set during test resource creation.
  • Tests

    • Removed tests related to the removed self-management controller and updated integration helpers accordingly.

@openshift-ci openshift-ci Bot requested review from qiujian16 and zhiweiyin318 June 11, 2026 16:53
@openshift-ci

openshift-ci Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tesshuflower
Once this PR has been reviewed and has the lgtm label, please assign zhiweiyin318 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f79f70b-dc3e-4c7d-ae43-71b1785867f6

📥 Commits

Reviewing files that changed from the base of the PR and between 66a54c8 and 6dd2039.

📒 Files selected for processing (6)
  • pkg/addonmanager/base_manager.go
  • pkg/addonmanager/controllers/cmamanagedby/controller.go
  • pkg/addonmanager/controllers/cmamanagedby/controller_test.go
  • pkg/utils/helpers.go
  • test/integration/kube/assertion_test.go
  • test/integration/v1alpha1_kube/assertion_test.go
💤 Files with no reviewable changes (4)
  • test/integration/kube/assertion_test.go
  • test/integration/v1alpha1_kube/assertion_test.go
  • pkg/addonmanager/controllers/cmamanagedby/controller.go
  • pkg/addonmanager/controllers/cmamanagedby/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/addonmanager/base_manager.go

Walkthrough

This PR removes the CMAManagedBy controller and its startup wiring, deletes the ManagedByAddonManager helper and v1alpha1 import, and updates tests to stop setting the addon lifecycle annotation when creating ClusterManagementAddOn objects.

Changes

CMAManagedBy Controller and Lifecycle Annotation Removal

Layer / File(s) Summary
Controller implementation and startup wiring removal
pkg/addonmanager/base_manager.go
The cmamanagedby import is removed, the controller is no longer constructed during startup, and its Run goroutine is no longer started in BaseAddonManagerImpl.StartWithInformers.
Helper removal and test annotation cleanup
pkg/utils/helpers.go, test/integration/kube/assertion_test.go, test/integration/v1alpha1_kube/assertion_test.go
Removes the ManagedByAddonManager helper and the v1alpha1 import from helpers; test helpers no longer set the addon lifecycle annotation when creating ClusterManagementAddOn objects.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested labels

lgtm, approved

Suggested reviewers

  • haoqing0110
  • qiujian16
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing the cmamanagedby migration shim and dropping support for addon-framework v0.9.3, which is the primary objective of this PR.
Description check ✅ Passed The description comprehensively covers all aspects: summary of changes, detailed list of deletions, migration guidance for addon developers, and reference to the related issue. All required template sections are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mikeshng

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@qiujian16

Copy link
Copy Markdown
Member

/assign @haoqing0110

… support

Removes all compatibility code that allowed addons built with
addon-framework <=v0.9.3 (using the deprecated WithInstallStrategy())
to coexist with the OCM addon-manager via the lifecycle: self annotation.

- Delete pkg/addonmanager/controllers/cmamanagedby/ — the migration
  shim that cleared lifecycle: self annotations on ClusterManagementAddOns
- Remove cmamanagedby controller instantiation from base_manager.go
- Remove ManagedByAddonManager filter function and AddonManagementFilterFunc
  type from pkg/utils/helpers.go — the annotation check is no longer needed
- Remove lifecycle: addon-manager annotation from integration test CMA
  setup — the annotation is now inert and has no effect

Addon developers still using WithInstallStrategy() must migrate to
declaring spec.installStrategy in their ClusterManagementAddOn CR.
See open-cluster-management-io/ocm#1428

Closes open-cluster-management-io/ocm#355

Signed-off-by: Tesshu Flower <tflower@redhat.com>
@tesshuflower tesshuflower force-pushed the drop-v0.9.3-support branch from f11dab6 to 6dd2039 Compare June 12, 2026 16:55
@tesshuflower

Copy link
Copy Markdown
Contributor Author

The e2e checks are hitting the same error with the GH action engineerd/setup-kind as we've seen in the OCM repo - there is some discrepancy between the code in v0.6.2 tag vs the v0.6.2 branch. Perhaps Github started prioritizing the tag vs the branch name and things started to break (either that or the tag has moved).

Started separate PR here to address: #386

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants