Improve Catalog Selection Testing + Fix small issue

We initially assumed that the resolvedBundle index we check doesn't matter when it comes to checking for prior package deprecations in the resolver but the index needs to match the prior result or the bundle names may not match and give a false negative. Also improved the unit test cases (which caught the above issue). Signed-off-by: dtfranz <[email protected]>
operator-framework · Aug 26, 2024 · 96019b6 · 96019b6
1 parent 2391092
commit 96019b6
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 17 deletions.
diff --git a/internal/resolve/catalog.go b/internal/resolve/catalog.go
@@ -114,7 +114,7 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 		if len(resolvedBundles) != 0 {
 			// We've already found one or more package candidates
 			currentIsDeprecated := isDeprecated(thisBundle, thisDeprecation)
-			priorIsDeprecated := isDeprecated(*resolvedBundles[0], priorDeprecation) // Slice index doesn't matter; the whole slice is either deprecated or not
+			priorIsDeprecated := isDeprecated(*resolvedBundles[len(resolvedBundles)-1], priorDeprecation)
 			if currentIsDeprecated && !priorIsDeprecated {
 				// Skip this deprecated package and retain the non-deprecated package(s)
 				return nil

diff --git a/internal/resolve/catalog_test.go b/internal/resolve/catalog_test.go
@@ -236,46 +236,97 @@ func TestAcceptDeprecated(t *testing.T) {
 
 func TestPackageVariationsBetweenCatalogs(t *testing.T) {
 	pkgName := randPkg()
-	genImgRef := func(catalog, name string) string {
-		return fmt.Sprintf("%s/%s", catalog, name)
-	}
 	w := staticCatalogWalker{
 		"a": func() (*declcfg.DeclarativeConfig, error) { return &declcfg.DeclarativeConfig{}, nil },
 		"b": func() (*declcfg.DeclarativeConfig, error) {
-			fbc := genPackage(pkgName)
-			fbc.Bundles = append(fbc.Bundles, genBundle(pkgName, "1.0.3"))
-			for i := range fbc.Bundles {
-				fbc.Bundles[i].Image = genImgRef("catalog-b", fbc.Bundles[i].Name)
+			fbc := &declcfg.DeclarativeConfig{
+				Packages: []declcfg.Package{{Name: pkgName}},
+				Bundles:  []declcfg.Bundle{genBundle(pkgName, "1.0.0")},
+				Deprecations: []declcfg.Deprecation{{
+					Package: pkgName,
+					Entries: []declcfg.DeprecationEntry{
+						{
+							Reference: declcfg.PackageScopedReference{Schema: declcfg.SchemaBundle, Name: bundleName(pkgName, "1.0.0")},
+							Message:   fmt.Sprintf("bundle %s is deprecated", bundleName(pkgName, "1.0.0")),
+						},
+					},
+				}},
 			}
 			return fbc, nil
 		},
 		"c": func() (*declcfg.DeclarativeConfig, error) {
-			fbc := genPackage(pkgName)
-			fbc.Bundles = append(fbc.Bundles, genBundle(pkgName, "0.1.1"))
-			fbc.Deprecations = nil
-			for i := range fbc.Bundles {
-				fbc.Bundles[i].Image = genImgRef("catalog-c", fbc.Bundles[i].Name)
+			fbc := &declcfg.DeclarativeConfig{
+				Packages: []declcfg.Package{{Name: pkgName}},
+				Bundles:  []declcfg.Bundle{genBundle(pkgName, "1.0.1")},
+				Deprecations: []declcfg.Deprecation{{
+					Package: pkgName,
+					Entries: []declcfg.DeprecationEntry{
+						{
+							Reference: declcfg.PackageScopedReference{Schema: declcfg.SchemaBundle, Name: bundleName(pkgName, "1.0.1")},
+							Message:   fmt.Sprintf("bundle %s is deprecated", bundleName(pkgName, "1.0.1")),
+						},
+					},
+				}},
+			}
+			return fbc, nil
+		},
+		"d": func() (*declcfg.DeclarativeConfig, error) {
+			fbc := &declcfg.DeclarativeConfig{
+				Packages: []declcfg.Package{{Name: pkgName}},
+				Bundles:  []declcfg.Bundle{genBundle(pkgName, "1.0.2")},
+			}
+			return fbc, nil
+		},
+		"e": func() (*declcfg.DeclarativeConfig, error) {
+			fbc := &declcfg.DeclarativeConfig{
+				Packages: []declcfg.Package{{Name: pkgName}},
+				Bundles:  []declcfg.Bundle{genBundle(pkgName, "1.0.3")},
+				Deprecations: []declcfg.Deprecation{{
+					Package: pkgName,
+					Entries: []declcfg.DeprecationEntry{
+						{
+							Reference: declcfg.PackageScopedReference{Schema: declcfg.SchemaBundle, Name: bundleName(pkgName, "1.0.3")},
+							Message:   fmt.Sprintf("bundle %s is deprecated", bundleName(pkgName, "1.0.3")),
+						},
+					},
+				}},
+			}
+			return fbc, nil
+		},
+		"f": func() (*declcfg.DeclarativeConfig, error) {
+			fbc := &declcfg.DeclarativeConfig{
+				Packages: []declcfg.Package{{Name: pkgName}},
+				Bundles:  []declcfg.Bundle{genBundle(pkgName, "1.0.4")},
 			}
 			return fbc, nil
 		},
 	}
 	r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs}
 
-	t.Run("when catalog b has a newer version that matches the range", func(t *testing.T) {
+	t.Run("when package is deprecated in all but one catalog", func(t *testing.T) {
 		ce := buildFooClusterExtension(pkgName, "", ">=1.0.0 <=1.0.3", ocv1alpha1.UpgradeConstraintPolicyEnforce)
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
+		require.NoError(t, err)
+		assert.Equal(t, genBundle(pkgName, "1.0.2").Name, gotBundle.Name)
+		assert.Equal(t, bsemver.MustParse("1.0.2"), *gotVersion)
+		assert.Equal(t, (*declcfg.Deprecation)(nil), gotDeprecation)
+	})
+
+	t.Run("when package is found and deprecated in multiple catalogs", func(t *testing.T) {
+		ce := buildFooClusterExtension(pkgName, "", ">=1.0.0 <=1.0.1", ocv1alpha1.UpgradeConstraintPolicyEnforce)
+		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
 		require.Error(t, err)
 		assert.ErrorContains(t, err, "found in multiple catalogs: [b c]")
 		assert.Nil(t, gotBundle)
 		assert.Nil(t, gotVersion)
 		assert.Nil(t, gotDeprecation)
 	})
 
-	t.Run("when catalog c has a newer version that matches the range", func(t *testing.T) {
-		ce := buildFooClusterExtension(pkgName, "", ">=0.1.0 <1.0.0", ocv1alpha1.UpgradeConstraintPolicyEnforce)
+	t.Run("when package is found and not deprecated in multiple catalogs", func(t *testing.T) {
+		ce := buildFooClusterExtension(pkgName, "", ">=1.0.0 <=1.0.4", ocv1alpha1.UpgradeConstraintPolicyEnforce)
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
 		require.Error(t, err)
-		assert.ErrorContains(t, err, "found in multiple catalogs: [b c]")
+		assert.ErrorContains(t, err, "found in multiple catalogs: [d f]")
 		assert.Nil(t, gotBundle)
 		assert.Nil(t, gotVersion)
 		assert.Nil(t, gotDeprecation)