From 3070b480ab71530538792ddd907aafa165bbf222 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Tue, 31 Oct 2023 15:48:26 +0000 Subject: [PATCH] Move bundle variable code into a func Signed-off-by: Mikalai Radchuk --- internal/resolution/variablesources/bundle.go | 121 ++++++----- .../resolution/variablesources/bundle_test.go | 191 +++++++++++++++++- 2 files changed, 263 insertions(+), 49 deletions(-) diff --git a/internal/resolution/variablesources/bundle.go b/internal/resolution/variablesources/bundle.go index 88eaafc94..37ec9cdca 100644 --- a/internal/resolution/variablesources/bundle.go +++ b/internal/resolution/variablesources/bundle.go @@ -5,9 +5,10 @@ import ( "fmt" "sort" + "k8s.io/apimachinery/pkg/util/sets" + "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/input" - "k8s.io/apimachinery/pkg/util/sets" "github.com/operator-framework/operator-controller/internal/catalogmetadata" catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter" @@ -15,49 +16,21 @@ import ( olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables" ) -var _ input.VariableSource = &BundlesAndDepsVariableSource{} - -type BundlesAndDepsVariableSource struct { - catalogClient BundleProvider - variableSources []input.VariableSource -} - -func NewBundlesAndDepsVariableSource(catalogClient BundleProvider, inputVariableSources ...input.VariableSource) *BundlesAndDepsVariableSource { - return &BundlesAndDepsVariableSource{ - catalogClient: catalogClient, - variableSources: inputVariableSources, +func MakeBundleVariables( + allBundles []*catalogmetadata.Bundle, + requiredPackages []*olmvariables.RequiredPackageVariable, + installedPackages []*olmvariables.InstalledPackageVariable, +) ([]*olmvariables.BundleVariable, error) { + bundleQueue := make([]*catalogmetadata.Bundle, 0, len(requiredPackages)+len(installedPackages)) + for _, variable := range requiredPackages { + bundleQueue = append(bundleQueue, variable.Bundles()...) } -} - -func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) { - var variables []deppy.Variable - - // extract required package variables - for _, variableSource := range b.variableSources { - inputVariables, err := variableSource.GetVariables(ctx) - if err != nil { - return nil, err - } - variables = append(variables, inputVariables...) - } - - // create bundle queue for dependency resolution - var bundleQueue []*catalogmetadata.Bundle - for _, variable := range variables { - switch v := variable.(type) { - case *olmvariables.RequiredPackageVariable: - bundleQueue = append(bundleQueue, v.Bundles()...) - case *olmvariables.InstalledPackageVariable: - bundleQueue = append(bundleQueue, v.Bundles()...) - } - } - - allBundles, err := b.catalogClient.Bundles(ctx) - if err != nil { - return nil, err + for _, variable := range installedPackages { + bundleQueue = append(bundleQueue, variable.Bundles()...) } // build bundle and dependency variables + result := make([]*olmvariables.BundleVariable, 0, len(requiredPackages)+len(installedPackages)) visited := sets.Set[deppy.Identifier]{} for len(bundleQueue) > 0 { // pop head of queue @@ -73,32 +46,34 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp visited.Insert(id) // get bundle dependencies - dependencies, err := b.filterBundleDependencies(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] @@ -117,3 +92,53 @@ func (b *BundlesAndDepsVariableSource) filterBundleDependencies(allBundles []*ca return dependencies, nil } + +type BundlesAndDepsVariableSource struct { + catalogClient BundleProvider + variableSources []input.VariableSource +} + +func NewBundlesAndDepsVariableSource(catalogClient BundleProvider, inputVariableSources ...input.VariableSource) *BundlesAndDepsVariableSource { + return &BundlesAndDepsVariableSource{ + catalogClient: catalogClient, + variableSources: inputVariableSources, + } +} + +func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) { + variables := []deppy.Variable{} + + for _, variableSource := range b.variableSources { + inputVariables, err := variableSource.GetVariables(ctx) + if err != nil { + return nil, err + } + variables = append(variables, inputVariables...) + } + + allBundles, err := b.catalogClient.Bundles(ctx) + if err != nil { + return nil, err + } + + requiredPackages := []*olmvariables.RequiredPackageVariable{} + installedPackages := []*olmvariables.InstalledPackageVariable{} + for _, variable := range variables { + switch v := variable.(type) { + case *olmvariables.RequiredPackageVariable: + requiredPackages = append(requiredPackages, v) + case *olmvariables.InstalledPackageVariable: + installedPackages = append(installedPackages, v) + } + } + + bundles, err := MakeBundleVariables(allBundles, requiredPackages, installedPackages) + if err != nil { + return nil, err + } + + for _, v := range bundles { + variables = append(variables, v) + } + return variables, nil +} diff --git a/internal/resolution/variablesources/bundle_test.go b/internal/resolution/variablesources/bundle_test.go index 1c71a8ada..214016797 100644 --- a/internal/resolution/variablesources/bundle_test.go +++ b/internal/resolution/variablesources/bundle_test.go @@ -4,12 +4,20 @@ import ( "context" "encoding/json" "errors" + "sync" + "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" @@ -17,6 +25,187 @@ import ( testutil "github.com/operator-framework/operator-controller/test/util" ) +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(sync.RWMutex{}), + cmp.AllowUnexported( + sync.RWMutex{}, + olmvariables.BundleVariable{}, + input.SimpleVariable{}, + constraint.DependencyConstraint{}, + catalogmetadata.Bundle{}, + ), + cmpopts.IgnoreFields( + // Do not compare a func field + catalogmetadata.PackageRequired{}, "SemverRange", + ), + } + 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 @@ -374,7 +563,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() {