🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

perdasilva · 2025-05-14T16:00:28Z

Description

This PR refactors the rukpack/convert package to make it easier to configure registry+v1 conversion features like webhook and apiservice support by:

creating a canonical registry+v1 renderer that uses the right set of validators and generators into its own package
removing the PlainConverter from rukpack/convert (the registryv1.Renderer takes its place)
refactoring the RegistryV1ToHelmChart function into a fully fledged BundleToHelmChartConverter and using that in the Helm applier
in main configuring the BundleToHelmChartConverter used by the Helm Applier appropriately based on the feature flags

I decided not to add a controller-manager flag for toggling between cert providers. For now, let's rely on the FeatureGate values to do that. Once we have coalesced on default behavior we can start to think about this, I think. No need to add the complexity right now.

Reviewer Checklist

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

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

netlify · 2025-05-14T16:00:34Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`eae2c24`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6824c289b56f760008609c3b
😎 Deploy Preview	https://deploy-preview-1968--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 site configuration.

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

codecov · 2025-05-14T16:29:17Z

Codecov Report

Attention: Patch coverage is 82.69231% with 18 lines in your changes missing coverage. Please review.

Project coverage is 69.00%. Comparing base (6f3a121) to head (eae2c24).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...nternal/operator-controller/rukpak/convert/helm.go	84.61%	4 Missing and 2 partials ⚠️
...operator-controller/rukpak/util/testing/testing.go	0.00%	6 Missing ⚠️
cmd/operator-controller/main.go	66.66%	4 Missing and 1 partial ⚠️
...operator-controller/rukpak/bundle/source/source.go	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1968      +/-   ##
==========================================
- Coverage   69.01%   69.00%   -0.02%     
==========================================
  Files          77       78       +1     
  Lines        6972     6979       +7     
==========================================
+ Hits         4812     4816       +4     
- Misses       1878     1883       +5     
+ Partials      282      280       -2

Flag	Coverage Δ
e2e	`41.73% <65.30%> (-0.05%)`	⬇️
unit	`59.77% <73.07%> (-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.

camilamacedo86 · 2025-05-15T11:17:30Z

internal/operator-controller/rukpak/render/registryv1/generators/resources_test.go

@@ -12,7 +12,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"

-	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/generators"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1/generators"


👍 I think it is nice to allow more types

camilamacedo86 · 2025-05-15T11:19:48Z

cmd/operator-controller/main.go

+	var isWebhookSupportEnabled bool
+	if features.OperatorControllerFeatureGate.Enabled(features.WebhookProviderCertManager) {
+		certProvider = certproviders.CertManagerCertificateProvider{}
+		isWebhookSupportEnabled = true


👍 I see you are trying to allow more options without bringing the complexity
If we enable the Webhook then automatically we either enable WebhookProviderCertManager

camilamacedo86 · 2025-05-15T11:26:46Z

internal/operator-controller/applier/helm.go

@@ -53,13 +54,15 @@ type Preflight interface {
 	Upgrade(context.Context, *release.Release) error
 }

-type BundleToHelmChartFn func(rv1 fs.FS, installNamespace string, watchNamespace string) (*chart.Chart, error)
+type BundleToHelmChartConverter interface {
+	ToHelmChart(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error)


OK, so we are moving away to have a function to define a contract/interface to convert any BundleSource to a HelmChart.

For HelmChart, we’d skip the convert call anyway, right?
But for something like a registry/v2 source, having the interface could help.
Is that what you had in mind?

camilamacedo86 · 2025-05-15T11:27:58Z

...nal/operator-controller/rukpak/convert/testdata/combine-properties-bundle/manifests/csv.yaml

@@ -1,10 +0,0 @@
-apiVersion: operators.operatorframework.io/v1alpha1


Why did we remove the testdata samples?
Are we mocking in the tests directly?

I found it 👍

camilamacedo86 · 2025-05-15T11:29:49Z

internal/operator-controller/rukpak/bundle/source/source_test.go

+		bundlePathProperties:  &fstest.MapFile{Data: []byte(strings.Trim(propertiesYml, "\n"))},
+		bundlePathCSV:         &fstest.MapFile{Data: []byte(strings.Trim(csvYml, "\n"))},
+	}
+}


Here, I found the testdata removed 👍

camilamacedo86

Hi @perdasilva 👋

Thanks for the PR!
The bulk of the changes seem to be moving things around to better support other bundle source types — that makes sense 👍

I did notice the addition of the flag for CertManager provider and how it's combined. Would it maybe make sense to split that into a separate PR? That way we could give it more focused attention, and if anything breaks downstream, it’s easier to revert. (That said, I’m not blocking this PR because of it.)

Overall, I don’t have any objections — nothing here looks like a blocker for me, so I’m OK with moving forward.

/lgtm
/approve

openshift-ci · 2025-05-15T11:36:39Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

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

Per G. da Silva added 2 commits May 14, 2025 16:58

Refactor render package

9b29cf4

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

Remove plain converter

443874b

Signed-off-by: Per G. 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 May 14, 2025

perdasilva force-pushed the refactor-for-certproviders branch 4 times, most recently from a1bf48b to 3a3380a Compare May 14, 2025 16:13

Per G. da Silva added 2 commits May 14, 2025 17:19

RegistryV1ToHelmChart to BundleToHelmChartConverter

6606530

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

Configure bundle to helm converter based on feature flags

eae2c24

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

perdasilva force-pushed the refactor-for-certproviders branch from 3a3380a to eae2c24 Compare May 14, 2025 16:19

perdasilva changed the title ~~🌱 Webhook/convert refactor for configurable certproviders~~ 🌱 Webhook/convert refactor for configurable registry+v1 rendering behaviour May 14, 2025

perdasilva changed the title ~~🌱 Webhook/convert refactor for configurable registry+v1 rendering behaviour~~ 🌱 refactor rukpack package for configurable registry+v1 rendering behavior May 14, 2025

perdasilva marked this pull request as ready for review May 14, 2025 16:24

perdasilva requested a review from a team as a code owner May 14, 2025 16:24

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 May 14, 2025

openshift-ci bot requested review from gallettilance and gavinmbell May 14, 2025 16:24

perdasilva changed the title ~~🌱 refactor rukpack package for configurable registry+v1 rendering behavior~~ 🌱 Refactor rukpack package for configurable registry+v1 rendering behavior May 14, 2025

camilamacedo86 reviewed May 15, 2025

View reviewed changes

camilamacedo86 approved these changes May 15, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 May 15, 2025

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2025

openshift-merge-bot bot merged commit c9df915 into operator-framework:main May 15, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

Uh oh!

perdasilva commented May 14, 2025 •

edited

Loading

Uh oh!

netlify bot commented May 14, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 14, 2025 •

edited

Loading

Uh oh!

camilamacedo86 May 15, 2025

Uh oh!

camilamacedo86 May 15, 2025 •

edited

Loading

Uh oh!

camilamacedo86 May 15, 2025

Uh oh!

camilamacedo86 May 15, 2025

Uh oh!

camilamacedo86 May 15, 2025

Uh oh!

camilamacedo86 May 15, 2025

Uh oh!

camilamacedo86 left a comment •

edited

Loading

Uh oh!

openshift-ci bot commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

		@@ -1,10 +0,0 @@
		apiVersion: operators.operatorframework.io/v1alpha1

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

🌱 Refactor rukpack package for configurable registry+v1 rendering behavior #1968

Uh oh!

Conversation

perdasilva commented May 14, 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 May 14, 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 May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

camilamacedo86 May 15, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 May 15, 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!

camilamacedo86 May 15, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 May 15, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 May 15, 2025

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 May 15, 2025

Choose a reason for hiding this comment

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 May 15, 2025

Uh oh!

Uh oh!

Uh oh!

perdasilva commented May 14, 2025 •

edited

Loading

netlify bot commented May 14, 2025 •

edited

Loading

codecov bot commented May 14, 2025 •

edited

Loading

camilamacedo86 May 15, 2025 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading