address latest review feedback

Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>

Author: rashmigottipati

internal/operator-controller/bundleutil/bundle.go

@@ -29,24 +29,19 @@ func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error
 		return nil, fmt.Errorf("error unmarshalling package property: %w", err)
 	}
 
+	// Check if release field is explicitly present in JSON (even if empty).
+	// property.Package has Release string, so we can't distinguish "field absent" from "field empty".
+	// We unmarshal again into a helper struct with Release *string to detect presence.
+	var releaseField struct {
+		Release *string `json:"release"`
+	}
+	if err := json.Unmarshal(pkgData, &releaseField); err != nil {
+		return nil, fmt.Errorf("error unmarshalling package release field: %w", err)
+	}
+
 	// When BundleReleaseSupport is enabled and bundle has explicit release field, use it.
-	// Note: Build metadata is preserved here because with an explicit release field,
-	// build metadata serves its proper semver purpose (e.g., git commit, build number).
-	// In contrast, NewLegacyRegistryV1VersionRelease clears build metadata because it
-	// interprets build metadata AS the release value for registry+v1 bundles.
-	if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) && pkg.Release != "" {
-		vers, err := bsemver.Parse(pkg.Version)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing version %q: %w", pkg.Version, err)
-		}
-		rel, err := bundle.NewRelease(pkg.Release)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing release %q: %w", pkg.Release, err)
-		}
-		return &bundle.VersionRelease{
-			Version: vers,
-			Release: rel,
-		}, nil
+	if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) && releaseField.Release != nil {
+		return parseExplicitRelease(pkg.Version, *releaseField.Release)
 	}
 
 	// Fall back to legacy registry+v1 behavior (release in build metadata)
@@ -62,15 +57,53 @@ func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error
 	return vr, nil
 }
 
+// parseExplicitRelease parses version and release from separate fields.
+// Build metadata is preserved in the version because with an explicit release field,
+// build metadata serves its proper semver purpose (e.g., git commit, build number).
+// In contrast, NewLegacyRegistryV1VersionRelease clears build metadata because it
+// interprets build metadata AS the release value for registry+v1 bundles.
+func parseExplicitRelease(version, releaseStr string) (*bundle.VersionRelease, error) {
+	vers, err := bsemver.Parse(version)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing version %q: %w", version, err)
+	}
+
+	var rel bundle.Release
+	if releaseStr == "" {
+		// Explicit empty release: use empty slice (not nil)
+		rel = bundle.Release([]bsemver.PRVersion{})
+	} else {
+		rel, err = bundle.NewRelease(releaseStr)
+		if err != nil {
+			return nil, fmt.Errorf("error parsing release %q: %w", releaseStr, err)
+		}
+	}
+
+	return &bundle.VersionRelease{
+		Version: vers,
+		Release: rel,
+	}, nil
+}
+
 // MetadataFor returns a BundleMetadata for the given bundle name and version/release.
 func MetadataFor(bundleName string, vr bundle.VersionRelease) ocv1.BundleMetadata {
-	bm := ocv1.BundleMetadata{
-		Name:    bundleName,
-		Version: vr.Version.String(),
+	if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) {
+		// New behavior: separate Version and Release fields
+		bm := ocv1.BundleMetadata{
+			Name:    bundleName,
+			Version: vr.Version.String(),
+		}
+		if vr.Release != nil {
+			relStr := vr.Release.String()
+			bm.Release = &relStr
+		}
+		return bm
 	}
-	if vr.Release != nil {
-		relStr := vr.Release.String()
-		bm.Release = &relStr
+	// Old behavior for backward compatibility: reconstitute build metadata in Version field
+	// This preserves release information (e.g., "1.0.0+2") for standard CRD users where
+	// the Release field is pruned by the API server.
+	return ocv1.BundleMetadata{
+		Name:    bundleName,
+		Version: vr.AsLegacyRegistryV1Version().String(),
 	}
-	return bm
 }

internal/operator-controller/bundleutil/bundle_test.go

@@ -151,6 +151,18 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) {
 				},
 				wantErr: true,
 			},
+			{
+				name: "explicit empty release - preserves build metadata in version",
+				pkgProperty: &property.Property{
+					Type:  property.TypePackage,
+					Value: json.RawMessage(`{"version": "1.0.0+2", "release": ""}`),
+				},
+				wantVersionRelease: &bundle.VersionRelease{
+					Version: bsemver.MustParse("1.0.0+2"),          // Build metadata preserved (not extracted as release)
+					Release: bundle.Release([]bsemver.PRVersion{}), // Explicit empty release
+				},
+				wantErr: false,
+			},
 		}
 
 		for _, tc := range tests {
@@ -205,38 +217,76 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) {
 }
 
 func TestMetadataFor(t *testing.T) {
-	t.Run("with release", func(t *testing.T) {
-		vr := bundle.VersionRelease{
-			Version: bsemver.MustParse("1.0.0"),
-			Release: bundle.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}),
-		}
-		result := bundleutil.MetadataFor("test-bundle", vr)
-		require.Equal(t, "test-bundle", result.Name)
-		require.Equal(t, "1.0.0", result.Version)
-		require.NotNil(t, result.Release)
-		require.Equal(t, "2", *result.Release)
-	})
+	t.Run("with feature gate enabled", func(t *testing.T) {
+		prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport)
+		require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=true"))
+		t.Cleanup(func() {
+			require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled)))
+		})
 
-	t.Run("without release", func(t *testing.T) {
-		vr := bundle.VersionRelease{
-			Version: bsemver.MustParse("1.0.0"),
-			Release: nil,
-		}
-		result := bundleutil.MetadataFor("test-bundle", vr)
-		require.Equal(t, "test-bundle", result.Name)
-		require.Equal(t, "1.0.0", result.Version)
-		require.Nil(t, result.Release)
+		t.Run("with release", func(t *testing.T) {
+			vr := bundle.VersionRelease{
+				Version: bsemver.MustParse("1.0.0"),
+				Release: bundle.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}),
+			}
+			result := bundleutil.MetadataFor("test-bundle", vr)
+			require.Equal(t, "test-bundle", result.Name)
+			require.Equal(t, "1.0.0", result.Version)
+			require.NotNil(t, result.Release)
+			require.Equal(t, "2", *result.Release)
+		})
+
+		t.Run("without release", func(t *testing.T) {
+			vr := bundle.VersionRelease{
+				Version: bsemver.MustParse("1.0.0"),
+				Release: nil,
+			}
+			result := bundleutil.MetadataFor("test-bundle", vr)
+			require.Equal(t, "test-bundle", result.Name)
+			require.Equal(t, "1.0.0", result.Version)
+			require.Nil(t, result.Release)
+		})
+
+		t.Run("with explicit empty release", func(t *testing.T) {
+			vr := bundle.VersionRelease{
+				Version: bsemver.MustParse("1.0.0"),
+				Release: bundle.Release([]bsemver.PRVersion{}),
+			}
+			result := bundleutil.MetadataFor("test-bundle", vr)
+			require.Equal(t, "test-bundle", result.Name)
+			require.Equal(t, "1.0.0", result.Version)
+			require.NotNil(t, result.Release)
+			require.Empty(t, *result.Release)
+		})
 	})
 
-	t.Run("with explicit empty release", func(t *testing.T) {
-		vr := bundle.VersionRelease{
-			Version: bsemver.MustParse("1.0.0"),
-			Release: bundle.Release([]bsemver.PRVersion{}),
-		}
-		result := bundleutil.MetadataFor("test-bundle", vr)
-		require.Equal(t, "test-bundle", result.Name)
-		require.Equal(t, "1.0.0", result.Version)
-		require.NotNil(t, result.Release)
-		require.Empty(t, *result.Release)
+	t.Run("with feature gate disabled (legacy mode)", func(t *testing.T) {
+		prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport)
+		require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=false"))
+		t.Cleanup(func() {
+			require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled)))
+		})
+
+		t.Run("reconstitutes build metadata in version", func(t *testing.T) {
+			vr := bundle.VersionRelease{
+				Version: bsemver.MustParse("1.0.0"),
+				Release: bundle.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}),
+			}
+			result := bundleutil.MetadataFor("test-bundle", vr)
+			require.Equal(t, "test-bundle", result.Name)
+			require.Equal(t, "1.0.0+2", result.Version) // Build metadata reconstituted
+			require.Nil(t, result.Release)              // Release field not used in legacy mode
+		})
+
+		t.Run("preserves original build metadata when no release", func(t *testing.T) {
+			vr := bundle.VersionRelease{
+				Version: bsemver.MustParse("1.0.0"),
+				Release: nil,
+			}
+			result := bundleutil.MetadataFor("test-bundle", vr)
+			require.Equal(t, "test-bundle", result.Name)
+			require.Equal(t, "1.0.0", result.Version)
+			require.Nil(t, result.Release)
+		})
 	})
 }

internal/operator-controller/catalogmetadata/filter/successors.go

@@ -12,37 +12,54 @@ import (
 	"github.com/operator-framework/operator-controller/internal/shared/util/filter"
 )
 
-func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Channel) (filter.Predicate[declcfg.Bundle], error) {
-	// Construct VersionRelease from BundleMetadata.
-	// If the Release field is not nil (even if empty string), use it as the explicit release.
-	// If the Release field is nil, parse release from version build metadata (registry+v1 legacy format).
-	var installedVersionRelease *bundle.VersionRelease
-	var err error
-
-	if installedBundle.Release != nil {
-		// Bundle has explicit release field (or explicitly empty) - parse version and release from separate fields.
-		// Note: We can't use NewLegacyRegistryV1VersionRelease here because the version might
-		// already contain build metadata (e.g., "1.0.0+git.abc"), which serves its proper
-		// semver purpose when using explicit pkg.Release. Concatenating would create invalid
-		// semver like "1.0.0+git.abc+2".
-		version, err := bsemver.Parse(installedBundle.Version)
+// parseInstalledBundleVersionRelease constructs a VersionRelease from BundleMetadata.
+// If the Release field is not nil (even if empty string), use it as the explicit release.
+// If the Release field is nil, parse release from version build metadata (registry+v1 legacy format).
+func parseInstalledBundleVersionRelease(installedBundle ocv1.BundleMetadata) (*bundle.VersionRelease, error) {
+	// Handle legacy registry+v1 format: release embedded in version's build metadata
+	if installedBundle.Release == nil {
+		vr, err := bundle.NewLegacyRegistryV1VersionRelease(installedBundle.Version)
 		if err != nil {
-			return nil, fmt.Errorf("failed to parse installed bundle version %q: %w", installedBundle.Version, err)
-		}
-		release, err := bundle.NewRelease(*installedBundle.Release)
-		if err != nil {
-			return nil, fmt.Errorf("failed to parse installed bundle release %q: %w", *installedBundle.Release, err)
-		}
-		installedVersionRelease = &bundle.VersionRelease{
-			Version: version,
-			Release: release,
+			return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err)
 		}
-	} else {
-		// Legacy registry+v1: release embedded in version's build metadata
-		installedVersionRelease, err = bundle.NewLegacyRegistryV1VersionRelease(installedBundle.Version)
+		return vr, nil
+	}
+
+	// Bundle has explicit release field (or explicitly empty) - parse version and release from separate fields.
+	// Note: We can't use NewLegacyRegistryV1VersionRelease here because the version might
+	// already contain build metadata (e.g., "1.0.0+git.abc"), which serves its proper
+	// semver purpose when using explicit pkg.Release. Concatenating would create invalid
+	// semver like "1.0.0+git.abc+2".
+	version, err := bsemver.Parse(installedBundle.Version)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse installed bundle version %q: %w", installedBundle.Version, err)
+	}
+
+	// if version has build metadata but release is empty, this is likely
+	// old-format data (e.g., "1.0.0+2" with Release="") that should be parsed as legacy.
+	// This handles edge case scenarios.
+	if len(version.Build) > 0 && *installedBundle.Release == "" {
+		vr, err := bundle.NewLegacyRegistryV1VersionRelease(installedBundle.Version)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err)
 		}
+		return vr, nil
+	}
+
+	release, err := bundle.NewRelease(*installedBundle.Release)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse installed bundle release %q: %w", *installedBundle.Release, err)
+	}
+	return &bundle.VersionRelease{
+		Version: version,
+		Release: release,
+	}, nil
+}
+
+func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Channel) (filter.Predicate[declcfg.Bundle], error) {
+	installedVersionRelease, err := parseInstalledBundleVersionRelease(installedBundle)
+	if err != nil {
+		return nil, err
 	}
 
 	successorsPredicate, err := legacySuccessor(installedBundle, channels...)

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

@@ -103,16 +103,14 @@ func MigrateStorage(m StorageMigrator) ReconcileStepFunc {
 func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error)) ReconcileStepFunc {
 	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
-		releaseValue := ""
-		if state.resolvedRevisionMetadata.Release != nil {
-			releaseValue = *state.resolvedRevisionMetadata.Release
-		}
 		revisionAnnotations := map[string]string{
 			labels.BundleNameKey:      state.resolvedRevisionMetadata.Name,
 			labels.PackageNameKey:     state.resolvedRevisionMetadata.Package,
 			labels.BundleVersionKey:   state.resolvedRevisionMetadata.Version,
 			labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
-			labels.BundleReleaseKey:   releaseValue,
+		}
+		if state.resolvedRevisionMetadata.Release != nil {
+			revisionAnnotations[labels.BundleReleaseKey] = *state.resolvedRevisionMetadata.Release
 		}
 		objLbls := map[string]string{
 			labels.OwnerKindKey: ocv1.ClusterExtensionKind,

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

@@ -408,16 +408,14 @@ func UnpackBundle(i imageutil.Puller, cache imageutil.Cache) ReconcileStepFunc {
 func ApplyBundle(a Applier) ReconcileStepFunc {
 	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
-		releaseValue := ""
-		if state.resolvedRevisionMetadata.Release != nil {
-			releaseValue = *state.resolvedRevisionMetadata.Release
-		}
 		revisionAnnotations := map[string]string{
 			labels.BundleNameKey:      state.resolvedRevisionMetadata.Name,
 			labels.PackageNameKey:     state.resolvedRevisionMetadata.Package,
 			labels.BundleVersionKey:   state.resolvedRevisionMetadata.Version,
 			labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
-			labels.BundleReleaseKey:   releaseValue,
+		}
+		if state.resolvedRevisionMetadata.Release != nil {
+			revisionAnnotations[labels.BundleReleaseKey] = *state.resolvedRevisionMetadata.Release
 		}
 		objLbls := map[string]string{
 			labels.OwnerKindKey: ocv1.ClusterExtensionKind,