Part 2: Reduce number of variable sources. Bundle variables.

internal/resolution/variablesources/bundles_and_dependencies.go → internal/resolution/variablesources/bundle.go

@@ -30,9 +30,8 @@ func NewBundlesAndDepsVariableSource(allBundles []*catalogmetadata.Bundle, input
 }
 
 func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) {
-	var variables []deppy.Variable
+	variables := []deppy.Variable{}
 
-	// extract required package variables
 	for _, variableSource := range b.variableSources {
 		inputVariables, err := variableSource.GetVariables(ctx)
 		if err != nil {
@@ -41,18 +40,43 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp
 		variables = append(variables, inputVariables...)
 	}
 
-	// create bundle queue for dependency resolution
-	var bundleQueue []*catalogmetadata.Bundle
+	requiredPackages := []*olmvariables.RequiredPackageVariable{}
+	installedPackages := []*olmvariables.InstalledPackageVariable{}
 	for _, variable := range variables {
 		switch v := variable.(type) {
 		case *olmvariables.RequiredPackageVariable:
-			bundleQueue = append(bundleQueue, v.Bundles()...)
+			requiredPackages = append(requiredPackages, v)
 		case *olmvariables.InstalledPackageVariable:
-			bundleQueue = append(bundleQueue, v.Bundles()...)
+			installedPackages = append(installedPackages, v)
 		}
 	}
 
+	bundles, err := MakeBundleVariables(b.allBundles, requiredPackages, installedPackages)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, v := range bundles {
+		variables = append(variables, v)
+	}
+	return variables, nil
+}
+
+func MakeBundleVariables(
+	allBundles []*catalogmetadata.Bundle,
+	requiredPackages []*olmvariables.RequiredPackageVariable,
+	installedPackages []*olmvariables.InstalledPackageVariable,
+) ([]*olmvariables.BundleVariable, error) {
+	var bundleQueue []*catalogmetadata.Bundle
+	for _, variable := range requiredPackages {
+		bundleQueue = append(bundleQueue, variable.Bundles()...)
+	}
+	for _, variable := range installedPackages {
+		bundleQueue = append(bundleQueue, variable.Bundles()...)
+	}
+
 	// build bundle and dependency variables
+	var result []*olmvariables.BundleVariable
 	visited := sets.Set[deppy.Identifier]{}
 	for len(bundleQueue) > 0 {
 		// pop head of queue
@@ -68,32 +92,34 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp
 		visited.Insert(id)
 
 		// get bundle dependencies
-		dependencies, err := b.filterBundleDependencies(b.allBundles, head)
+		dependencies, err := filterBundleDependencies(allBundles, head)
 		if err != nil {
-			return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err)
+			return nil, fmt.Errorf("could not determine dependencies for bundle with id %q: %w", id, err)
 		}
 
 		// add bundle dependencies to queue for processing
 		bundleQueue = append(bundleQueue, dependencies...)
 
 		// create variable
-		variables = append(variables, olmvariables.NewBundleVariable(head, dependencies))
+		result = append(result, olmvariables.NewBundleVariable(head, dependencies))
 	}
 
-	return variables, nil
+	return result, nil
 }
 
-func (b *BundlesAndDepsVariableSource) filterBundleDependencies(allBundles []*catalogmetadata.Bundle, bundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) {
+func filterBundleDependencies(allBundles []*catalogmetadata.Bundle, bundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) {
 	var dependencies []*catalogmetadata.Bundle
 	added := sets.Set[deppy.Identifier]{}
 
 	// gather required package dependencies
-	// todo(perdasilva): disambiguate between not found and actual errors
 	requiredPackages, _ := bundle.RequiredPackages()
 	for _, requiredPackage := range requiredPackages {
-		packageDependencyBundles := catalogfilter.Filter(allBundles, catalogfilter.And(catalogfilter.WithPackageName(requiredPackage.PackageName), catalogfilter.InBlangSemverRange(requiredPackage.SemverRange)))
+		packageDependencyBundles := catalogfilter.Filter(allBundles, catalogfilter.And(
+			catalogfilter.WithPackageName(requiredPackage.PackageName),
+			catalogfilter.InBlangSemverRange(requiredPackage.SemverRange),
+		))
 		if len(packageDependencyBundles) == 0 {
-			return nil, fmt.Errorf("could not find package dependencies for bundle '%s'", bundle.Name)
+			return nil, fmt.Errorf("could not find package dependencies for bundle %q", bundle.Name)
 		}
 		for i := 0; i < len(packageDependencyBundles); i++ {
 			bundle := packageDependencyBundles[i]

internal/resolution/variablesources/bundles_and_dependencies_test.go → internal/resolution/variablesources/bundle_test.go

@@ -4,18 +4,200 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/operator-framework/deppy/pkg/deppy"
+	"github.com/operator-framework/deppy/pkg/deppy/constraint"
+	"github.com/operator-framework/deppy/pkg/deppy/input"
 	"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"
 
 	"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"
 )
 
+func TestMakeBundleVariables_ValidDepedencies(t *testing.T) {
+	const fakeCatalogName = "fake-catalog"
+	fakeChannel := catalogmetadata.Channel{Channel: declcfg.Channel{Name: "stable"}}
+	bundleSet := map[string]*catalogmetadata.Bundle{
+		// Test package which we will be using as input into
+		// the testable function
+		"test-package.v1.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "test-package.v1.0.0",
+				Package: "test-package",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "1.0.0"}`)},
+					{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "first-level-dependency", "versionRange": ">=1.0.0 <2.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+
+		// First level dependency of test-package. Will be explicitly
+		// provided into the testable function as part of variable.
+		// This package must have at least one dependency with a version
+		// range so we can test that result has correct ordering:
+		// the testable function must give priority to newer versions.
+		"first-level-dependency.v1.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "first-level-dependency.v1.0.0",
+				Package: "first-level-dependency",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "first-level-dependency", "version": "1.0.0"}`)},
+					{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "second-level-dependency", "versionRange": ">=1.0.0 <2.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+
+		// Second level dependency that matches requirements of the first level dependency.
+		"second-level-dependency.v1.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "second-level-dependency.v1.0.0",
+				Package: "second-level-dependency",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "second-level-dependency", "version": "1.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+
+		// Second level dependency that matches requirements of the first level dependency.
+		"second-level-dependency.v1.0.1": {
+			Bundle: declcfg.Bundle{
+				Name:    "second-level-dependency.v1.0.1",
+				Package: "second-level-dependency",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "second-level-dependency", "version": "1.0.1"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+
+		// Second level dependency that does not match requirements of the first level dependency.
+		"second-level-dependency.v2.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "second-level-dependency.v2.0.0",
+				Package: "second-level-dependency",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "second-level-dependency", "version": "2.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+
+		// Package that is in a our fake catalog, but is not involved
+		// in this dependency chain. We need this to make sure that
+		// the testable function filters out unrelated bundles.
+		"uninvolved-package.v1.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "uninvolved-package.v1.0.0",
+				Package: "uninvolved-package",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "uninvolved-package", "version": "1.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+	}
+
+	allBundles := make([]*catalogmetadata.Bundle, 0, len(bundleSet))
+	for _, bundle := range bundleSet {
+		allBundles = append(allBundles, bundle)
+	}
+	requiredPackages := []*olmvariables.RequiredPackageVariable{
+		olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
+			bundleSet["first-level-dependency.v1.0.0"],
+		}),
+	}
+	installedPackages := []*olmvariables.InstalledPackageVariable{
+		olmvariables.NewInstalledPackageVariable("test-package", []*catalogmetadata.Bundle{
+			bundleSet["first-level-dependency.v1.0.0"],
+		}),
+	}
+
+	bundles, err := variablesources.MakeBundleVariables(allBundles, requiredPackages, installedPackages)
+	require.NoError(t, err)
+
+	// Each dependency must have a variable.
+	// Dependencies from the same package must be sorted by version
+	// with higher versions first.
+	expectedVariables := []*olmvariables.BundleVariable{
+		olmvariables.NewBundleVariable(
+			bundleSet["first-level-dependency.v1.0.0"],
+			[]*catalogmetadata.Bundle{
+				bundleSet["second-level-dependency.v1.0.1"],
+				bundleSet["second-level-dependency.v1.0.0"],
+			},
+		),
+		olmvariables.NewBundleVariable(
+			bundleSet["second-level-dependency.v1.0.1"],
+			nil,
+		),
+		olmvariables.NewBundleVariable(
+			bundleSet["second-level-dependency.v1.0.0"],
+			nil,
+		),
+	}
+	gocmpopts := []cmp.Option{
+		cmpopts.IgnoreUnexported(catalogmetadata.Bundle{}),
+		cmp.AllowUnexported(
+			olmvariables.BundleVariable{},
+			input.SimpleVariable{},
+			constraint.DependencyConstraint{},
+		),
+	}
+	require.Empty(t, cmp.Diff(bundles, expectedVariables, gocmpopts...))
+}
+
+func TestMakeBundleVariables_NonExistentDepedencies(t *testing.T) {
+	const fakeCatalogName = "fake-catalog"
+	fakeChannel := catalogmetadata.Channel{Channel: declcfg.Channel{Name: "stable"}}
+	bundleSet := map[string]*catalogmetadata.Bundle{
+		"test-package.v1.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "test-package.v1.0.0",
+				Package: "test-package",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "1.0.0"}`)},
+					{Type: property.TypePackageRequired, Value: json.RawMessage(`{"packageName": "first-level-dependency", "versionRange": ">=1.0.0 <2.0.0"}`)},
+				},
+			},
+			CatalogName: fakeCatalogName,
+			InChannels:  []*catalogmetadata.Channel{&fakeChannel},
+		},
+	}
+
+	allBundles := make([]*catalogmetadata.Bundle, 0, len(bundleSet))
+	for _, bundle := range bundleSet {
+		allBundles = append(allBundles, bundle)
+	}
+	requiredPackages := []*olmvariables.RequiredPackageVariable{
+		olmvariables.NewRequiredPackageVariable("test-package", []*catalogmetadata.Bundle{
+			bundleSet["test-package.v1.0.0"],
+		}),
+	}
+	installedPackages := []*olmvariables.InstalledPackageVariable{}
+
+	bundles, err := variablesources.MakeBundleVariables(allBundles, requiredPackages, installedPackages)
+	assert.ErrorContains(t, err, `could not determine dependencies for bundle with id "fake-catalog-test-package-test-package.v1.0.0"`)
+	assert.Nil(t, bundles)
+}
+
 var _ = Describe("BundlesAndDepsVariableSource", func() {
 	var (
 		bdvs           *variablesources.BundlesAndDepsVariableSource
@@ -368,7 +550,7 @@ var _ = Describe("BundlesAndDepsVariableSource", func() {
 		)
 		_, err := bdvs.GetVariables(context.TODO())
 		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(ContainSubstring("could not determine dependencies for bundle with id 'fake-catalog-test-package-bundle-2': could not find package dependencies for bundle 'bundle-2'"))
+		Expect(err.Error()).To(ContainSubstring(`could not determine dependencies for bundle with id "fake-catalog-test-package-bundle-2": could not find package dependencies for bundle "bundle-2"`))
 	})
 
 	It("should return error if an inner variable source returns an error", func() {