✨ Implement Boxcutter #1946

thetechnick · 2025-04-30T14:47:07Z

Description

Reviewer Checklist

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

netlify · 2025-04-30T14:47:25Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`9401d71`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/68791d2f254a5000081562b9
😎 Deploy Preview	https://deploy-preview-1946--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.

joelanford · 2025-04-30T16:55:24Z

api/v1/clusterextensionrevision_types.go

+type ClusterExtensionRevisionObject struct {
+	// +kubebuilder:validation:EmbeddedResource
+	// +kubebuilder:pruning:PreserveUnknownFields
+	Object unstructured.Unstructured `json:"object"`
+}


I suspect embedding fully objects in the ClusterExtensionRevision will immediately fall over for the larger registry+v1 bundles, particularly those that ship many CRDs, which tend to be large due to descriptions and schemas.

Thoughts on sharding objects out to secrets or some other resource?

joelanford · 2025-04-30T17:01:06Z

internal/operator-controller/applier/boxcutter.go

+		labels := obj.GetLabels()
+		if labels == nil {
+			labels = map[string]string{}
+		}
+		for k, v := range objectLabels {
+			labels[k] = v
+		}


nit: This could be simplified using the new-ish maps package in the standard library.

Suggested change

labels := obj.GetLabels()

if labels == nil {

labels = map[string]string{}

}

for k, v := range objectLabels {

labels[k] = v

}

labels := maps.Clone(obj.GetLabels())

maps.Copy(labels, objectLabels)

joelanford · 2025-04-30T17:03:46Z

internal/operator-controller/applier/boxcutter.go

+	hasher.Reset()
+
+	printer := spew.ConfigState{
+		Indent:         " ",
+		SortKeys:       true,
+		DisableMethods: true,
+		SpewKeys:       true,
+	}
+	if _, err := printer.Fprintf(hasher, "%#v", objectToWrite); err != nil {
+		panic(err)
+	}


Is this an OLM-ism or a boxcutter-ism? I wonder if json.NewEncoder(hasher).Encode(objectToWrite) would be better?

This is a kubernetes apimachinery thing:
https://github.com/kubernetes/kubernetes/blob/627ce4946ccd6b67bc18f5a4dda92cf6c583e9b7/pkg/util/hash/hash.go#L26-L32
https://github.com/kubernetes/apimachinery/blob/master/pkg/util/dump/dump.go

I'll import from apimachinery next.

joelanford · 2025-04-30T17:05:12Z

internal/operator-controller/applier/boxcutter.go

+type revisionAscending []ocv1.ClusterExtensionRevision
+
+func (a revisionAscending) Len() int      { return len(a) }
+func (a revisionAscending) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
+func (a revisionAscending) Less(i, j int) bool {
+	iObj := a[i]
+	jObj := a[j]
+
+	return iObj.Spec.Revision < jObj.Spec.Revision
+}


Nit: might be able to avoid the type definition and associated boilerplate with slices.SortFunc?

openshift-ci · 2025-05-07T11:57:59Z

[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 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:

OWNERS

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

codecov · 2025-07-15T12:09:46Z

Codecov Report

Attention: Patch coverage is 4.73373% with 644 lines in your changes missing coverage. Please review.

Project coverage is 67.64%. Comparing base (5f5142d) to head (9401d71).

Files with missing lines	Patch %	Lines
...controllers/clusterextensionrevision_controller.go	0.00%	290 Missing ⚠️
internal/operator-controller/applier/boxcutter.go	0.00%	139 Missing ⚠️
api/v1/zz_generated.deepcopy.go	0.00%	114 Missing ⚠️
cmd/operator-controller/main.go	34.17%	44 Missing and 8 partials ⚠️
...perator-controller/rukpak/util/testing/bundlefs.go	0.00%	42 Missing ⚠️
...troller/controllers/clusterextension_controller.go	30.00%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1946      +/-   ##
==========================================
- Coverage   73.55%   67.64%   -5.91%     
==========================================
  Files          78       82       +4     
  Lines        7260     7912     +652     
==========================================
+ Hits         5340     5352      +12     
- Misses       1568     2201     +633     
- Partials      352      359       +7

Flag	Coverage Δ
e2e	`40.22% <4.73%> (-3.50%)`	⬇️
experimental-e2e	`45.81% <4.88%> (-4.18%)`	⬇️
unit	`54.06% <0.29%> (-4.81%)`	⬇️

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.

Signed-off-by: Per Goncalves da Silva <[email protected]>

…time libs Signed-off-by: Per Goncalves da Silva <[email protected]>

Signed-off-by: Per Goncalves da Silva <[email protected]>

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 Apr 30, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2025

joelanford reviewed Apr 30, 2025

View reviewed changes

camilamacedo86 mentioned this pull request Jun 28, 2025

✨ Add support for deploying OCI helm charts in OLM v1 #1971

Merged

4 tasks

thetechnick force-pushed the poc-boxcutter branch from 36d9cd7 to b401796 Compare July 7, 2025 13:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2025

perdasilva force-pushed the poc-boxcutter branch 2 times, most recently from 1c23866 to dc66501 Compare July 15, 2025 11:12

perdasilva changed the title ~~POC: Implement Boxcutter~~ ✨ Implement Boxcutter Jul 15, 2025

thetechnick and others added 10 commits July 17, 2025 17:15

Implement Boxcutter applier, revision controller

fef0982

Connect Boxcutter Applier with ClusterExtension

f5a9e8e

Add Boxcutter featuregate

daf7fc9

Signed-off-by: Per Goncalves da Silva <[email protected]>

Update experimental manifest and code generation

4ff395a

Signed-off-by: Per Goncalves da Silva <[email protected]>

Update boxcutter library to branch with latest k8s and controller-run…

cc7d899

…time libs Signed-off-by: Per Goncalves da Silva <[email protected]>

Update manifests

609ccb1

Signed-off-by: Per Goncalves da Silva <[email protected]>

Fix up makefile

dc6c4c8

Signed-off-by: Per Goncalves da Silva <[email protected]>

Fix lint

1d768da

Signed-off-by: Per Goncalves da Silva <[email protected]>

Remove ClusterExtensionRevision from crd-docs

842be9b

Signed-off-by: Per Goncalves da Silva <[email protected]>

Fixes

7a68084

Signed-off-by: Per Goncalves da Silva <[email protected]>

perdasilva force-pushed the poc-boxcutter branch 3 times, most recently from f796c03 to 9e976a7 Compare July 17, 2025 15:38

Progress

9401d71

Signed-off-by: Per Goncalves da Silva <[email protected]>

perdasilva force-pushed the poc-boxcutter branch from 9e976a7 to 9401d71 Compare July 17, 2025 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

✨ Implement Boxcutter #1946

✨ Implement Boxcutter #1946

Uh oh!

thetechnick commented Apr 30, 2025

Uh oh!

netlify bot commented Apr 30, 2025 •

edited

Loading

Uh oh!

joelanford Apr 30, 2025

Uh oh!

joelanford Apr 30, 2025 •

edited

Loading

Uh oh!

perdasilva May 6, 2025

Uh oh!

joelanford Apr 30, 2025

Uh oh!

thetechnick May 12, 2025

Uh oh!

joelanford Apr 30, 2025

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

codecov bot commented Jul 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

✨ Implement Boxcutter #1946

Are you sure you want to change the base?

✨ Implement Boxcutter #1946

Uh oh!

Conversation

thetechnick commented Apr 30, 2025

Description

Reviewer Checklist

Uh oh!

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

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

joelanford Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

joelanford Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perdasilva May 6, 2025

Choose a reason for hiding this comment

Uh oh!

joelanford Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

thetechnick May 12, 2025

Choose a reason for hiding this comment

Uh oh!

joelanford Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

netlify bot commented Apr 30, 2025 •

edited

Loading

joelanford Apr 30, 2025 •

edited

Loading

codecov bot commented Jul 15, 2025 •

edited

Loading