From c24fdbcdbec8ee5832633ffe45d0faadb2d2edb2 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk <509198+m1kola@users.noreply.github.com> Date: Thu, 12 Oct 2023 09:22:22 +0100 Subject: [PATCH] Reduce variable count (#453) After this commit we will be creating one bundle per package. Previously we we creating one variable per bundle occurance in a channel which is currently unnecessary. Signed-off-by: Mikalai Radchuk --- internal/resolution/variables/bundle.go | 25 ++++------- internal/resolution/variables/bundle_test.go | 6 +-- .../resolution/variables/installed_package.go | 4 +- .../resolution/variables/required_package.go | 4 +- .../bundles_and_dependencies.go | 41 +++++++++---------- .../bundles_and_dependencies_test.go | 16 ++++---- .../variablesources/crd_constraints.go | 13 +++--- .../variablesources/crd_constraints_test.go | 26 ++++++------ 8 files changed, 61 insertions(+), 74 deletions(-) diff --git a/internal/resolution/variables/bundle.go b/internal/resolution/variables/bundle.go index 7b0568ffe..764c1454c 100644 --- a/internal/resolution/variables/bundle.go +++ b/internal/resolution/variables/bundle.go @@ -26,17 +26,17 @@ func (b *BundleVariable) Dependencies() []*catalogmetadata.Bundle { return b.dependencies } -func NewBundleVariable(id deppy.Identifier, bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable { - var dependencyIDs []deppy.Identifier +func NewBundleVariable(bundle *catalogmetadata.Bundle, dependencies []*catalogmetadata.Bundle) *BundleVariable { + dependencyIDs := make([]deppy.Identifier, 0, len(dependencies)) for _, bundle := range dependencies { - dependencyIDs = append(dependencyIDs, BundleToBundleVariableIDs(bundle)...) + dependencyIDs = append(dependencyIDs, BundleVariableID(bundle)) } var constraints []deppy.Constraint if len(dependencyIDs) > 0 { constraints = append(constraints, constraint.Dependency(dependencyIDs...)) } return &BundleVariable{ - SimpleVariable: input.NewSimpleVariable(id, constraints...), + SimpleVariable: input.NewSimpleVariable(BundleVariableID(bundle), constraints...), bundle: bundle, dependencies: dependencies, } @@ -59,16 +59,9 @@ func NewBundleUniquenessVariable(id deppy.Identifier, atMostIDs ...deppy.Identif } } -// BundleToBundleVariableIDs returns a list of all possible IDs for a bundle. -// A bundle can be present in multiple channels and we need a separate variable -// with a unique ID for each occurrence. -// Result must be deterministic. -func BundleToBundleVariableIDs(bundle *catalogmetadata.Bundle) []deppy.Identifier { - out := make([]deppy.Identifier, 0, len(bundle.InChannels)) - for _, ch := range bundle.InChannels { - out = append(out, deppy.Identifier( - fmt.Sprintf("%s-%s-%s-%s", bundle.CatalogName, bundle.Package, ch.Name, bundle.Name), - )) - } - return out +// BundleVariableID returns an ID for a given bundle. +func BundleVariableID(bundle *catalogmetadata.Bundle) deppy.Identifier { + return deppy.Identifier( + fmt.Sprintf("%s-%s-%s", bundle.CatalogName, bundle.Package, bundle.Name), + ) } diff --git a/internal/resolution/variables/bundle_test.go b/internal/resolution/variables/bundle_test.go index da29e3742..26d831f54 100644 --- a/internal/resolution/variables/bundle_test.go +++ b/internal/resolution/variables/bundle_test.go @@ -40,11 +40,7 @@ func TestBundleVariable(t *testing.T) { }}, }, } - ids := olmvariables.BundleToBundleVariableIDs(bundle) - if len(ids) != len(bundle.InChannels) { - t.Fatalf("bundle should produce one variable ID per channel; received: %d", len(bundle.InChannels)) - } - bv := olmvariables.NewBundleVariable(ids[0], bundle, dependencies) + bv := olmvariables.NewBundleVariable(bundle, dependencies) if bv.Bundle() != bundle { t.Errorf("bundle '%v' does not match expected '%v'", bv.Bundle(), bundle) diff --git a/internal/resolution/variables/installed_package.go b/internal/resolution/variables/installed_package.go index 35bd87a01..f5ccd12cd 100644 --- a/internal/resolution/variables/installed_package.go +++ b/internal/resolution/variables/installed_package.go @@ -23,9 +23,9 @@ func (r *InstalledPackageVariable) Bundles() []*catalogmetadata.Bundle { func NewInstalledPackageVariable(packageName string, bundles []*catalogmetadata.Bundle) *InstalledPackageVariable { id := deppy.IdentifierFromString(fmt.Sprintf("installed package %s", packageName)) - var variableIDs []deppy.Identifier + variableIDs := make([]deppy.Identifier, 0, len(bundles)) for _, bundle := range bundles { - variableIDs = append(variableIDs, BundleToBundleVariableIDs(bundle)...) + variableIDs = append(variableIDs, BundleVariableID(bundle)) } return &InstalledPackageVariable{ SimpleVariable: input.NewSimpleVariable(id, constraint.Mandatory(), constraint.Dependency(variableIDs...)), diff --git a/internal/resolution/variables/required_package.go b/internal/resolution/variables/required_package.go index 368d09c6c..dd1add43b 100644 --- a/internal/resolution/variables/required_package.go +++ b/internal/resolution/variables/required_package.go @@ -23,9 +23,9 @@ func (r *RequiredPackageVariable) Bundles() []*catalogmetadata.Bundle { func NewRequiredPackageVariable(packageName string, bundles []*catalogmetadata.Bundle) *RequiredPackageVariable { id := deppy.IdentifierFromString(fmt.Sprintf("required package %s", packageName)) - var variableIDs []deppy.Identifier + variableIDs := make([]deppy.Identifier, 0, len(bundles)) for _, bundle := range bundles { - variableIDs = append(variableIDs, BundleToBundleVariableIDs(bundle)...) + variableIDs = append(variableIDs, BundleVariableID(bundle)) } return &RequiredPackageVariable{ SimpleVariable: input.NewSimpleVariable(id, constraint.Mandatory(), constraint.Dependency(variableIDs...)), diff --git a/internal/resolution/variablesources/bundles_and_dependencies.go b/internal/resolution/variablesources/bundles_and_dependencies.go index 6b8dcd782..88d2a91c2 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies.go +++ b/internal/resolution/variablesources/bundles_and_dependencies.go @@ -63,25 +63,25 @@ func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]depp var head *catalogmetadata.Bundle head, bundleQueue = bundleQueue[0], bundleQueue[1:] - for _, id := range olmvariables.BundleToBundleVariableIDs(head) { - // ignore bundles that have already been processed - if _, ok := visited[id]; ok { - continue - } - visited[id] = struct{}{} - - // get bundle dependencies - dependencies, err := b.filterBundleDependencies(allBundles, head) - if err != nil { - return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err) - } + id := olmvariables.BundleVariableID(head) - // add bundle dependencies to queue for processing - bundleQueue = append(bundleQueue, dependencies...) + // ignore bundles that have already been processed + if _, ok := visited[id]; ok { + continue + } + visited[id] = struct{}{} - // create variable - variables = append(variables, olmvariables.NewBundleVariable(id, head, dependencies)) + // get bundle dependencies + dependencies, err := b.filterBundleDependencies(allBundles, head) + if err != nil { + return nil, fmt.Errorf("could not determine dependencies for bundle with id '%s': %w", id, err) } + + // add bundle dependencies to queue for processing + bundleQueue = append(bundleQueue, dependencies...) + + // create variable + variables = append(variables, olmvariables.NewBundleVariable(head, dependencies)) } return variables, nil @@ -101,11 +101,10 @@ func (b *BundlesAndDepsVariableSource) filterBundleDependencies(allBundles []*ca } for i := 0; i < len(packageDependencyBundles); i++ { bundle := packageDependencyBundles[i] - for _, id := range olmvariables.BundleToBundleVariableIDs(bundle) { - if _, ok := added[id]; !ok { - dependencies = append(dependencies, bundle) - added[id] = struct{}{} - } + id := olmvariables.BundleVariableID(bundle) + if _, ok := added[id]; !ok { + dependencies = append(dependencies, bundle) + added[id] = struct{}{} } } } diff --git a/internal/resolution/variablesources/bundles_and_dependencies_test.go b/internal/resolution/variablesources/bundles_and_dependencies_test.go index 78805f75e..1c71a8ada 100644 --- a/internal/resolution/variablesources/bundles_and_dependencies_test.go +++ b/internal/resolution/variablesources/bundles_and_dependencies_test.go @@ -316,13 +316,13 @@ var _ = Describe("BundlesAndDepsVariableSource", func() { // dependencies (bundles 7, 8, 9, 10, 11) to appear here due to their GVKs being required by // some of the packages. Expect(bundleVariables).To(WithTransform(CollectBundleVariableIDs, Equal([]string{ - "fake-catalog-test-package-stable-bundle-2", - "fake-catalog-test-package-stable-bundle-1", - "fake-catalog-test-package-2-stable-bundle-15", - "fake-catalog-test-package-2-stable-bundle-16", - "fake-catalog-test-package-2-stable-bundle-17", - "fake-catalog-some-package-stable-bundle-5", - "fake-catalog-some-package-stable-bundle-4", + "fake-catalog-test-package-bundle-2", + "fake-catalog-test-package-bundle-1", + "fake-catalog-test-package-2-bundle-15", + "fake-catalog-test-package-2-bundle-16", + "fake-catalog-test-package-2-bundle-17", + "fake-catalog-some-package-bundle-5", + "fake-catalog-some-package-bundle-4", }))) // check dependencies for one of the bundles @@ -374,7 +374,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-stable-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() { diff --git a/internal/resolution/variablesources/crd_constraints.go b/internal/resolution/variablesources/crd_constraints.go index 746ed14fa..5cd896028 100644 --- a/internal/resolution/variablesources/crd_constraints.go +++ b/internal/resolution/variablesources/crd_constraints.go @@ -50,15 +50,14 @@ func (g *CRDUniquenessConstraintsVariableSource) GetVariables(ctx context.Contex bundles := []*catalogmetadata.Bundle{v.Bundle()} bundles = append(bundles, v.Dependencies()...) for _, bundle := range bundles { - for _, id := range olmvariables.BundleToBundleVariableIDs(bundle) { - // get bundleID package and update map - packageName := bundle.Package + id := olmvariables.BundleVariableID(bundle) + // get bundleID package and update map + packageName := bundle.Package - if _, ok := pkgToBundleMap[packageName]; !ok { - pkgToBundleMap[packageName] = map[deppy.Identifier]struct{}{} - } - pkgToBundleMap[packageName][id] = struct{}{} + if _, ok := pkgToBundleMap[packageName]; !ok { + pkgToBundleMap[packageName] = map[deppy.Identifier]struct{}{} } + pkgToBundleMap[packageName][id] = struct{}{} } } } diff --git a/internal/resolution/variablesources/crd_constraints_test.go b/internal/resolution/variablesources/crd_constraints_test.go index d9c02c608..a34f39d63 100644 --- a/internal/resolution/variablesources/crd_constraints_test.go +++ b/internal/resolution/variablesources/crd_constraints_test.go @@ -203,7 +203,7 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { bundleSet["bundle-15"], bundleSet["bundle-16"], }), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-2"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-2"], []*catalogmetadata.Bundle{ bundleSet["bundle-3"], @@ -213,7 +213,7 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { bundleSet["bundle-7"], }, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-1"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-1"], []*catalogmetadata.Bundle{ bundleSet["bundle-6"], @@ -221,23 +221,23 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { bundleSet["bundle-8"], }, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-3"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-3"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-4"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-4"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-5"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-5"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-6"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-6"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-7"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-7"], []*catalogmetadata.Bundle{ bundleSet["bundle-8"], @@ -245,27 +245,27 @@ var _ = Describe("CRDUniquenessConstraintsVariableSource", func() { bundleSet["bundle-10"], }, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-8"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-8"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-9"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-9"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-10"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-10"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-14"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-14"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-15"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-15"], []*catalogmetadata.Bundle{}, ), - olmvariables.NewBundleVariable(olmvariables.BundleToBundleVariableIDs(bundleSet["bundle-16"])[0], + olmvariables.NewBundleVariable( bundleSet["bundle-16"], []*catalogmetadata.Bundle{}, ),