🌱 Part 3: Reduce number of variable sources. Installed packages

internal/resolution/variablesources/bundle_deployment.go

@@ -3,8 +3,6 @@ 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"
@@ -34,21 +32,18 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de
 		variableSources = append(variableSources, o.inputVariableSource)
 	}
 
-	processed := sets.Set[string]{}
-	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.allBundles, bundleDeployment.Spec.Template.Spec.Source.Image.Ref)
-			if err != nil {
-				return nil, err
-			}
-			variableSources = append(variableSources, ips)
-		}
+	variables, err := variableSources.GetVariables(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	installedPackages, err := MakeInstalledPackageVariables(o.allBundles, o.bundleDeployments)
+	if err != nil {
+		return nil, err
 	}
 
-	return variableSources.GetVariables(ctx)
+	for _, v := range installedPackages {
+		variables = append(variables, v)
+	}
+	return variables, nil
 }

internal/resolution/variablesources/installed_package.go

@@ -1,71 +1,74 @@
 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 {
-	allBundles  []*catalogmetadata.Bundle
-	successors  successorsFunc
-	bundleImage string
-}
-
-func (r *InstalledPackageVariableSource) GetVariables(_ context.Context) ([]deppy.Variable, error) {
-	// find corresponding bundle for the installed content
-	resultSet := catalogfilter.Filter(r.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(r.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(allBundles []*catalogmetadata.Bundle, 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{
-		allBundles:  allBundles,
-		bundleImage: bundleImage,
-		successors:  successors,
-	}, nil
+	result := make([]*olmvariables.InstalledPackageVariable, 0, len(bundleDeployments))
+	processed := sets.Set[string]{}
+	for _, bundleDeployment := range bundleDeployments {
+		if bundleDeployment.Spec.Template == nil {
+			continue
+		}
+		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,24 +1,24 @@
 package variablesources_test
 
 import (
-	"context"
 	"encoding/json"
 	"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/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/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"
 )
 
-func TestInstalledPackageVariableSource(t *testing.T) {
+func TestMakeInstalledPackageVariables(t *testing.T) {
 	someOtherPackageChannel := catalogmetadata.Channel{Channel: declcfg.Channel{
 		Name:    "stable",
 		Package: "some-other-package",
@@ -81,7 +81,7 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 			},
 		},
 	}}
-	bundleList := []*catalogmetadata.Bundle{
+	allBundles := []*catalogmetadata.Bundle{
 		{Bundle: declcfg.Bundle{
 			Name:    "test-package.v0.0.1",
 			Package: "test-package",
@@ -201,19 +201,40 @@ func TestInstalledPackageVariableSource(t *testing.T) {
 		},
 	}
 
+	fakeBundleDeployment := func(name, bundleImage string) rukpakv1alpha1.BundleDeployment {
+		return rukpakv1alpha1.BundleDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+			Spec: rukpakv1alpha1.BundleDeploymentSpec{
+				Template: &rukpakv1alpha1.BundleTemplate{
+					Spec: rukpakv1alpha1.BundleSpec{
+						Source: rukpakv1alpha1.BundleSource{
+							Image: &rukpakv1alpha1.ImageSource{
+								Ref: bundleImage,
+							},
+						},
+					},
+				},
+			},
+		}
+	}
+
 	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(bundleList, bundleImage)
+			installedPackages, err := variablesources.MakeInstalledPackageVariables(
+				allBundles,
+				[]rukpakv1alpha1.BundleDeployment{
+					fakeBundleDeployment("test-bd", 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)
@@ -227,14 +248,16 @@ 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(bundleList, bundleImage)
+				installedPackages, err := variablesources.MakeInstalledPackageVariables(
+					allBundles,
+					[]rukpakv1alpha1.BundleDeployment{
+						fakeBundleDeployment("test-bd", 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
@@ -245,14 +268,16 @@ 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(bundleList, bundleImage)
+				installedPackages, err := variablesources.MakeInstalledPackageVariables(
+					allBundles,
+					[]rukpakv1alpha1.BundleDeployment{
+						fakeBundleDeployment("test-bd", 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
@@ -268,14 +293,16 @@ 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(bundleList, bundleImage)
+		installedPackages, err := variablesources.MakeInstalledPackageVariables(
+			allBundles,
+			[]rukpakv1alpha1.BundleDeployment{
+				fakeBundleDeployment("test-bd", 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)
@@ -284,4 +311,16 @@ 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,
+			[]rukpakv1alpha1.BundleDeployment{
+				fakeBundleDeployment("test-bd", bundleImage),
+			},
+		)
+		assert.Nil(t, installedPackages)
+		assert.ErrorContains(t, err, `bundleImage "registry.io/repo/[email protected]" not found`)
+	})
 }