Changes for PR comments...

#1359 (comment)

#1359 (comment)

#1359 (comment)

#1359 (comment)

#1359 (comment)

#1359 (comment)
Signed-off-by: Brett Tofel <[email protected]>

Author: bentito

internal/resolve/catalog.go

@@ -232,52 +232,33 @@ func isDeprecated(bundle declcfg.Bundle, deprecation *declcfg.Deprecation) bool
 
 type CatalogWalkFunc func(context.Context, *catalogd.ClusterCatalog, *declcfg.DeclarativeConfig, error) error
 
-func CatalogWalker(listCatalogs func(context.Context, ...client.ListOption) ([]catalogd.ClusterCatalog, error), getPackage func(context.Context, *catalogd.ClusterCatalog, string) (*declcfg.DeclarativeConfig, error)) func(ctx context.Context, packageName string, f CatalogWalkFunc, catalogListOpts ...client.ListOption) error {
+func CatalogWalker(
+	listCatalogs func(context.Context, ...client.ListOption) ([]catalogd.ClusterCatalog, error),
+	getPackage func(context.Context, *catalogd.ClusterCatalog, string) (*declcfg.DeclarativeConfig, error),
+) func(ctx context.Context, packageName string, f CatalogWalkFunc, catalogListOpts ...client.ListOption) error {
 	return func(ctx context.Context, packageName string, f CatalogWalkFunc, catalogListOpts ...client.ListOption) error {
 		l := log.FromContext(ctx)
 		catalogs, err := listCatalogs(ctx, catalogListOpts...)
 		if err != nil {
 			return fmt.Errorf("error listing catalogs: %w", err)
 		}
 
-		// Track if at least one catalog was processed (not disabled)
-		processedCatalogs := false
-		bundleFound := false
-
 		for i := range catalogs {
 			cat := &catalogs[i]
 
 			// skip catalogs with Availability set to "Disabled"
-			if cat.Spec.Availability == "" || cat.Spec.Availability == "Enabled" {
-				// Continue processing the catalog as it is enabled
-			} else if cat.Spec.Availability == "Disabled" {
+			if cat.Spec.Availability == "Disabled" {
 				l.Info("excluding ClusterCatalog from resolution process since it is disabled", "catalog", cat.Name)
 				continue
 			}
 
-			// Mark that we processed at least one catalog
-			processedCatalogs = true
-
 			fbc, fbcErr := getPackage(ctx, cat, packageName)
-			if fbcErr == nil && fbc != nil {
-				bundleFound = true
-			}
 
 			if walkErr := f(ctx, cat, fbc, fbcErr); walkErr != nil {
 				return walkErr
 			}
 		}
 
-		// If no catalogs were processed at all, return a 'no catalogs' error
-		if !processedCatalogs {
-			return fmt.Errorf("no enabled catalogs found")
-		}
-
-		// Return an error if no valid bundle was found in any processed catalog
-		if !bundleFound {
-			return fmt.Errorf("no bundles found for package: %s", packageName)
-		}
-
 		return nil
 	}
 }

internal/resolve/catalog_test.go

@@ -982,7 +982,7 @@ func TestAllCatalogsDisabled(t *testing.T) {
 	}
 
 	getPackage := func(ctx context.Context, cat *catalogd.ClusterCatalog, packageName string) (*declcfg.DeclarativeConfig, error) {
-		return genPackage(pkgName), nil
+		panic("getPackage should never be called when all catalogs are disabled")
 	}
 
 	r := CatalogResolver{
@@ -992,7 +992,7 @@ func TestAllCatalogsDisabled(t *testing.T) {
 	ce := buildFooClusterExtension(pkgName, []string{}, ">=1.0.0", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
 	_, _, _, err := r.Resolve(context.Background(), ce, nil)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "no enabled catalogs found")
+	assert.Contains(t, err.Error(), "no bundles found for package")
 }
 
 func TestSomeCatalogsDisabled(t *testing.T) {
@@ -1032,33 +1032,58 @@ func TestPriorityRespectedWithDisabledCatalogs(t *testing.T) {
 	listCatalogs := func(ctx context.Context, options ...client.ListOption) ([]catalogd.ClusterCatalog, error) {
 		return []catalogd.ClusterCatalog{
 			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "enabledCatalog",
+				},
 				Spec: catalogd.ClusterCatalogSpec{
-					Priority:     1,
+					Priority:     0, // Lower priority
 					Availability: "Enabled",
 				},
 			},
 			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "disabledCatalog",
+				},
 				Spec: catalogd.ClusterCatalogSpec{
-					Priority:     0,
+					Priority:     1, // Higher priority
 					Availability: "Disabled",
 				},
 			},
 		}, nil
 	}
 
 	getPackage := func(ctx context.Context, cat *catalogd.ClusterCatalog, packageName string) (*declcfg.DeclarativeConfig, error) {
-		return genPackage(pkgName), nil
+		if cat.Spec.Availability == "Disabled" {
+			panic("getPackage should not be called for disabled catalogs")
+		}
+		// Enabled catalog returns a lower version
+		return &declcfg.DeclarativeConfig{
+			Packages: []declcfg.Package{{Name: pkgName}},
+			Channels: []declcfg.Channel{
+				{
+					Package: pkgName,
+					Name:    "alpha",
+					Entries: []declcfg.ChannelEntry{
+						{Name: bundleName(pkgName, "1.0.0")},
+					},
+				},
+			},
+			Bundles: []declcfg.Bundle{
+				genBundle(pkgName, "1.0.0"),
+			},
+		}, nil
 	}
 
 	r := CatalogResolver{
 		WalkCatalogsFunc: CatalogWalker(listCatalogs, getPackage),
 	}
 
-	ce := buildFooClusterExtension(pkgName, []string{}, ">=1.0.0", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
+	ce := buildFooClusterExtension(pkgName, []string{}, "", ocv1alpha1.UpgradeConstraintPolicyCatalogProvided)
 	gotBundle, gotVersion, _, err := r.Resolve(context.Background(), ce, nil)
 	require.NoError(t, err)
 	require.NotNil(t, gotBundle)
-	require.Equal(t, bsemver.MustParse("3.0.0"), *gotVersion)
+	require.Equal(t, genBundle(pkgName, "1.0.0"), *gotBundle)
+	require.Equal(t, bsemver.MustParse("1.0.0"), *gotVersion)
 }
 
 func TestCatalogWithoutAvailabilityIsEnabled(t *testing.T) {