Turn installed package variable source into a func

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

Author: Mikalai Radchuk

internal/resolution/variablesources/bundle_deployment.go

@@ -6,7 +6,6 @@ import (
 	"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"
 )
 
@@ -37,21 +36,23 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de
 		return nil, err
 	}
 
-	processed := sets.Set[string]{}
-	for _, bundleDeployment := range bundleDeployments.Items {
-		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)
-			if err != nil {
-				return nil, err
-			}
-			variableSources = append(variableSources, ips)
-		}
+	allBundles, err := o.catalogClient.Bundles(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	installedPackages, err := MakeInstalledPackageVariables(allBundles, bundleDeployments.Items)
+	if err != nil {
+		return nil, err
 	}
 
-	return variableSources.GetVariables(ctx)
+	variables, err := variableSources.GetVariables(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, v := range installedPackages {
+		variables = append(variables, v)
+	}
+	return variables, nil
 }

internal/resolution/variablesources/installed_package.go

@@ -1,76 +1,71 @@
 package variablesources
 
 import (
-	"context"
 	"fmt"
 	"sort"
 
 	mmsemver "github.com/Masterminds/semver/v3"
-	"github.com/operator-framework/deppy/pkg/deppy"
-	"github.com/operator-framework/deppy/pkg/deppy/input"
+	"k8s.io/apimachinery/pkg/util/sets"
+
+	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
 
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
 	catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort"
-	"github.com/operator-framework/operator-controller/internal/resolution/variables"
+	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
 	"github.com/operator-framework/operator-controller/pkg/features"
 )
 
-var _ input.VariableSource = &InstalledPackageVariableSource{}
-
-type InstalledPackageVariableSource struct {
-	catalogClient BundleProvider
-	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
-	}
-
-	// find corresponding bundle for the installed content
-	resultSet := catalogfilter.Filter(allBundles, catalogfilter.WithBundleImage(r.bundleImage))
-	if len(resultSet) == 0 {
-		return nil, r.notFoundError()
-	}
-
-	// TODO: fast follow - we should check whether we are already supporting the channel attribute in the operator spec.
-	//       if so, we should take the value from spec of the operator CR in the owner ref of the bundle deployment.
-	//       If that channel is set, we need to update the filter above to filter by channel as well.
-	sort.SliceStable(resultSet, func(i, j int) bool {
-		return catalogsort.ByVersion(resultSet[i], resultSet[j])
-	})
-	installedBundle := resultSet[0]
-
-	upgradeEdges, err := r.successors(allBundles, installedBundle)
-	if err != nil {
-		return nil, err
-	}
-
-	// you can always upgrade to yourself, i.e. not upgrade
-	upgradeEdges = append(upgradeEdges, installedBundle)
-	return []deppy.Variable{
-		variables.NewInstalledPackageVariable(installedBundle.Package, upgradeEdges),
-	}, nil
-}
-
-func (r *InstalledPackageVariableSource) notFoundError() error {
-	return fmt.Errorf("bundleImage %q not found", r.bundleImage)
-}
-
-func NewInstalledPackageVariableSource(catalogClient BundleProvider, bundleImage string) (*InstalledPackageVariableSource, error) {
-	successors := legacySemanticsSuccessors
+// MakeInstalledPackageVariables returns variables representing packages
+// already installed in the system.
+// Meaning that each BundleDeployment managed by operator-controller
+// has own variable.
+func MakeInstalledPackageVariables(
+	allBundles []*catalogmetadata.Bundle,
+	bundleDeployments []rukpakv1alpha1.BundleDeployment,
+) ([]*olmvariables.InstalledPackageVariable, error) {
+	var successors successorsFunc = legacySemanticsSuccessors
 	if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) {
 		successors = semverSuccessors
 	}
 
-	return &InstalledPackageVariableSource{
-		catalogClient: catalogClient,
-		bundleImage:   bundleImage,
-		successors:    successors,
-	}, nil
+	result := make([]*olmvariables.InstalledPackageVariable, 0, len(bundleDeployments))
+	processed := sets.Set[string]{}
+	for _, bundleDeployment := range bundleDeployments {
+		sourceImage := bundleDeployment.Spec.Template.Spec.Source.Image
+		if sourceImage == nil || sourceImage.Ref == "" {
+			continue
+		}
+
+		if processed.Has(sourceImage.Ref) {
+			continue
+		}
+		processed.Insert(sourceImage.Ref)
+
+		bundleImage := sourceImage.Ref
+
+		// find corresponding bundle for the installed content
+		resultSet := catalogfilter.Filter(allBundles, catalogfilter.WithBundleImage(bundleImage))
+		if len(resultSet) == 0 {
+			return nil, fmt.Errorf("bundleImage %q not found", bundleImage)
+		}
+
+		sort.SliceStable(resultSet, func(i, j int) bool {
+			return catalogsort.ByVersion(resultSet[i], resultSet[j])
+		})
+		installedBundle := resultSet[0]
+
+		upgradeEdges, err := successors(allBundles, installedBundle)
+		if err != nil {
+			return nil, err
+		}
+
+		// you can always upgrade to yourself, i.e. not upgrade
+		upgradeEdges = append(upgradeEdges, installedBundle)
+		result = append(result, olmvariables.NewInstalledPackageVariable(installedBundle.Package, upgradeEdges))
+	}
+
+	return result, nil
 }
 
 // successorsFunc must return successors of a currently installed bundle

internal/resolution/variablesources/installed_package_test.go

@@ -1,25 +1,26 @@
 package variablesources_test
 
 import (
-	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 
-	"github.com/operator-framework/deppy/pkg/deppy"
-	"github.com/operator-framework/operator-registry/alpha/declcfg"
-	"github.com/operator-framework/operator-registry/alpha/property"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 
+	"github.com/operator-framework/deppy/pkg/deppy"
+	"github.com/operator-framework/operator-registry/alpha/declcfg"
+	"github.com/operator-framework/operator-registry/alpha/property"
+	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
+
 	"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"
 	"github.com/operator-framework/operator-controller/pkg/features"
-	testutil "github.com/operator-framework/operator-controller/test/util"
 )
 
-func TestInstalledPackageVariableSource(t *testing.T) {
+func TestMakeInstalledPackageVariables(t *testing.T) {
 	someOtherPackageChannel := catalogmetadata.Channel{Channel: declcfg.Channel{
 		Name:    "stable",
 		Package: "some-other-package",
@@ -82,7 +83,7 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 			},
 		},
 	}}
-	bundleList := []*catalogmetadata.Bundle{
+	allBundles := []*catalogmetadata.Bundle{
 		{Bundle: declcfg.Bundle{
 			Name:    "test-package.v0.0.1",
 			Package: "test-package",
@@ -202,21 +203,41 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 		},
 	}
 
-	fakeCatalogClient := testutil.NewFakeCatalogClient(bundleList)
+	fakeBundleDeployments := func(bundleImages ...string) []rukpakv1alpha1.BundleDeployment {
+		bundleDeployments := []rukpakv1alpha1.BundleDeployment{}
+		for idx, bundleImage := range bundleImages {
+			bd := rukpakv1alpha1.BundleDeployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("bd-%d", idx),
+				},
+				Spec: rukpakv1alpha1.BundleDeploymentSpec{
+					Template: &rukpakv1alpha1.BundleTemplate{
+						Spec: rukpakv1alpha1.BundleSpec{
+							Source: rukpakv1alpha1.BundleSource{
+								Image: &rukpakv1alpha1.ImageSource{
+									Ref: bundleImage,
+								},
+							},
+						},
+					},
+				},
+			}
+			bundleDeployments = append(bundleDeployments, bd)
+		}
+
+		return bundleDeployments
+	}
 
 	t.Run("with ForceSemverUpgradeConstraints feature gate enabled", func(t *testing.T) {
 		defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, true)()
 
 		t.Run("with non-zero major version", func(t *testing.T) {
 			const bundleImage = "registry.io/repo/[email protected]"
-			ipvs, err := variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
+			installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage))
 			require.NoError(t, err)
 
-			variables, err := ipvs.GetVariables(context.TODO())
-			require.NoError(t, err)
-			require.Len(t, variables, 1)
-			packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable)
-			assert.True(t, ok)
+			require.Len(t, installedPackages, 1)
+			packageVariable := installedPackages[0]
 			assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier())
 
 			// ensure bundles are in version order (high to low)
@@ -230,14 +251,11 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 		t.Run("with zero major version", func(t *testing.T) {
 			t.Run("with zero minor version", func(t *testing.T) {
 				const bundleImage = "registry.io/repo/[email protected]"
-				ipvs, err := variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
+				installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage))
 				require.NoError(t, err)
 
-				variables, err := ipvs.GetVariables(context.TODO())
-				require.NoError(t, err)
-				require.Len(t, variables, 1)
-				packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable)
-				assert.True(t, ok)
+				require.Len(t, installedPackages, 1)
+				packageVariable := installedPackages[0]
 				assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier())
 
 				// No upgrades are allowed in major version zero when minor version is also zero
@@ -248,14 +266,11 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 
 			t.Run("with non-zero minor version", func(t *testing.T) {
 				const bundleImage = "registry.io/repo/[email protected]"
-				ipvs, err := variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
+				installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage))
 				require.NoError(t, err)
 
-				variables, err := ipvs.GetVariables(context.TODO())
-				require.NoError(t, err)
-				require.Len(t, variables, 1)
-				packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable)
-				assert.True(t, ok)
+				require.Len(t, installedPackages, 1)
+				packageVariable := installedPackages[0]
 				assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier())
 
 				// Patch version upgrades are allowed, but not minor upgrades
@@ -271,14 +286,11 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 		defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, false)()
 
 		const bundleImage = "registry.io/repo/[email protected]"
-		ipvs, err := variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
+		installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage))
 		require.NoError(t, err)
 
-		variables, err := ipvs.GetVariables(context.TODO())
-		require.NoError(t, err)
-		require.Len(t, variables, 1)
-		packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable)
-		assert.True(t, ok)
+		require.Len(t, installedPackages, 1)
+		packageVariable := installedPackages[0]
 		assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier())
 
 		// ensure bundles are in version order (high to low)
@@ -287,4 +299,11 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 		assert.Equal(t, "test-package.v2.1.0", packageVariable.Bundles()[0].Name)
 		assert.Equal(t, "test-package.v2.0.0", packageVariable.Bundles()[1].Name)
 	})
+
+	t.Run("installed bundle not found", func(t *testing.T) {
+		const bundleImage = "registry.io/repo/[email protected]"
+		installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage))
+		assert.Nil(t, installedPackages)
+		assert.ErrorContains(t, err, `bundleImage "registry.io/repo/[email protected]" not found`)
+	})
 }