Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions helm/experimental.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ options:
- HelmChartSupport
- BoxcutterRuntime
- DeploymentConfig
- CompositeVersionComparison
disabled:
- WebhookProviderOpenshiftServiceCA
# List of enabled experimental features for catalogd
Expand Down
45 changes: 31 additions & 14 deletions internal/operator-controller/bundleutil/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,42 @@ import (
func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) {
for _, p := range b.Properties {
if p.Type == property.TypePackage {
var pkg property.Package
if err := json.Unmarshal(p.Value, &pkg); err != nil {
return nil, fmt.Errorf("error unmarshalling package property: %w", err)
}

// TODO: For now, we assume that all bundles are registry+v1 bundles.
// In the future, when we support other bundle formats, we should stop
// using the legacy mechanism (i.e. using build metadata in the version)
// to determine the bundle's release.
vr, err := bundle.NewLegacyRegistryV1VersionRelease(pkg.Version)
if err != nil {
return nil, err
}
return vr, nil
return parseVersionRelease(p.Value)
}
}
return nil, fmt.Errorf("no package property found in bundle %q", b.Name)
}

func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error) {
var pkg property.Package
if err := json.Unmarshal(pkgData, &pkg); err != nil {
return nil, fmt.Errorf("error unmarshalling package property: %w", err)
}

// If the bundle has an explicit release field, use it
if 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
}

// Fall back to legacy registry+v1 behavior (release in build metadata)
vr, err := bundle.NewLegacyRegistryV1VersionRelease(pkg.Version)
if err != nil {
return nil, err
}
return vr, nil
}

// MetadataFor returns a BundleMetadata for the given bundle name and version.
func MetadataFor(bundleName string, bundleVersion bsemver.Version) ocv1.BundleMetadata {
return ocv1.BundleMetadata{
Expand Down
41 changes: 40 additions & 1 deletion internal/operator-controller/bundleutil/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,44 @@ func TestGetVersionAndRelease(t *testing.T) {
pkgProperty: nil,
wantErr: true,
},
{
name: "explicit release field - takes precedence over build metadata",
pkgProperty: &property.Property{
Type: property.TypePackage,
Value: json.RawMessage(`{"version": "1.0.0+ignored", "release": "2"}`),
},
wantVersionRelease: &bundle.VersionRelease{
Version: bsemver.MustParse("1.0.0+ignored"),
Release: bundle.Release([]bsemver.PRVersion{
{VersionNum: 2, IsNum: true},
}),
},
wantErr: false,
},
{
name: "explicit release field - complex release",
pkgProperty: &property.Property{
Type: property.TypePackage,
Value: json.RawMessage(`{"version": "2.1.0", "release": "1.alpha.3"}`),
},
wantVersionRelease: &bundle.VersionRelease{
Version: bsemver.MustParse("2.1.0"),
Release: bundle.Release([]bsemver.PRVersion{
{VersionNum: 1, IsNum: true},
{VersionStr: "alpha"},
{VersionNum: 3, IsNum: true},
}),
},
wantErr: false,
},
{
name: "explicit release field - invalid release",
pkgProperty: &property.Property{
Type: property.TypePackage,
Value: json.RawMessage(`{"version": "1.0.0", "release": "001"}`),
},
wantErr: true,
},
}

for _, tc := range tests {
Expand All @@ -83,11 +121,12 @@ func TestGetVersionAndRelease(t *testing.T) {
Properties: properties,
}

_, err := bundleutil.GetVersionAndRelease(bundle)
actual, err := bundleutil.GetVersionAndRelease(bundle)
if tc.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
require.Equal(t, tc.wantVersionRelease, actual)
}
})
}
Expand Down
30 changes: 28 additions & 2 deletions internal/operator-controller/catalogmetadata/compare/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/operator-framework/operator-registry/alpha/declcfg"

"github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
slicesutil "github.com/operator-framework/operator-controller/internal/shared/util/slices"
)

Expand Down Expand Up @@ -39,9 +40,22 @@ func newMastermindsRange(versionRange string) (bsemver.Range, error) {
}

// ByVersionAndRelease is a comparison function that compares bundles by
// version and release. Bundles with lower versions/releases are
// considered less than bundles with higher versions/releases.
// version and release. When the CompositeVersionComparison feature gate is
// enabled, it uses Bundle.Compare() (which reads pkg.Release from olm.package)
// and falls back to build metadata comparison if equal. When disabled, it uses
// version+release from build metadata (backward compatible).
func ByVersionAndRelease(b1, b2 declcfg.Bundle) int {
if features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) {
// Use CompositeVersion comparison (reads pkg.Release from olm.package)
result := b2.Compare(&b1)
if result != 0 {
return result
}
// If CompositeVersion comparison is equal, fall back to build metadata
return compareByBuildMetadata(b1, b2)
}
Comment on lines 47 to +56
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ByVersionAndRelease checks the CompositeVersionComparison feature gate on every comparator invocation. Since this comparator is used inside sorts (O(n log n) comparisons) and the gate value won’t change during a sort, consider hoisting the Enabled() check out (e.g., selecting a comparator function once at call site or providing a factory) to avoid repeated locking/map lookups in hot paths like catalog bundle sorting.

Copilot uses AI. Check for mistakes.

// Default: use version+release from build metadata (backward compatible)
vr1, err1 := bundleutil.GetVersionAndRelease(b1)
vr2, err2 := bundleutil.GetVersionAndRelease(b2)

Expand All @@ -54,6 +68,18 @@ func ByVersionAndRelease(b1, b2 declcfg.Bundle) int {
return vr2.Compare(*vr1)
}

// compareByBuildMetadata compares bundles using build metadata parsing.
// This is used as a fallback when CompositeVersion comparison returns equal.
func compareByBuildMetadata(b1, b2 declcfg.Bundle) int {
vr1, err1 := bundleutil.GetVersionAndRelease(b1)
vr2, err2 := bundleutil.GetVersionAndRelease(b2)

if err1 != nil || err2 != nil {
return compareErrors(err2, err1)
}
return vr2.Compare(*vr1)
}

func ByDeprecationFunc(deprecation declcfg.Deprecation) func(a, b declcfg.Bundle) int {
deprecatedBundles := sets.New[string]()
for _, entry := range deprecation.Entries {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/operator-framework/operator-registry/alpha/property"

"github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
)

func TestNewVersionRange(t *testing.T) {
Expand Down Expand Up @@ -138,13 +139,13 @@ func TestByVersionAndRelease(t *testing.T) {
t.Run("all bundles valid", func(t *testing.T) {
toSort := []declcfg.Bundle{b3_1, b2, b3_2, b1}
slices.SortStableFunc(toSort, compare.ByVersionAndRelease)
assert.Equal(t, []declcfg.Bundle{b1, b3_2, b3_1, b2}, toSort)
assert.Equal(t, []declcfg.Bundle{b1, b3_2, b3_1, b2}, toSort, "should sort descending: 1.0.0 > 1.0.0-alpha+2 > 1.0.0-alpha+1 > 0.0.1")
})

t.Run("some bundles are missing version", func(t *testing.T) {
toSort := []declcfg.Bundle{b3_1, b4noVersion, b2, b3_2, b5empty, b1}
slices.SortStableFunc(toSort, compare.ByVersionAndRelease)
assert.Equal(t, []declcfg.Bundle{b1, b3_2, b3_1, b2, b4noVersion, b5empty}, toSort)
assert.Equal(t, []declcfg.Bundle{b1, b3_2, b3_1, b2, b4noVersion, b5empty}, toSort, "bundles with invalid/missing versions should sort last")
})
}

Expand All @@ -161,10 +162,57 @@ func TestByDeprecationFunc(t *testing.T) {
c := declcfg.Bundle{Name: "c"}
d := declcfg.Bundle{Name: "d"}

assert.Equal(t, 0, byDeprecation(a, b))
assert.Equal(t, 0, byDeprecation(b, a))
assert.Equal(t, 1, byDeprecation(a, c))
assert.Equal(t, -1, byDeprecation(c, a))
assert.Equal(t, 0, byDeprecation(c, d))
assert.Equal(t, 0, byDeprecation(d, c))
assert.Equal(t, 0, byDeprecation(a, b), "both deprecated bundles are equal")
assert.Equal(t, 0, byDeprecation(b, a), "both deprecated bundles are equal")
assert.Equal(t, 1, byDeprecation(a, c), "deprecated bundle 'a' should sort after non-deprecated 'c'")
assert.Equal(t, -1, byDeprecation(c, a), "non-deprecated bundle 'c' should sort before deprecated 'a'")
assert.Equal(t, 0, byDeprecation(c, d), "both non-deprecated bundles are equal")
assert.Equal(t, 0, byDeprecation(d, c), "both non-deprecated bundles are equal")
}

// TestByVersionAndRelease_WithCompositeVersionComparison tests the feature-gated hybrid comparison.
// This test detects the current feature gate state and validates the correct behavior path.
// When CompositeVersionComparison is enabled (experimental deployments), it uses Bundle.Compare()
// with build metadata fallback. When disabled (standard deployments), it uses legacy build metadata only.
func TestByVersionAndRelease_WithCompositeVersionComparison(t *testing.T) {
// Registry+v1 bundles: same version, different build metadata
b1 := declcfg.Bundle{
Name: "package1.v1.0.0+1",
Properties: []property.Property{
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+1"}`)},
},
}
b2 := declcfg.Bundle{
Name: "package1.v1.0.0+2",
Properties: []property.Property{
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+2"}`)},
},
}

// Detect current feature gate state and test the appropriate path
if features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) {
t.Log("CompositeVersionComparison enabled - testing hybrid comparison with build metadata fallback")
result := compare.ByVersionAndRelease(b1, b2)
assert.Positive(t, result, "Bundle.Compare() returns 0 for registry+v1, fallback to build metadata: 1.0.0+2 > 1.0.0+1")

// Test bundles with explicit pkg.Release field
explicitR1 := declcfg.Bundle{
Name: "pkg.v2.0.0-r1",
Properties: []property.Property{
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "pkg", "version": "2.0.0", "release": "1"}`)},
},
}
explicitR2 := declcfg.Bundle{
Name: "pkg.v2.0.0-r2",
Properties: []property.Property{
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "pkg", "version": "2.0.0", "release": "2"}`)},
},
}
result = compare.ByVersionAndRelease(explicitR1, explicitR2)
assert.Positive(t, result, "2.0.0+release.2 > 2.0.0+release.1 (explicit release field)")
} else {
t.Log("CompositeVersionComparison disabled - testing legacy build metadata comparison only")
result := compare.ByVersionAndRelease(b1, b2)
assert.Positive(t, result, "should sort by build metadata: 1.0.0+2 > 1.0.0+1")
}
Comment on lines +192 to +217
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is conditional on the ambient CompositeVersionComparison feature-gate value, so it may silently skip validating the gated comparison path (and can behave differently depending on prior tests that mutate the global gate). Please make it deterministic by explicitly toggling the gate within the test (e.g., subtests for enabled/disabled with Set()+t.Cleanup restoring previous state) and asserting the expected ordering in both cases.

Copilot uses AI. Check for mistakes.
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,21 @@ func InAnyChannel(channels ...declcfg.Channel) filter.Predicate[declcfg.Bundle]
return false
}
}

// SameVersionHigherRelease returns a predicate that matches bundles with the same
// semantic version as the provided version-release, but with a higher release value.
// This is used to identify re-released bundles (e.g., 2.0.0+2 when 2.0.0+1 is installed).
func SameVersionHigherRelease(expect bundle.VersionRelease) filter.Predicate[declcfg.Bundle] {
return func(b declcfg.Bundle) bool {
actual, err := bundleutil.GetVersionAndRelease(b)
if err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could failing silently here lead to some hard to spot bugs?

return false
}

if expect.Version.Compare(actual.Version) != 0 {
return false
}

return expect.Release.Compare(actual.Release) < 0
}
Comment on lines +55 to +67
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SameVersionHigherRelease() relies on bundleutil.GetVersionAndRelease(), which currently only parses legacy registry+v1 "release" from semver build metadata (see bundleutil/bundle.go TODO). With CompositeVersionComparison enabled, bundles that use an explicit pkg.Release field (and a clean semver Version like "1.0.0") will never be recognized as higher-release successors. Consider updating the version/release extraction used here to understand pkg.Release (e.g., via declcfg.Bundle.Compare/CompositeVersion or by parsing the olm.package property’s release field) so successor detection works for the new bundle format.

Copilot uses AI. Check for mistakes.
}
Loading
Loading