WIP ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. #2315

camilamacedo86 · 2025-11-09T00:17:51Z

Revisions are now discovered using a label-based cache instead of walking explicit Spec.Previous chains.
This makes lookups faster, safer, and ensures no orphan revisions are left behind when a ClusterExtension is deleted.
Migration automatically sets ownerReferences for existing revisions.

What Changed

Before:

Revisions tracked previous ones via Spec.Previous
Migration didn’t set ownerReferences
Finding related revisions required walking the chain

Now:

Revisions are queried by label olm.operatorframework.io/owner
Filtered in-memory (skip current, archived, deleting)
Migration sets ownerReferences

Why It's Better

Faster: Uses cache, no API calls
Simpler: One label query instead of walking chains
Cleaner: Deleting a ClusterExtension auto-deletes its revisions
No orphans left behind

Where it matters ( With Boxcutter runtime only )

Upgrade: Migrated revisions get ownerReferences
Management: Faster revision lookups, less API load
Cleanup: Deleting a ClusterExtension removes all its revisions — no orphans left behind

netlify · 2025-11-09T00:17:56Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c7ea251`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/6910dc7b3fe89a000824ae42
😎 Deploy Preview	https://deploy-preview-2315--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull Request Overview

This PR refactors how previous revisions are discovered during ClusterExtensionRevision reconciliation. Instead of relying on a static Spec.Previous field, the controller now dynamically queries for previous revisions using the owner label, enabling more flexible revision management.

Key changes:

Introduced ListPreviousRevisions() method to dynamically discover sibling revisions
Updated toBoxcutterRevision() to call ListPreviousRevisions() and return errors
Enhanced test coverage with comprehensive test cases for the new listing logic
Improved test names and added scenario comments for better readability

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/operator-controller/controllers/clusterextensionrevision_controller.go	Added `ListPreviousRevisions()` method for dynamic revision discovery, refactored `toBoxcutterRevision()` signature to return errors, removed unused `unstructured` import
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go	Added comprehensive tests for `ListPreviousRevisions()`, updated test names to be more descriptive, added scenario comments, updated `mockTrackingCache` to delegate Get/List to actual client, removed obsolete `Spec.Previous` test setup, added owner label to test revision helper

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2025-11-09T00:29:55Z

Codecov Report

❌ Patch coverage is 69.35484% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.12%. Comparing base (b3f85d5) to head (6076304).

Files with missing lines	Patch %	Lines
...controllers/clusterextensionrevision_controller.go	59.45%	11 Missing and 4 partials ⚠️
internal/operator-controller/applier/boxcutter.go	83.33%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2315      +/-   ##
==========================================
+ Coverage   71.14%   74.12%   +2.97%     
==========================================
  Files          91       91              
  Lines        7046     7095      +49     
==========================================
+ Hits         5013     5259     +246     
+ Misses       1618     1416     -202     
- Partials      415      420       +5

Flag	Coverage Δ
e2e	`45.51% <0.00%> (-0.43%)`	⬇️
experimental-e2e	`48.01% <41.93%> (?)`
unit	`58.59% <67.74%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

openshift-ci · 2025-11-09T11:26:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

cmd/OWNERS
internal/operator-controller/OWNERS

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

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-09T13:18:18Z

internal/operator-controller/applier/boxcutter_test.go

+		client.
+			On("Update", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything).
+			Once().
+			Return(nil)


The mock expects an Update call but the implementation uses Status().Update() which calls the status writer's Update method. The mock should expect the status update through the status writer, not a direct Update on the client.

Copilot · 2025-11-09T13:18:18Z

internal/operator-controller/applier/boxcutter.go

+		ObservedGeneration: rev.Generation,
+	})
+	meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeSucceeded,
+		Status:             metav1.ConditionTrue,
+		Reason:             ocv1.ClusterExtensionRevisionReasonRolloutSuccess,
+		Message:            "Migrated from Helm storage",
+		ObservedGeneration: rev.Generation,


Using rev.Generation here will be 0 since the revision hasn't been persisted yet. The generation is only set by the API server after creation. This should likely be set to 1 or the ObservedGeneration field should be set after the revision is created and its generation is known.

Suggested change

ObservedGeneration: rev.Generation,

})

meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{

Type: ocv1.ClusterExtensionRevisionTypeSucceeded,

Status: metav1.ConditionTrue,

Reason: ocv1.ClusterExtensionRevisionReasonRolloutSuccess,

Message: "Migrated from Helm storage",

ObservedGeneration: rev.Generation,

ObservedGeneration: 1,

})

meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{

Type: ocv1.ClusterExtensionRevisionTypeSucceeded,

Status: metav1.ConditionTrue,

Reason: ocv1.ClusterExtensionRevisionReasonRolloutSuccess,

Message: "Migrated from Helm storage",

ObservedGeneration: 1,

…it chains Instead of tracking revision history through Spec.Previous fields, we now find related revisions using labels and the controller-runtime cache. This is more efficient and works better with controller-runtime's caching layer. To support this change, the Helm-to-Boxcutter migration now sets ownerReferences on migrated revisions, ensuring they work identically to newly created revisions. Assisted-by: Cursor

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-09T18:32:44Z

internal/operator-controller/controllers/clusterextensionrevision_controller.go

+// Scenario: Given a ClusterExtensionRevision, find all related active previous revisions
+//   - When the revision has no owner label, return empty list (revision not properly labeled)
+//   - When looking up by owner label, filter out the current revision itself
+//   - When a revision is archived or being deleting, exclude it from results


Typo in comment: "being deleting" should be "being deleted"

Suggested change

// - When a revision is archived or being deleting, exclude it from results

// - When a revision is archived or being deleted, exclude it from results

Copilot AI review requested due to automatic review settings November 9, 2025 00:17

camilamacedo86 requested a review from a team as a code owner November 9, 2025 00:17

openshift-ci bot requested review from grokspawn and rashmigottipati November 9, 2025 00:17

Copilot AI reviewed Nov 9, 2025

View reviewed changes

camilamacedo86 changed the title ~~✨ (chore): (Boxcutter): Use label selector for listing previous revisions~~ WIP ✨ (chore): (Boxcutter): Use label selector for listing previous revisions Nov 9, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2025

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch from 3a1c266 to 9c8f9af Compare November 9, 2025 11:26

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch from 9c8f9af to 6026ae8 Compare November 9, 2025 11:35

camilamacedo86 changed the title ~~WIP ✨ (chore): (Boxcutter): Use label selector for listing previous revisions~~ WIP ✨ (chore): (Boxcutter): se label-based cache for revision lookups instead of explicit chains; Nov 9, 2025

camilamacedo86 changed the title ~~WIP ✨ (chore): (Boxcutter): se label-based cache for revision lookups instead of explicit chains;~~ ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. Nov 9, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2025

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch 4 times, most recently from c067348 to 6076304 Compare November 9, 2025 12:57

Copilot AI review requested due to automatic review settings November 9, 2025 13:10

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch from 6076304 to 5a89bed Compare November 9, 2025 13:10

Copilot AI reviewed Nov 9, 2025

View reviewed changes

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch from 5a89bed to fbf27d6 Compare November 9, 2025 13:50

Copilot AI review requested due to automatic review settings November 9, 2025 18:24

camilamacedo86 force-pushed the boxcutter-use-labels-cache branch from fbf27d6 to c7ea251 Compare November 9, 2025 18:24

Copilot AI reviewed Nov 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. #2315

WIP ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. #2315

camilamacedo86 commented Nov 9, 2025 •

edited

Loading

Uh oh!

netlify bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codecov bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

openshift-ci bot commented Nov 9, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 9, 2025

Uh oh!

Copilot AI Nov 9, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	// - When a revision is archived or being deleting, exclude it from results
	// - When a revision is archived or being deleted, exclude it from results

WIP ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. #2315

Are you sure you want to change the base?

WIP ✨ (Valid for Boxcutter Runtime Only) switched to label-based revision lookups — faster, safer, and no orphans left behind. #2315

Conversation

camilamacedo86 commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Changed

Why It's Better

Where it matters ( With Boxcutter runtime only )

Uh oh!

netlify bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

codecov bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

openshift-ci bot commented Nov 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

camilamacedo86 commented Nov 9, 2025 •

edited

Loading

netlify bot commented Nov 9, 2025 •

edited

Loading

codecov bot commented Nov 9, 2025 •

edited

Loading