Skip to content

🌱 Simplify Boxcutter applier interface #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
openshift-merge-bot merged 1 commit into from
Jan 14, 2026
Merged

🌱 Simplify Boxcutter applier interface #2446

openshift-merge-bot merged 1 commit into from
Jan 14, 2026
+18 −52

Conversation

@pedjak
Copy link
Contributor

@pedjak pedjak commented Jan 13, 2026 •
edited
Loading

Description

  • Change Apply() to return only error instead of (bool, string, error), removing status interpretation logic from the applier. ClusterExtensionRevision conditions are already mirrored to ClusterExtension.
  • Change ApplyBundleWithBoxcutter to accept a function instead of an interface, making unit tests simpler by allowing inline function mocks

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ci 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 Jan 13, 2026
@netlify
Copy link

netlify bot commented Jan 13, 2026 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 484448a
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/6967794f88197c000825fda9
😎 Deploy Preview https://deploy-preview-2446--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@pedjak pedjak force-pushed the simplify-boxcutter-applier branch 3 times, most recently from aff9804 to f9d8598 Compare January 14, 2026 10:23
@codecov
Copy link

codecov bot commented Jan 14, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.48%. Comparing base (347be32) to head (484448a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/operator-controller/applier/boxcutter.go 30.00% 7 Missing ⚠️
cmd/operator-controller/main.go 0.00% 1 Missing ⚠️
...ontroller/controllers/boxcutter_reconcile_steps.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2446      +/-   ##
==========================================
+ Coverage   69.38%   69.48%   +0.09%     
==========================================
  Files         101      101              
  Lines        7719     7701      -18     
==========================================
- Hits         5356     5351       -5     
+ Misses       1925     1914      -11     
+ Partials      438      436       -2
Flag Coverage Δ
e2e 46.46% <0.00%> (+0.12%) ⬆️
experimental-e2e 13.56% <0.00%> (+0.03%) ⬆️
unit 57.08% <30.76%> (+0.06%) ⬆️

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.

@pedjak @claude
Simplify Boxcutter applier interface
484448a 
- Change Apply() to return only error instead of (bool, string, error),
  removing status interpretation logic from the applier. ClusterExtensionRevision conditions
  are already mirrored to ClusterExtension.
- Change ApplyBundleWithBoxcutter to accept a function instead of an
  interface, making unit tests simpler by allowing inline function mocks

Co-Authored-By: Claude <noreply@anthropic.com>
@pedjak pedjak force-pushed the simplify-boxcutter-applier branch from f9d8598 to 484448a Compare January 14, 2026 11:09
@pedjak pedjak changed the title 🌱 Simplify Boxcutter applier by removing status interpretation logic 🌱 Simplify Boxcutter applier interface Jan 14, 2026
@pedjak pedjak removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2026
@pedjak pedjak marked this pull request as ready for review January 14, 2026 11:11
camilamacedo86
camilamacedo86 reviewed Jan 14, 2026
View reviewed changes
internal/operator-controller/applier/boxcutter_test.go
} else {
require.NoError(t, err)
assert.False(t, installSucceeded)
assert.Equal(t, "New revision created", installStatus)
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we ensure that we have the same status as before?
Is not the goal only refactory ( as described in the pr title? ) Should we change the behaviour ?

Copy link
Contributor Author

@pedjak pedjak Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already mirror statuses from active cluster revisions into ClusterExtension status. As you can see, e2e tests are passing without any change.

This PR is not just refactoring, it simplifies BoxCutter apply logic:

  • we do not try to read CER conditions and based on that report to the caller if CER installation was successful or not
  • instead, ClusterExtension reconciliation is triggered on CER update, and ClusterExtension controller is going to detect if the active revision is installed and mirror fields under .status accordingly.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2026
@openshift-ci
Copy link

openshift-ci bot commented Jan 14, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2026
@pedjak
Copy link
Contributor Author

pedjak commented Jan 14, 2026

/override codecov/patch

@openshift-ci
Copy link

openshift-ci bot commented Jan 14, 2026

@pedjak: Overrode contexts on behalf of pedjak: codecov/patch

Details

In response to this:

/override codecov/patch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit dc20dfb into operator-framework:main Jan 14, 2026
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

@tmshort tmshort Awaiting requested review from tmshort

Assignees

@camilamacedo86 camilamacedo86

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pedjak @camilamacedo86