Move data fetching to a single place (#508)

Signed-off-by: Mikalai Radchuk <[email protected]>

internal/controllers/variable_source.go

@@ -17,26 +17,68 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
+	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
 
+	operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
+	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	"github.com/operator-framework/operator-controller/internal/resolution/variablesources"
 )
 
-func NewVariableSource(cl client.Client, catalogClient variablesources.BundleProvider) variablesources.NestedVariableSource {
-	return variablesources.NestedVariableSource{
+// BundleProvider provides the way to retrieve a list of Bundles from a source,
+// generally from a catalog client of some kind.
+type BundleProvider interface {
+	Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
+}
+
+type VariableSource struct {
+	client        client.Client
+	catalogClient BundleProvider
+}
+
+func NewVariableSource(cl client.Client, catalogClient BundleProvider) *VariableSource {
+	return &VariableSource{
+		client:        cl,
+		catalogClient: catalogClient,
+	}
+}
+
+func (v *VariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) {
+	operatorList := operatorsv1alpha1.OperatorList{}
+	if err := v.client.List(ctx, &operatorList); err != nil {
+		return nil, err
+	}
+
+	bundleDeploymentList := rukpakv1alpha1.BundleDeploymentList{}
+	if err := v.client.List(ctx, &bundleDeploymentList); err != nil {
+		return nil, err
+	}
+
+	allBundles, err := v.catalogClient.Bundles(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	// We are in process of getting rid of extra variable sources.
+	// See this for progress: https://github.com/operator-framework/operator-controller/issues/437
+	vs := variablesources.NestedVariableSource{
 		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewOperatorVariableSource(cl, catalogClient, inputVariableSource), nil
+			return variablesources.NewOperatorVariableSource(operatorList.Items, allBundles, inputVariableSource), nil
 		},
 		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewBundleDeploymentVariableSource(cl, catalogClient, inputVariableSource), nil
+			return variablesources.NewBundleDeploymentVariableSource(bundleDeploymentList.Items, allBundles, inputVariableSource), nil
 		},
 		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewBundlesAndDepsVariableSource(catalogClient, inputVariableSource), nil
+			return variablesources.NewBundlesAndDepsVariableSource(allBundles, inputVariableSource), nil
 		},
 		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
 			return variablesources.NewCRDUniquenessConstraintsVariableSource(inputVariableSource), nil
 		},
 	}
+	return vs.GetVariables(ctx)
 }

internal/resolution/variablesources/bundle_deployment.go

@@ -3,25 +3,27 @@ package variablesources
 import (
 	"context"
 
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
 	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
-	"k8s.io/apimachinery/pkg/util/sets"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 )
 
 var _ input.VariableSource = &BundleDeploymentVariableSource{}
 
 type BundleDeploymentVariableSource struct {
-	client              client.Client
-	catalogClient       BundleProvider
+	bundleDeployments   []rukpakv1alpha1.BundleDeployment
+	allBundles          []*catalogmetadata.Bundle
 	inputVariableSource input.VariableSource
 }
 
-func NewBundleDeploymentVariableSource(cl client.Client, catalogClient BundleProvider, inputVariableSource input.VariableSource) *BundleDeploymentVariableSource {
+func NewBundleDeploymentVariableSource(bundleDeployments []rukpakv1alpha1.BundleDeployment, allBundles []*catalogmetadata.Bundle, inputVariableSource input.VariableSource) *BundleDeploymentVariableSource {
 	return &BundleDeploymentVariableSource{
-		client:              cl,
-		catalogClient:       catalogClient,
+		bundleDeployments:   bundleDeployments,
+		allBundles:          allBundles,
 		inputVariableSource: inputVariableSource,
 	}
 }
@@ -32,20 +34,15 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de
 		variableSources = append(variableSources, o.inputVariableSource)
 	}
 
-	bundleDeployments := rukpakv1alpha1.BundleDeploymentList{}
-	if err := o.client.List(ctx, &bundleDeployments); err != nil {
-		return nil, err
-	}
-
 	processed := sets.Set[string]{}
-	for _, bundleDeployment := range bundleDeployments.Items {
+	for _, bundleDeployment := range o.bundleDeployments {
 		sourceImage := bundleDeployment.Spec.Template.Spec.Source.Image
 		if sourceImage != nil && sourceImage.Ref != "" {
 			if processed.Has(sourceImage.Ref) {
 				continue
 			}
 			processed.Insert(sourceImage.Ref)
-			ips, err := NewInstalledPackageVariableSource(o.catalogClient, bundleDeployment.Spec.Template.Spec.Source.Image.Ref)
+			ips, err := NewInstalledPackageVariableSource(o.allBundles, bundleDeployment.Spec.Template.Spec.Source.Image.Ref)
 			if err != nil {
 				return nil, err
 			}

internal/resolution/variablesources/bundle_deployment_test.go

@@ -13,26 +13,15 @@ import (
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
 	"github.com/operator-framework/operator-controller/internal/resolution/variablesources"
-	testutil "github.com/operator-framework/operator-controller/test/util"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/operator-framework/deppy/pkg/deppy"
 	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
 )
 
-func BundleDeploymentFakeClient(objects ...client.Object) client.Client {
-	scheme := runtime.NewScheme()
-	utilruntime.Must(rukpakv1alpha1.AddToScheme(scheme))
-	return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
-}
-
-func bundleDeployment(name, image string) *rukpakv1alpha1.BundleDeployment {
-	return &rukpakv1alpha1.BundleDeployment{
+func bundleDeployment(name, image string) rukpakv1alpha1.BundleDeployment {
+	return rukpakv1alpha1.BundleDeployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
 		},
@@ -53,7 +42,6 @@ func bundleDeployment(name, image string) *rukpakv1alpha1.BundleDeployment {
 }
 
 var _ = Describe("BundleDeploymentVariableSource", func() {
-	var fakeCatalogClient testutil.FakeCatalogClient
 	var betaChannel catalogmetadata.Channel
 	var stableChannel catalogmetadata.Channel
 	var testBundleList []*catalogmetadata.Bundle
@@ -111,14 +99,14 @@ var _ = Describe("BundleDeploymentVariableSource", func() {
 				},
 			}, InChannels: []*catalogmetadata.Channel{&stableChannel}},
 		}
-
-		fakeCatalogClient = testutil.NewFakeCatalogClient(testBundleList)
 	})
 
 	It("should produce RequiredPackage variables", func() {
-		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"))
+		bundleDeployments := []rukpakv1alpha1.BundleDeployment{
+			bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"),
+		}
 
-		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockRequiredPackageSource{})
+		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(bundleDeployments, testBundleList, &MockRequiredPackageSource{})
 		variables, err := bdVariableSource.GetVariables(context.Background())
 		Expect(err).ToNot(HaveOccurred())
 
@@ -137,9 +125,11 @@ var _ = Describe("BundleDeploymentVariableSource", func() {
 		})))
 	})
 	It("should return an error if the bundleDeployment image doesn't match any operator resource", func() {
-		cl := BundleDeploymentFakeClient(bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:nonexistent"))
+		bundleDeployments := []rukpakv1alpha1.BundleDeployment{
+			bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:nonexistent"),
+		}
 
-		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(cl, &fakeCatalogClient, &MockRequiredPackageSource{})
+		bdVariableSource := variablesources.NewBundleDeploymentVariableSource(bundleDeployments, testBundleList, &MockRequiredPackageSource{})
 		_, err := bdVariableSource.GetVariables(context.Background())
 		Expect(err.Error()).To(Equal("bundleImage \"quay.io/operatorhubio/prometheus@sha256:nonexistent\" not found"))
 	})

internal/resolution/variablesources/bundles_and_dependencies.go

@@ -18,13 +18,13 @@ import (
 var _ input.VariableSource = &BundlesAndDepsVariableSource{}
 
 type BundlesAndDepsVariableSource struct {
-	catalogClient   BundleProvider
+	allBundles      []*catalogmetadata.Bundle
 	variableSources []input.VariableSource
 }
 
-func NewBundlesAndDepsVariableSource(catalogClient BundleProvider, inputVariableSources ...input.VariableSource) *BundlesAndDepsVariableSource {
+func NewBundlesAndDepsVariableSource(allBundles []*catalogmetadata.Bundle, inputVariableSources ...input.VariableSource) *BundlesAndDepsVariableSource {
 	return &BundlesAndDepsVariableSource{
-		catalogClient:   catalogClient,
+		allBundles:      allBundles,
 		variableSources: inputVariableSources,
 	}
 }
@@ -52,11 +52,6 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp
 		}
 	}
 
-	allBundles, err := b.catalogClient.Bundles(ctx)
-	if err != nil {
-		return nil, err
-	}
-
 	// build bundle and dependency variables
 	visited := sets.Set[deppy.Identifier]{}
 	for len(bundleQueue) > 0 {
@@ -73,7 +68,7 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp
 		visited.Insert(id)
 
 		// get bundle dependencies
-		dependencies, err := b.filterBundleDependencies(allBundles, head)
+		dependencies, err := b.filterBundleDependencies(b.allBundles, head)
 		if err != nil {
 			return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err)
 		}

internal/resolution/variablesources/bundles_and_dependencies_test.go

@@ -14,14 +14,12 @@ import (
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
 	"github.com/operator-framework/operator-controller/internal/resolution/variablesources"
-	testutil "github.com/operator-framework/operator-controller/test/util"
 )
 
 var _ = Describe("BundlesAndDepsVariableSource", func() {
 	var (
-		bdvs              *variablesources.BundlesAndDepsVariableSource
-		testBundleList    []*catalogmetadata.Bundle
-		fakeCatalogClient testutil.FakeCatalogClient
+		bdvs           *variablesources.BundlesAndDepsVariableSource
+		testBundleList []*catalogmetadata.Bundle
 	)
 
 	BeforeEach(func() {
@@ -222,12 +220,11 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 				InChannels: []*catalogmetadata.Channel{&channel},
 			},
 		}
-		fakeCatalogClient = testutil.NewFakeCatalogClient(testBundleList)
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
-			&fakeCatalogClient,
+			testBundleList,
 			&MockRequiredPackageSource{
 				ResultSet: []deppy.Variable{
-					// must match data in fakeCatalogClient
+					// must match data in testBundleList
 					olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
 						{
 							CatalogName: "fake-catalog",
@@ -259,7 +256,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 			},
 			&MockRequiredPackageSource{
 				ResultSet: []deppy.Variable{
-					// must match data in fakeCatalogClient
+					// must match data in testBundleList
 					olmvariables.NewRequiredPackageVariable("test-package-2", []*catalogmetadata.Bundle{
 						// test-package-2 required package - no dependencies
 						{
@@ -335,13 +332,10 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 	})
 
 	It("should return error if dependencies not found", func() {
-		emptyCatalogClient := testutil.NewFakeCatalogClient(make([]*catalogmetadata.Bundle, 0))
-
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
-			&emptyCatalogClient,
+			[]*catalogmetadata.Bundle{},
 			&MockRequiredPackageSource{
 				ResultSet: []deppy.Variable{
-					// must match data in fakeCatalogClient
 					olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
 						{
 							CatalogName: "fake-catalog",
@@ -379,7 +373,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 
 	It("should return error if an inner variable source returns an error", func() {
 		bdvs = variablesources.NewBundlesAndDepsVariableSource(
-			&fakeCatalogClient,
+			testBundleList,
 			&MockRequiredPackageSource{Error: errors.New("fake error")},
 		)
 		_, err := bdvs.GetVariables(context.TODO())

internal/resolution/variablesources/installed_package.go

@@ -19,19 +19,14 @@ import (
 var _ input.VariableSource = &InstalledPackageVariableSource{}
 
 type InstalledPackageVariableSource struct {
-	catalogClient BundleProvider
-	successors    successorsFunc
-	bundleImage   string
+	allBundles  []*catalogmetadata.Bundle
+	successors  successorsFunc
+	bundleImage string
 }
 
-func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) {
-	allBundles, err := r.catalogClient.Bundles(ctx)
-	if err != nil {
-		return nil, err
-	}
-
+func (r *InstalledPackageVariableSource) GetVariables(_ context.Context) ([]deppy.Variable, error) {
 	// find corresponding bundle for the installed content
-	resultSet := catalogfilter.Filter(allBundles, catalogfilter.WithBundleImage(r.bundleImage))
+	resultSet := catalogfilter.Filter(r.allBundles, catalogfilter.WithBundleImage(r.bundleImage))
 	if len(resultSet) == 0 {
 		return nil, r.notFoundError()
 	}
@@ -44,7 +39,7 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de
 	})
 	installedBundle := resultSet[0]
 
-	upgradeEdges, err := r.successors(allBundles, installedBundle)
+	upgradeEdges, err := r.successors(r.allBundles, installedBundle)
 	if err != nil {
 		return nil, err
 	}
@@ -60,16 +55,16 @@ func (r *InstalledPackageVariableSource) notFoundError() error {
 	return fmt.Errorf("bundleImage %q not found", r.bundleImage)
 }
 
-func NewInstalledPackageVariableSource(catalogClient BundleProvider, bundleImage string) (*InstalledPackageVariableSource, error) {
+func NewInstalledPackageVariableSource(allBundles []*catalogmetadata.Bundle, bundleImage string) (*InstalledPackageVariableSource, error) {
 	successors := legacySemanticsSuccessors
 	if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) {
 		successors = semverSuccessors
 	}
 
 	return &InstalledPackageVariableSource{
-		catalogClient: catalogClient,
-		bundleImage:   bundleImage,
-		successors:    successors,
+		allBundles:  allBundles,
+		bundleImage: bundleImage,
+		successors:  successors,
 	}, nil
 }