🌱 Add internal direct bundle install support #2252

perdasilva · 2025-10-07T09:15:27Z

Description

Adds annotation driven internal bundle install for internal purposes (at least for now) to aid in the testing / validation of extension and revision status development

P.S. this is not meant for user consumption (at least not yet) and I'm avoiding making API changes to not pollute the downstream techpreview API and possibly creating confusion

Reviewer Checklist

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

netlify · 2025-10-07T09:15:32Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`2c0d54a`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/68e6840661ea3a0008f0e885
😎 Deploy Preview	https://deploy-preview-2252--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.

codecov · 2025-10-07T09:28:59Z

Codecov Report

❌ Patch coverage is 13.11475% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.32%. Comparing base (6604f2a) to head (2c0d54a).

Files with missing lines	Patch %	Lines
internal/operator-controller/resolve/bundle.go	0.00%	41 Missing ⚠️
cmd/operator-controller/main.go	40.00%	7 Missing and 2 partials ⚠️
internal/operator-controller/resolve/resolver.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2252      +/-   ##
==========================================
- Coverage   72.73%   72.32%   -0.41%     
==========================================
  Files          89       90       +1     
  Lines        8747     8800      +53     
==========================================
+ Hits         6362     6365       +3     
- Misses       1968     2016      +48     
- Partials      417      419       +2

Flag	Coverage Δ
e2e	`38.97% <13.11%> (-0.23%)`	⬇️
experimental-e2e	`45.64% <13.11%> (-0.23%)`	⬇️
unit	`57.61% <0.00%> (-0.35%)`	⬇️

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.

camilamacedo86

👍 For now it seems reasonable

/lgtm

openshift-ci · 2025-10-07T09:51:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86
Once this PR has been reviewed and has the lgtm label, please assign kevinrizza 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

joelanford · 2025-10-07T12:25:38Z

cmd/operator-controller/main.go

+			}
+
+			packageName := path.Base(ref.Name())
+			bundleVersion := bsemver.MustParse(ext.Annotations[directBundleInstallVersionAnnotation])


We shouldn't require or trust user input for the bundle version. Maybe fine for internal use, but in a user-facing feature, I think we'll have to dereference the version from the bundle image.

For now, perhaps leave a comment here to that effect?

The problem is that the version is embedded all the way down in the CSV. I.e. we'd need to pull the image and unpack it to figure out what it is. There's no guarantee that, e.g., the tag will be a valid semver. But, I agree. The ideal user-facing version of this would be to update the API, add a new source type, and just take the bundle image as input. I'll add the comment though.

Maybe the solution to this is something like: the resolver always returns an image reference, sometimes returns package and version information. After unpack, if we got nothing from the resolver, we just use what the bundle says, if we got something from the resolver we compare notes and error if there's a mismatch.

joelanford · 2025-10-07T12:27:33Z

cmd/operator-controller/main.go

 		},
 	}

+	var resolver resolve.Func = func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) {


It feels a bit messy for this to live in main.go. Maybe we should go ahead and make a composite resolver in the resolve package that can switch between the implementations?

I understand this isn’t the final solution — it’s more like one of the many intermediate steps to achieve the goal. So I’d be fine with handling it in a follow-up.
That said, it’s a really good point. 👍 +1

Yeah - the idea was to be quick and dirty. But fair call. Tomorrow I'll massage it in to something more tenable.

openshift-ci · 2025-10-08T12:51:23Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

tmshort · 2025-10-08T20:51:15Z

internal/operator-controller/resolve/bundle.go

+	// TODO: This is a temporary workaround to get the bundle from the filesystem
+	//    until the operator-registry library is updated to support reading from a
+	//    fs.FS. This will be removed once the library is updated.
+	bundlePath, err := getDirFSPath(bundleFS)


I really don't like the FS interface, as it's so limiting, and you're already needing to reflect it to get the proper path. I'd almost prefer to get rid of the FS interface, and simply use a path string.

(This is really just me whining about the FS interface is all... not asking for a change unless you agree and really want to.)

I don't disagree. But, atm, this FG is just an artifice to help us play around with the boxcutter status without having to deal with catalogs. Ultimately, I'd probably suggest moving away from FS and towards a formalized bundle interface that can surface all this kind of information without too many headaches

pedjak

it would great to add at least some unit tests and great if we can have an e2e test demonstrating the usage.

pedjak · 2025-10-10T14:03:14Z

internal/operator-controller/resolve/bundle.go

+)
+
+const (
+	directBundleInstallImageAnnotation = "olm.operatorframework.io/bundle-image"


do we want to reflect in the annotation name that it is alpha feature, so something like:

alpha.olm.operatorframework.io/bundle-image-ref

pedjak · 2025-10-10T14:08:47Z

internal/operator-controller/resolve/bundle.go

+	//    fs.FS. This will be removed once the library is updated.
+	bundlePath, err := getDirFSPath(bundleFS)
+	if err != nil {
+		panic(fmt.Errorf("expected to be able to recover bundle path from bundleFS: %v", err))


why panic instead of

return nil, nil, nil, fmt.Errorf("expected to be able to recover bundle path from bundleFS: %v", err)

pedjak · 2025-10-10T14:09:22Z

internal/operator-controller/resolve/bundle.go

+		return nil, nil, nil, err
+	}
+	if len(fbc.Bundles) != 1 {
+		return nil, nil, nil, errors.New("expected exactly one bundle")


perhaps you can provide in the error message the number of bundles we got?

pedjak · 2025-10-10T14:09:57Z

internal/operator-controller/resolve/bundle.go

+	bundle.Image = canonicalRef.String()
+	v, err := bundleutil.GetVersion(bundle)
+	if err != nil {
+		return nil, nil, nil, err


can you provide the error context here?

pedjak · 2025-10-10T14:15:01Z

internal/operator-controller/resolve/bundle.go

+	if ext.Annotations == nil || ext.Annotations[directBundleInstallImageAnnotation] == "" {
+		return nil, nil, nil, fmt.Errorf("ClusterExtension is missing required annotation %s", directBundleInstallImageAnnotation)
+	}


This check is performed even in MultiResolver, before invoking BundleResolver. Hence, we can drop it here.

pedjak · 2025-10-10T14:18:11Z

internal/operator-controller/resolve/bundle.go

+	directBundleInstallImageAnnotation = "olm.operatorframework.io/bundle-image"
+)
+
+type BundleResolver struct {


How about to name it BundleImageRefResolver? IMHO, BundleResolver is too generic.

perdasilva · 2025-10-10T14:25:47Z

it would great to add at least some unit tests and great if we can have an e2e test demonstrating the usage.

100% - the idea here though was to have something quick and dirty to aid us in playing around with the status stuff without having to deal with catalogs. There will definitely need to be tests of all kinds before this ever becomes user facing. Right now it shouldn't be on the critical path of other PRs. If it breaks, that's ok.

perdasilva requested a review from a team as a code owner October 7, 2025 09:15

openshift-ci bot requested review from joelanford and oceanc80 October 7, 2025 09:16

perdasilva force-pushed the direct-bundle-install branch from 53d52c9 to 15594cc Compare October 7, 2025 09:17

perdasilva changed the title ~~Direct bundle install~~ 🌱 Direct bundle install Oct 7, 2025

perdasilva changed the title ~~🌱 Direct bundle install~~ 🌱 Add internal direct bundle install support Oct 7, 2025

perdasilva force-pushed the direct-bundle-install branch from 15594cc to fb36b11 Compare October 7, 2025 09:30

camilamacedo86 approved these changes Oct 7, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 Oct 7, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2025

joelanford reviewed Oct 7, 2025

View reviewed changes

joelanford mentioned this pull request Oct 7, 2025

✨ Add experimental, feature-gated direct bundle install support #2255

Closed

4 tasks

perdasilva force-pushed the direct-bundle-install branch from fb36b11 to 28f3498 Compare October 8, 2025 12:51

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2025

perdasilva force-pushed the direct-bundle-install branch 2 times, most recently from fc7a5a6 to e85efd0 Compare October 8, 2025 12:55

Add internal direct bundle install method

2c0d54a

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

perdasilva force-pushed the direct-bundle-install branch from e85efd0 to 2c0d54a Compare October 8, 2025 15:32

tmshort reviewed Oct 8, 2025

View reviewed changes

pedjak requested changes Oct 10, 2025

View reviewed changes

openshift-ci bot assigned pedjak Oct 10, 2025

🌱 Add internal direct bundle install support #2252

Are you sure you want to change the base?

🌱 Add internal direct bundle install support #2252

Uh oh!

Conversation

perdasilva commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

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

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

camilamacedo86 left a comment • 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!

openshift-ci bot commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 8, 2025

Uh oh!

tmshort Oct 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

perdasilva commented Oct 7, 2025 •

edited

Loading

netlify bot commented Oct 7, 2025 •

edited

Loading

codecov bot commented Oct 7, 2025 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

tmshort Oct 8, 2025 •

edited

Loading