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 <mradchuk@redhat.com>

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),
+	)
 }

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)

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...)),

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...)),

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{}{}
 			}
 		}
 	}

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() {

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{}{}
 			}
 		}
 	}

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,59 +213,59 @@ 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"],
 					bundleSet["bundle-7"],
 					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"],
 					bundleSet["bundle-9"],
 					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{},
 			),