Skip to content

🌱 OPRUN-4101: Move helm converter to applier package #2207

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

Merged
merged 2 commits into from
Sep 12, 2025
Merged

🌱 OPRUN-4101: Move helm converter to applier package #2207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
191bf91
move helm converter to applier package
Sep 10, 2025
f721a07
Address reviewer comments
Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/finalizers"
"github.com/operator-framework/operator-controller/internal/operator-controller/resolve"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/certproviders"
Expand Down Expand Up @@ -655,7 +654,7 @@ func setupHelm(
ceReconciler.Applier = &applier.Helm{
ActionClientGetter: acg,
Preflights: preflights,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
HelmChartProvider: &applier.RegistryV1HelmChartProvider{
BundleRenderer: registryv1.Renderer,
CertificateProvider: certProvider,
IsWebhookSupportEnabled: certProvider != nil,
Expand Down
2 changes: 2 additions & 0 deletions internal/operator-controller/applier/boxcutter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,8 @@ func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 {
return prevRevisions[len(prevRevisions)-1].Spec.Revision
}

// TODO: in the next refactor iteration BundleRenderer and RegistryV1BundleRenderer into the RegistryV1ChartProvider

type BundleRenderer interface {
Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error)
}
Expand Down
22 changes: 7 additions & 15 deletions internal/operator-controller/applier/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
)

type BundleToHelmChartConverter interface {
ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error)
// HelmChartProvider provides helm charts from bundle sources and cluster extensions
type HelmChartProvider interface {
Get(bundle source.BundleSource, clusterExtension *ocv1.ClusterExtension) (*chart.Chart, error)
}

type HelmReleaseToObjectsConverter struct {
Expand All @@ -62,7 +62,7 @@ type Helm struct {
ActionClientGetter helmclient.ActionClientGetter
Preflights []Preflight
PreAuthorizer authorization.PreAuthorizer
BundleToHelmChartConverter BundleToHelmChartConverter
HelmChartProvider HelmChartProvider
HelmReleaseToObjectsConverter HelmReleaseToObjectsConverterInterface

Manager contentmanager.Manager
Expand Down Expand Up @@ -199,12 +199,8 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
}

func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
if h.BundleToHelmChartConverter == nil {
return nil, errors.New("BundleToHelmChartConverter is nil")
}
watchNamespace, err := GetWatchNamespace(ext)
if err != nil {
return nil, err
if h.HelmChartProvider == nil {
return nil, errors.New("HelmChartProvider is nil")
}
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
meta := new(chart.Metadata)
Expand All @@ -216,11 +212,7 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char
)
}
}

bundleConfig := map[string]interface{}{
bundle.BundleConfigWatchNamespaceKey: watchNamespace,
}
return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(bundleFS), ext.Spec.Namespace, bundleConfig)
return h.HelmChartProvider.Get(source.FromFS(bundleFS), ext)
}

func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {
Expand Down
121 changes: 34 additions & 87 deletions internal/operator-controller/applier/helm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package applier_test
import (
"context"
"errors"
"fmt"
"io"
"os"
"testing"
Expand All @@ -14,11 +13,7 @@ import (
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"sigs.k8s.io/controller-runtime/pkg/client"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
Expand All @@ -28,10 +23,7 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
cmcache "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager/cache"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
registryv1Bundle "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
)

var _ contentmanager.Manager = (*mockManagedContentCacheManager)(nil)
Expand Down Expand Up @@ -246,8 +238,8 @@ func TestApply_Base(t *testing.T) {
t.Run("fails trying to obtain an action client", func(t *testing.T) {
mockAcg := &mockActionGetter{actionClientForErr: errors.New("failed getting action client")}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}

installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -260,8 +252,8 @@ func TestApply_Base(t *testing.T) {
t.Run("fails getting current release and !driver.ErrReleaseNotFound", func(t *testing.T) {
mockAcg := &mockActionGetter{getClientErr: errors.New("failed getting current release")}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}

installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -279,8 +271,8 @@ func TestApply_Installation(t *testing.T) {
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}

installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -299,7 +291,7 @@ func TestApply_Installation(t *testing.T) {
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -317,7 +309,7 @@ func TestApply_Installation(t *testing.T) {
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -338,7 +330,7 @@ func TestApply_Installation(t *testing.T) {
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
Manager: &mockManagedContentCacheManager{
cache: &mockManagedContentCache{},
Expand All @@ -359,8 +351,8 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}

installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -384,7 +376,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
PreAuthorizer: &mockPreAuthorizer{nil, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -404,9 +396,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}
// Use a ClusterExtension with valid Spec fields.
validCE := &ocv1.ClusterExtension{
Expand All @@ -433,9 +425,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
},
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}
// Use a ClusterExtension with valid Spec fields.
validCE := &ocv1.ClusterExtension{
Expand Down Expand Up @@ -464,7 +456,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
PreAuthorizer: &mockPreAuthorizer{nil, nil},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
Manager: &mockManagedContentCacheManager{
cache: &mockManagedContentCache{},
Expand Down Expand Up @@ -498,8 +490,8 @@ func TestApply_Upgrade(t *testing.T) {
dryRunUpgradeErr: errors.New("failed attempting to dry-run upgrade chart"),
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
ActionClientGetter: mockAcg,
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
}

installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
Expand All @@ -522,7 +514,7 @@ func TestApply_Upgrade(t *testing.T) {
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -545,7 +537,7 @@ func TestApply_Upgrade(t *testing.T) {
mockPf := &mockPreflight{}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -569,7 +561,7 @@ func TestApply_Upgrade(t *testing.T) {
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
Preflights: []applier.Preflight{mockPf},
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
}

Expand All @@ -590,7 +582,7 @@ func TestApply_Upgrade(t *testing.T) {
}
helmApplier := applier.Helm{
ActionClientGetter: mockAcg,
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
Manager: &mockManagedContentCacheManager{
cache: &mockManagedContentCache{},
Expand All @@ -604,53 +596,8 @@ func TestApply_Upgrade(t *testing.T) {
})
}

func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testing.T) {
featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SingleOwnNamespaceInstallSupport, true)
t.Run("generates bundle resources using the configured watch namespace", func(t *testing.T) {
var expectedWatchNamespace = "watch-namespace"

helmApplier := applier.Helm{
ActionClientGetter: &mockActionGetter{
getClientErr: driver.ErrReleaseNotFound,
desiredRel: &release.Release{
Info: &release.Info{Status: release.StatusDeployed},
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey])
return nil, nil
},
},
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
Manager: &mockManagedContentCacheManager{
cache: &mockManagedContentCache{},
},
}

testExt := &ocv1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{
Name: "testExt",
},
Spec: ocv1.ClusterExtensionSpec{
Config: &ocv1.ClusterExtensionConfig{
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
Inline: &apiextensionsv1.JSON{
Raw: []byte(fmt.Sprintf(`{"%s":"%s"}`, registryv1Bundle.BundleConfigWatchNamespaceKey, expectedWatchNamespace)),
},
},
},
}

_, _, _ = helmApplier.Apply(context.TODO(), validFS, testExt, testObjectLabels, testStorageLabels)
})
}

func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
t.Run("generates bundle resources in AllNamespaces install mode", func(t *testing.T) {
var expectedWatchNamespace = corev1.NamespaceAll

helmApplier := applier.Helm{
ActionClientGetter: &mockActionGetter{
getClientErr: driver.ErrReleaseNotFound,
Expand All @@ -659,9 +606,9 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey])
HelmChartProvider: &fakeRegistryV1HelmChartProvider{
fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
require.Equal(t, testCE, ext)
return nil, nil
},
},
Expand All @@ -683,8 +630,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
Manifest: validManifest,
},
},
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
HelmChartProvider: &fakeRegistryV1HelmChartProvider{
fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
return nil, errors.New("some error")
},
},
Expand All @@ -698,10 +645,10 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
})
}

type fakeBundleToHelmChartConverter struct {
fn func(source.BundleSource, string, map[string]interface{}) (*chart.Chart, error)
type fakeRegistryV1HelmChartProvider struct {
fn func(source.BundleSource, *ocv1.ClusterExtension) (*chart.Chart, error)
}

func (f fakeBundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
return f.fn(bundle, installNamespace, config)
func (f fakeRegistryV1HelmChartProvider) Get(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
return f.fn(bundle, ext)
}
Loading
Loading