Skip to content

Commit ed3bdcf

Browse files
perdasilvaPer Goncalves da Silva
perdasilva
and
Per Goncalves da Silva
authored
🌱 OPRUN-4101: Move helm converter to applier package (#2207)
* move helm converter to applier package Signed-off-by: Per Goncalves da Silva <[email protected]> * Address reviewer comments Signed-off-by: Per Goncalves da Silva <[email protected]> --------- Signed-off-by: Per Goncalves da Silva <[email protected]> Co-authored-by: Per Goncalves da Silva <[email protected]>
1 parent d0c7c0c commit ed3bdcf

File tree

7 files changed

+336
-331
lines changed

7 files changed

+336
-331
lines changed

‎cmd/operator-controller/main.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ import (
7171
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
7272
"github.com/operator-framework/operator-controller/internal/operator-controller/finalizers"
7373
"github.com/operator-framework/operator-controller/internal/operator-controller/resolve"
74-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
7574
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
7675
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
7776
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/certproviders"
@@ -655,7 +654,7 @@ func setupHelm(
655654
ceReconciler.Applier = &applier.Helm{
656655
ActionClientGetter: acg,
657656
Preflights: preflights,
658-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{
657+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{
659658
BundleRenderer: registryv1.Renderer,
660659
CertificateProvider: certProvider,
661660
IsWebhookSupportEnabled: certProvider != nil,

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,8 @@ func latestRevisionNumber(prevRevisions []ocv1.ClusterExtensionRevision) int64 {
389389
return prevRevisions[len(prevRevisions)-1].Spec.Revision
390390
}
391391

392+
// TODO: in the next refactor iteration BundleRenderer and RegistryV1BundleRenderer into the RegistryV1ChartProvider
393+
392394
type BundleRenderer interface {
393395
Render(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error)
394396
}

‎internal/operator-controller/applier/helm.go‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,14 @@ import (
2929
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
3030
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
3131
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
32-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
3332
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
3433
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
3534
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
3635
)
3736

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

4242
type HelmReleaseToObjectsConverter struct {
@@ -62,7 +62,7 @@ type Helm struct {
6262
ActionClientGetter helmclient.ActionClientGetter
6363
Preflights []Preflight
6464
PreAuthorizer authorization.PreAuthorizer
65-
BundleToHelmChartConverter BundleToHelmChartConverter
65+
HelmChartProvider HelmChartProvider
6666
HelmReleaseToObjectsConverter HelmReleaseToObjectsConverterInterface
6767

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

201201
func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
202-
if h.BundleToHelmChartConverter == nil {
203-
return nil, errors.New("BundleToHelmChartConverter is nil")
204-
}
205-
watchNamespace, err := GetWatchNamespace(ext)
206-
if err != nil {
207-
return nil, err
202+
if h.HelmChartProvider == nil {
203+
return nil, errors.New("HelmChartProvider is nil")
208204
}
209205
if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) {
210206
meta := new(chart.Metadata)
@@ -216,11 +212,7 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char
216212
)
217213
}
218214
}
219-
220-
bundleConfig := map[string]interface{}{
221-
bundle.BundleConfigWatchNamespaceKey: watchNamespace,
222-
}
223-
return h.BundleToHelmChartConverter.ToHelmChart(source.FromFS(bundleFS), ext.Spec.Namespace, bundleConfig)
215+
return h.HelmChartProvider.Get(source.FromFS(bundleFS), ext)
224216
}
225217

226218
func (h *Helm) renderClientOnlyRelease(ctx context.Context, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, error) {

‎internal/operator-controller/applier/helm_test.go‎

Lines changed: 34 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package applier_test
33
import (
44
"context"
55
"errors"
6-
"fmt"
76
"io"
87
"os"
98
"testing"
@@ -14,11 +13,7 @@ import (
1413
"helm.sh/helm/v3/pkg/chart"
1514
"helm.sh/helm/v3/pkg/release"
1615
"helm.sh/helm/v3/pkg/storage/driver"
17-
corev1 "k8s.io/api/core/v1"
1816
rbacv1 "k8s.io/api/rbac/v1"
19-
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
20-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
21-
featuregatetesting "k8s.io/component-base/featuregate/testing"
2217
"sigs.k8s.io/controller-runtime/pkg/client"
2318

2419
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
@@ -28,10 +23,7 @@ import (
2823
"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
2924
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
3025
cmcache "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager/cache"
31-
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
32-
registryv1Bundle "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
3326
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
34-
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
3527
)
3628

3729
var _ contentmanager.Manager = (*mockManagedContentCacheManager)(nil)
@@ -246,8 +238,8 @@ func TestApply_Base(t *testing.T) {
246238
t.Run("fails trying to obtain an action client", func(t *testing.T) {
247239
mockAcg := &mockActionGetter{actionClientForErr: errors.New("failed getting action client")}
248240
helmApplier := applier.Helm{
249-
ActionClientGetter: mockAcg,
250-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
241+
ActionClientGetter: mockAcg,
242+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
251243
}
252244

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

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

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

@@ -317,7 +309,7 @@ func TestApply_Installation(t *testing.T) {
317309
}
318310
helmApplier := applier.Helm{
319311
ActionClientGetter: mockAcg,
320-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
312+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
321313
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
322314
}
323315

@@ -338,7 +330,7 @@ func TestApply_Installation(t *testing.T) {
338330
}
339331
helmApplier := applier.Helm{
340332
ActionClientGetter: mockAcg,
341-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
333+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
342334
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
343335
Manager: &mockManagedContentCacheManager{
344336
cache: &mockManagedContentCache{},
@@ -359,8 +351,8 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
359351
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
360352
}
361353
helmApplier := applier.Helm{
362-
ActionClientGetter: mockAcg,
363-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
354+
ActionClientGetter: mockAcg,
355+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
364356
}
365357

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

@@ -404,9 +396,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
404396
},
405397
}
406398
helmApplier := applier.Helm{
407-
ActionClientGetter: mockAcg,
408-
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
409-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
399+
ActionClientGetter: mockAcg,
400+
PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth},
401+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
410402
}
411403
// Use a ClusterExtension with valid Spec fields.
412404
validCE := &ocv1.ClusterExtension{
@@ -433,9 +425,9 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
433425
},
434426
}
435427
helmApplier := applier.Helm{
436-
ActionClientGetter: mockAcg,
437-
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
438-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
428+
ActionClientGetter: mockAcg,
429+
PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil},
430+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
439431
}
440432
// Use a ClusterExtension with valid Spec fields.
441433
validCE := &ocv1.ClusterExtension{
@@ -464,7 +456,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
464456
helmApplier := applier.Helm{
465457
ActionClientGetter: mockAcg,
466458
PreAuthorizer: &mockPreAuthorizer{nil, nil},
467-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
459+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
468460
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
469461
Manager: &mockManagedContentCacheManager{
470462
cache: &mockManagedContentCache{},
@@ -498,8 +490,8 @@ func TestApply_Upgrade(t *testing.T) {
498490
dryRunUpgradeErr: errors.New("failed attempting to dry-run upgrade chart"),
499491
}
500492
helmApplier := applier.Helm{
501-
ActionClientGetter: mockAcg,
502-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
493+
ActionClientGetter: mockAcg,
494+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
503495
}
504496

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

@@ -545,7 +537,7 @@ func TestApply_Upgrade(t *testing.T) {
545537
mockPf := &mockPreflight{}
546538
helmApplier := applier.Helm{
547539
ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf},
548-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
540+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
549541
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
550542
}
551543

@@ -569,7 +561,7 @@ func TestApply_Upgrade(t *testing.T) {
569561
helmApplier := applier.Helm{
570562
ActionClientGetter: mockAcg,
571563
Preflights: []applier.Preflight{mockPf},
572-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
564+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
573565
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
574566
}
575567

@@ -590,7 +582,7 @@ func TestApply_Upgrade(t *testing.T) {
590582
}
591583
helmApplier := applier.Helm{
592584
ActionClientGetter: mockAcg,
593-
BundleToHelmChartConverter: &convert.BundleToHelmChartConverter{},
585+
HelmChartProvider: &applier.RegistryV1HelmChartProvider{},
594586
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
595587
Manager: &mockManagedContentCacheManager{
596588
cache: &mockManagedContentCache{},
@@ -604,53 +596,8 @@ func TestApply_Upgrade(t *testing.T) {
604596
})
605597
}
606598

607-
func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testing.T) {
608-
featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SingleOwnNamespaceInstallSupport, true)
609-
t.Run("generates bundle resources using the configured watch namespace", func(t *testing.T) {
610-
var expectedWatchNamespace = "watch-namespace"
611-
612-
helmApplier := applier.Helm{
613-
ActionClientGetter: &mockActionGetter{
614-
getClientErr: driver.ErrReleaseNotFound,
615-
desiredRel: &release.Release{
616-
Info: &release.Info{Status: release.StatusDeployed},
617-
Manifest: validManifest,
618-
},
619-
},
620-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
621-
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
622-
require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey])
623-
return nil, nil
624-
},
625-
},
626-
HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{},
627-
Manager: &mockManagedContentCacheManager{
628-
cache: &mockManagedContentCache{},
629-
},
630-
}
631-
632-
testExt := &ocv1.ClusterExtension{
633-
ObjectMeta: metav1.ObjectMeta{
634-
Name: "testExt",
635-
},
636-
Spec: ocv1.ClusterExtensionSpec{
637-
Config: &ocv1.ClusterExtensionConfig{
638-
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
639-
Inline: &apiextensionsv1.JSON{
640-
Raw: []byte(fmt.Sprintf(`{"%s":"%s"}`, registryv1Bundle.BundleConfigWatchNamespaceKey, expectedWatchNamespace)),
641-
},
642-
},
643-
},
644-
}
645-
646-
_, _, _ = helmApplier.Apply(context.TODO(), validFS, testExt, testObjectLabels, testStorageLabels)
647-
})
648-
}
649-
650599
func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
651600
t.Run("generates bundle resources in AllNamespaces install mode", func(t *testing.T) {
652-
var expectedWatchNamespace = corev1.NamespaceAll
653-
654601
helmApplier := applier.Helm{
655602
ActionClientGetter: &mockActionGetter{
656603
getClientErr: driver.ErrReleaseNotFound,
@@ -659,9 +606,9 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
659606
Manifest: validManifest,
660607
},
661608
},
662-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
663-
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
664-
require.Equal(t, expectedWatchNamespace, config[registryv1Bundle.BundleConfigWatchNamespaceKey])
609+
HelmChartProvider: &fakeRegistryV1HelmChartProvider{
610+
fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
611+
require.Equal(t, testCE, ext)
665612
return nil, nil
666613
},
667614
},
@@ -683,8 +630,8 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
683630
Manifest: validManifest,
684631
},
685632
},
686-
BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{
687-
fn: func(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
633+
HelmChartProvider: &fakeRegistryV1HelmChartProvider{
634+
fn: func(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
688635
return nil, errors.New("some error")
689636
},
690637
},
@@ -698,10 +645,10 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
698645
})
699646
}
700647

701-
type fakeBundleToHelmChartConverter struct {
702-
fn func(source.BundleSource, string, map[string]interface{}) (*chart.Chart, error)
648+
type fakeRegistryV1HelmChartProvider struct {
649+
fn func(source.BundleSource, *ocv1.ClusterExtension) (*chart.Chart, error)
703650
}
704651

705-
func (f fakeBundleToHelmChartConverter) ToHelmChart(bundle source.BundleSource, installNamespace string, config map[string]interface{}) (*chart.Chart, error) {
706-
return f.fn(bundle, installNamespace, config)
652+
func (f fakeRegistryV1HelmChartProvider) Get(bundle source.BundleSource, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
653+
return f.fn(bundle, ext)
707654
}

0 commit comments

Comments
 (0)