From 8ecced71e24fcd54e91c448fa4da02c0ada93292 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 | 54 +++-- .../resolution/variablesources/bundle_test.go | 184 +++++++++++++++++- 2 files changed, 223 insertions(+), 15 deletions(-) diff --git a/internal/resolution/variablesources/bundle.go b/internal/resolution/variablesources/bundle.go index 0288537a9..eebc308d9 100644 --- a/internal/resolution/variablesources/bundle.go +++ b/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] diff --git a/internal/resolution/variablesources/bundle_test.go b/internal/resolution/variablesources/bundle_test.go index b3d4a6c5a..32da44fbd 100644 --- a/internal/resolution/variablesources/bundle_test.go +++ b/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() {