From 1d1b7d6d064d63ab8e3154c13344ff8ac9f17233 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Mon, 15 Jun 2026 13:55:09 -0400 Subject: [PATCH 1/3] Consolidate Release types with operator-registry Signed-off-by: Rashmi Gottipati --- .../bundle/versionrelease.go | 150 ------ .../bundle/versionrelease_test.go | 470 ------------------ .../operator-controller/bundleutil/bundle.go | 92 +++- .../bundleutil/bundle_test.go | 45 +- .../catalogmetadata/compare/compare.go | 2 +- .../filter/bundle_predicates.go | 5 +- .../catalogmetadata/filter/successors.go | 50 +- .../catalogmetadata/filter/successors_test.go | 5 +- .../clusterextension_controller_test.go | 67 +-- .../operator-controller/resolve/catalog.go | 3 +- .../resolve/catalog_test.go | 27 +- .../operator-controller/resolve/resolver.go | 7 +- 12 files changed, 191 insertions(+), 732 deletions(-) delete mode 100644 internal/operator-controller/bundle/versionrelease.go delete mode 100644 internal/operator-controller/bundle/versionrelease_test.go diff --git a/internal/operator-controller/bundle/versionrelease.go b/internal/operator-controller/bundle/versionrelease.go deleted file mode 100644 index 48ab9d0a16..0000000000 --- a/internal/operator-controller/bundle/versionrelease.go +++ /dev/null @@ -1,150 +0,0 @@ -package bundle - -import ( - "errors" - "fmt" - "strings" - - bsemver "github.com/blang/semver/v4" - - slicesutil "github.com/operator-framework/operator-controller/internal/shared/util/slices" -) - -// NewLegacyRegistryV1VersionRelease parses a registry+v1 bundle version string and returns a -// VersionRelease. Some registry+v1 bundles utilize the build metadata field of the semver version -// as release information (a semver spec violation maintained for backward compatibility). When the -// bundle version includes build metadata that is parsable as a release, the returned -// VersionRelease has the build metadata extracted into the Release field, and the Version field -// has its Build metadata cleared. When the bundle version includes build metadata that is NOT -// parseable as a release, the returned VersionRelease has its Version set to the full semver -// version (with build metadata) and its Release left empty. -func NewLegacyRegistryV1VersionRelease(vStr string) (*VersionRelease, error) { - vers, err := bsemver.Parse(vStr) - if err != nil { - return nil, err - } - - vr := &VersionRelease{ - Version: vers, - } - - rel, err := NewRelease(strings.Join(vr.Version.Build, ".")) - if err == nil { - // If the version build metadata parses successfully as a release - // then use it as a release and drop the build metadata - // - // If we don't parse the build metadata as a release successfully, - // that doesn't mean we have an invalid version. It just means - // that we have a valid semver version with valid build metadata, - // but no release value. In this case, we return a VersionRelease - // with: - // - Version: the full version (with build metadata) - // - Release: - vr.Release = rel - vr.Version.Build = nil - } - return vr, nil -} - -type VersionRelease struct { - Version bsemver.Version - Release Release -} - -// Compare compares two VersionRelease values. It returns: -// -// -1 if vr < other -// 0 if vr == other -// +1 if vr > other -// -// Comparison is done first by Version, then by Release if versions are equal. -func (vr *VersionRelease) Compare(other VersionRelease) int { - if vCmp := vr.Version.Compare(other.Version); vCmp != 0 { - return vCmp - } - return vr.Release.Compare(other.Release) -} - -// AsLegacyRegistryV1Version converts a VersionRelease into a standard semver version. -// If the VersionRelease's Release field is set, the returned semver version's build -// metadata is set to the VersionRelease's Release. Otherwise, the build metadata is -// set to the VersionRelease's Version field's build metadata. -func (vr *VersionRelease) AsLegacyRegistryV1Version() bsemver.Version { - v := bsemver.Version{ - Major: vr.Version.Major, - Minor: vr.Version.Minor, - Patch: vr.Version.Patch, - Pre: vr.Version.Pre, - Build: vr.Version.Build, - } - if len(vr.Release) > 0 { - v.Build = slicesutil.Map(vr.Release, func(i bsemver.PRVersion) string { return i.String() }) - } - return v -} - -type Release []bsemver.PRVersion - -// String returns the string representation of the release. -// Returns an empty string if the release is nil or empty. -func (r Release) String() string { - if len(r) == 0 { - return "" - } - parts := make([]string, len(r)) - for i, pr := range r { - parts[i] = pr.String() - } - return strings.Join(parts, ".") -} - -// Compare compares two Release values. It returns: -// -// -1 if r < other -// 0 if r == other -// +1 if r > other -// -// Comparison is done segment by segment from left to right. Numeric segments are -// compared numerically, and alphanumeric segments are compared lexically in ASCII -// sort order. A shorter release is considered less than a longer release if all -// corresponding segments are equal. -func (r Release) Compare(other Release) int { - if len(r) == 0 && len(other) > 0 { - return -1 - } - if len(other) == 0 && len(r) > 0 { - return 1 - } - a := bsemver.Version{Pre: r} - b := bsemver.Version{Pre: other} - return a.Compare(b) -} - -// NewRelease parses a release string into a Release. The release string should be -// a dot-separated sequence of non-empty identifiers, where each identifier contains -// only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Numeric identifiers (those -// containing only digits) must not have leading zeros. An empty string returns a nil -// Release. Returns an error if any segment is invalid. -func NewRelease(relStr string) (Release, error) { - if relStr == "" { - return nil, nil - } - - var ( - segments = strings.Split(relStr, ".") - r = make(Release, 0, len(segments)) - errs []error - ) - for i, segment := range segments { - prVer, err := bsemver.NewPRVersion(segment) - if err != nil { - errs = append(errs, fmt.Errorf("segment %d: %v", i, err)) - continue - } - r = append(r, prVer) - } - if err := errors.Join(errs...); err != nil { - return nil, fmt.Errorf("invalid release %q: %v", relStr, err) - } - return r, nil -} diff --git a/internal/operator-controller/bundle/versionrelease_test.go b/internal/operator-controller/bundle/versionrelease_test.go deleted file mode 100644 index e54b1ba3b5..0000000000 --- a/internal/operator-controller/bundle/versionrelease_test.go +++ /dev/null @@ -1,470 +0,0 @@ -package bundle_test - -import ( - "testing" - - bsemver "github.com/blang/semver/v4" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" -) - -func TestNewLegacyRegistryV1VersionRelease(t *testing.T) { - type testCase struct { - name string - input string - expect func(*testing.T, *bundle.VersionRelease, error) - } - for _, tc := range []testCase{ - { - name: "empty input", - input: "", - expect: func(t *testing.T, _ *bundle.VersionRelease, err error) { - assert.Error(t, err) - }, - }, - { - name: "v-prefix is invalid", - input: "v1.0.0", - expect: func(t *testing.T, _ *bundle.VersionRelease, err error) { - assert.Error(t, err) - }, - }, - { - name: "valid semver, valid build metadata, but not a valid release", - input: "1.2.3-alpha+4.005", - expect: func(t *testing.T, vr *bundle.VersionRelease, err error) { - require.NoError(t, err) - assert.Equal(t, uint64(1), vr.Version.Major) - assert.Equal(t, uint64(2), vr.Version.Minor) - assert.Equal(t, uint64(3), vr.Version.Patch) - assert.Equal(t, []bsemver.PRVersion{{VersionStr: "alpha"}}, vr.Version.Pre) - assert.Equal(t, []string{"4", "005"}, vr.Version.Build) - assert.Empty(t, vr.Release) - }, - }, - { - name: "valid semver, valid build metadata, valid release", - input: "1.2.3-alpha+4.5", - expect: func(t *testing.T, vr *bundle.VersionRelease, err error) { - require.NoError(t, err) - assert.Equal(t, uint64(1), vr.Version.Major) - assert.Equal(t, uint64(2), vr.Version.Minor) - assert.Equal(t, uint64(3), vr.Version.Patch) - assert.Equal(t, []bsemver.PRVersion{{VersionStr: "alpha"}}, vr.Version.Pre) - assert.Empty(t, vr.Version.Build) - assert.Equal(t, bundle.Release([]bsemver.PRVersion{ - {VersionNum: 4, IsNum: true}, - {VersionNum: 5, IsNum: true}, - }), vr.Release) - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - actual, err := bundle.NewLegacyRegistryV1VersionRelease(tc.input) - tc.expect(t, actual, err) - }) - } -} - -func TestVersionRelease_AsLegacyRegistryV1Version(t *testing.T) { - type testCase struct { - name string - input bundle.VersionRelease - expect bsemver.Version - } - for _, tc := range []testCase{ - { - name: "Release is set, so release is set as build metadata", - input: bundle.VersionRelease{ - Version: bsemver.Version{ - Major: 1, - Minor: 2, - Patch: 3, - }, - Release: bundle.Release([]bsemver.PRVersion{{VersionStr: "release"}}), - }, - expect: bsemver.Version{ - Major: 1, - Minor: 2, - Patch: 3, - Build: []string{"release"}, - }, - }, - { - name: "Release is unset, so version build metadata is set as build metadata", - input: bundle.VersionRelease{ - Version: bsemver.Version{ - Major: 1, - Minor: 2, - Patch: 3, - Build: []string{"buildmetadata"}, - }, - }, - expect: bsemver.Version{ - Major: 1, - Minor: 2, - Patch: 3, - Build: []string{"buildmetadata"}, - }}, - } { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expect, tc.input.AsLegacyRegistryV1Version()) - }) - } -} - -func TestLegacyRegistryV1VersionRelease_Compare(t *testing.T) { - type testCase struct { - name string - v1 string - v2 string - expect int - } - for _, tc := range []testCase{ - { - name: "lower major version", - v1: "1.0.0-0+0", - v2: "2.0.0-0+0", - expect: -1, - }, - { - name: "lower minor version", - v1: "0.1.0-0+0", - v2: "0.2.0-0+0", - expect: -1, - }, - { - name: "lower patch version", - v1: "0.0.1-0+0", - v2: "0.0.2-0+0", - expect: -1, - }, - { - name: "lower prerelease version", - v1: "0.0.0-1+0", - v2: "0.0.0-2+0", - expect: -1, - }, - { - name: "lower build metadata", - v1: "0.0.0-0+1", - v2: "0.0.0-0+2", - expect: -1, - }, - { - name: "same major version", - v1: "1.0.0-0+0", - v2: "1.0.0-0+0", - expect: 0, - }, - { - name: "same minor version", - v1: "0.1.0-0+0", - v2: "0.1.0-0+0", - expect: 0, - }, - { - name: "same patch version", - v1: "0.0.1-0+0", - v2: "0.0.1-0+0", - expect: 0, - }, - { - name: "same prerelease version", - v1: "0.0.0-1+0", - v2: "0.0.0-1+0", - expect: 0, - }, - { - name: "same build metadata", - v1: "0.0.0-0+1", - v2: "0.0.0-0+1", - expect: 0, - }, - { - name: "higher major version", - v1: "2.0.0-0+0", - v2: "1.0.0-0+0", - expect: 1, - }, - { - name: "higher minor version", - v1: "0.2.0-0+0", - v2: "0.1.0-0+0", - expect: 1, - }, - { - name: "higher patch version", - v1: "0.0.2-0+0", - v2: "0.0.1-0+0", - expect: 1, - }, - { - name: "higher prerelease version", - v1: "0.0.0-2+0", - v2: "0.0.0-1+0", - expect: 1, - }, - { - name: "higher build metadata", - v1: "0.0.0-0+2", - v2: "0.0.0-0+1", - expect: 1, - }, - { - name: "non-release build metadata is less than valid release build metadata", - v1: "0.0.0-0+1.001", - v2: "0.0.0-0+0", - expect: -1, - }, - } { - t.Run(tc.name, func(t *testing.T) { - vr1, err1 := bundle.NewLegacyRegistryV1VersionRelease(tc.v1) - vr2, err2 := bundle.NewLegacyRegistryV1VersionRelease(tc.v2) - require.NoError(t, err1) - require.NoError(t, err2) - - actual := vr1.Compare(*vr2) - assert.Equal(t, tc.expect, actual) - }) - } -} - -func TestNewRelease(t *testing.T) { - type testCase struct { - name string - input string - expectErr bool - } - for _, tc := range []testCase{ - { - name: "empty string", - input: "", - expectErr: false, - }, - { - name: "single numeric segment", - input: "9", - expectErr: false, - }, - { - name: "single alphanumeric segment", - input: "alpha", - expectErr: false, - }, - { - name: "multiple segments", - input: "9.10.3", - expectErr: false, - }, - { - name: "mixed numeric and alphanumeric", - input: "9.alpha.10", - expectErr: false, - }, - { - name: "segment with hyphens in middle", - input: "alpha-beta", - expectErr: false, - }, - { - name: "segment starting with hyphen", - input: "-alpha", - expectErr: false, - }, - { - name: "segment ending with hyphen", - input: "alpha-", - expectErr: false, - }, - { - name: "segment with only hyphens", - input: "--", - expectErr: false, - }, - { - name: "numeric segment with leading zero (single)", - input: "0", - expectErr: false, - }, - { - name: "numeric segment with leading zeros", - input: "01", - expectErr: true, - }, - { - name: "alphanumeric segment with leading zeros", - input: "01alpha", - expectErr: false, - }, - { - name: "alphanumeric segment with number prefix", - input: "pre9", - expectErr: false, - }, - { - name: "alphanumeric segment with number suffix", - input: "9pre", - expectErr: false, - }, - { - name: "multiple segments with one having leading zeros", - input: "9.010.3", - expectErr: true, - }, - { - name: "empty segment at start", - input: ".alpha", - expectErr: true, - }, - { - name: "empty segment at end", - input: "alpha.", - expectErr: true, - }, - { - name: "empty segment in middle", - input: "alpha..beta", - expectErr: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - result, err := bundle.NewRelease(tc.input) - if tc.expectErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) - if tc.input == "" { - assert.Nil(t, result) - } else { - assert.NotNil(t, result) - } - } - }) - } -} - -func TestRelease_Compare(t *testing.T) { - type testCase struct { - name string - r1 string - r2 string - expect int - } - for _, tc := range []testCase{ - { - name: "both empty", - r1: "", - r2: "", - expect: 0, - }, - { - name: "first empty, second not", - r1: "", - r2: "9", - expect: -1, - }, - { - name: "first not empty, second empty", - r1: "9", - r2: "", - expect: 1, - }, - { - name: "equal numeric segments", - r1: "9", - r2: "9", - expect: 0, - }, - { - name: "lower numeric segment", - r1: "9", - r2: "10", - expect: -1, - }, - { - name: "higher numeric segment", - r1: "10", - r2: "9", - expect: 1, - }, - { - name: "equal alphanumeric segments", - r1: "alpha", - r2: "alpha", - expect: 0, - }, - { - name: "lower alphanumeric segment", - r1: "alpha", - r2: "beta", - expect: -1, - }, - { - name: "higher alphanumeric segment", - r1: "beta", - r2: "alpha", - expect: 1, - }, - { - name: "numeric vs alphanumeric (numeric is less)", - r1: "9", - r2: "alpha", - expect: -1, - }, - { - name: "alphanumeric vs numeric (alphanumeric is greater)", - r1: "alpha", - r2: "9", - expect: 1, - }, - { - name: "shorter release (all segments equal)", - r1: "9.10", - r2: "9.10.3", - expect: -1, - }, - { - name: "longer release (all segments equal)", - r1: "9.10.3", - r2: "9.10", - expect: 1, - }, - { - name: "complex equal releases", - r1: "9.alpha.10.beta", - r2: "9.alpha.10.beta", - expect: 0, - }, - { - name: "segment with hyphens", - r1: "alpha-beta", - r2: "alpha-gamma", - expect: -1, - }, - { - name: "alphanumeric with numbers prefix (lexicographic)", - r1: "pre9", - r2: "pre10", - expect: 1, // "pre9" > "pre10" lexicographically - }, - { - name: "alphanumeric with numbers suffix (lexicographic)", - r1: "9pre", - r2: "10pre", - expect: 1, // "9pre" > "10pre" lexicographically - }, - } { - t.Run(tc.name, func(t *testing.T) { - rel1, err := bundle.NewRelease(tc.r1) - require.NoError(t, err) - rel2, err := bundle.NewRelease(tc.r2) - require.NoError(t, err) - - actual := rel1.Compare(rel2) - assert.Equal(t, tc.expect, actual) - }) - } -} diff --git a/internal/operator-controller/bundleutil/bundle.go b/internal/operator-controller/bundleutil/bundle.go index 956feba539..bf0dbdbe02 100644 --- a/internal/operator-controller/bundleutil/bundle.go +++ b/internal/operator-controller/bundleutil/bundle.go @@ -10,11 +10,10 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) -func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) { +func GetVersionAndRelease(b declcfg.Bundle) (*declcfg.VersionRelease, error) { for _, p := range b.Properties { if p.Type == property.TypePackage { return parseVersionRelease(p.Value) @@ -23,7 +22,51 @@ func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) { return nil, fmt.Errorf("no package property found in bundle %q", b.Name) } -func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error) { +// newLegacyRegistryV1VersionRelease parses a registry+v1 bundle version string and returns a +// VersionRelease. Some registry+v1 bundles utilize the build metadata field of the semver version +// as release information (a semver spec violation maintained for backward compatibility). When the +// bundle version includes build metadata that is parsable as a release, the returned +// VersionRelease has the build metadata extracted into the Release field, and the Version field +// has its Build metadata cleared. When the bundle version includes build metadata that is NOT +// parseable as a release, the returned VersionRelease has its Version set to the full semver +// version (with build metadata) and its Release left empty. +func newLegacyRegistryV1VersionRelease(vStr string) (*declcfg.VersionRelease, error) { + vers, err := bsemver.Parse(vStr) + if err != nil { + return nil, err + } + + vr := &declcfg.VersionRelease{ + Version: vers, + } + + buildMetadata := "" + if len(vr.Version.Build) > 0 { + buildMetadata = vr.Version.Build[0] + for i := 1; i < len(vr.Version.Build); i++ { + buildMetadata += "." + vr.Version.Build[i] + } + } + + rel, err := declcfg.NewRelease(buildMetadata) + if err == nil && len(rel) > 0 { + // If the version build metadata parses successfully as a release + // then use it as a release and drop the build metadata + // + // If we don't parse the build metadata as a release successfully, + // that doesn't mean we have an invalid version. It just means + // that we have a valid semver version with valid build metadata, + // but no release value. In this case, we return a VersionRelease + // with: + // - Version: the full version (with build metadata) + // - Release: + vr.Release = rel + vr.Version.Build = nil + } + return vr, nil +} + +func parseVersionRelease(pkgData json.RawMessage) (*declcfg.VersionRelease, error) { var pkg property.Package if err := json.Unmarshal(pkgData, &pkg); err != nil { return nil, fmt.Errorf("error unmarshalling package property: %w", err) @@ -50,43 +93,39 @@ func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error // In the future, for supporting other bundle formats, we should not // use the legacy registry+v1 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 newLegacyRegistryV1VersionRelease(pkg.Version) } // 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 +// 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) { +func parseExplicitRelease(version, releaseStr string) (*declcfg.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 + var rel declcfg.Release if releaseStr == "" { // Explicit empty release: use empty slice (not nil) - rel = bundle.Release([]bsemver.PRVersion{}) + rel = declcfg.Release([]bsemver.PRVersion{}) } else { - rel, err = bundle.NewRelease(releaseStr) + rel, err = declcfg.NewRelease(releaseStr) if err != nil { return nil, fmt.Errorf("error parsing release %q: %w", releaseStr, err) } } - return &bundle.VersionRelease{ + return &declcfg.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 { +func MetadataFor(bundleName string, vr declcfg.VersionRelease) ocv1.BundleMetadata { if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) { // New behavior: separate Version and Release fields bm := ocv1.BundleMetadata{ @@ -104,6 +143,27 @@ func MetadataFor(bundleName string, vr bundle.VersionRelease) ocv1.BundleMetadat // the Release field is pruned by the API server. return ocv1.BundleMetadata{ Name: bundleName, - Version: vr.AsLegacyRegistryV1Version().String(), + Version: asLegacyRegistryV1Version(vr).String(), + } +} + +// asLegacyRegistryV1Version converts a VersionRelease into a standard semver version. +// If the VersionRelease's Release field is set, the returned semver version's build +// metadata is set to the VersionRelease's Release. Otherwise, the build metadata is +// set to the VersionRelease's Version field's build metadata. +func asLegacyRegistryV1Version(vr declcfg.VersionRelease) bsemver.Version { + v := bsemver.Version{ + Major: vr.Version.Major, + Minor: vr.Version.Minor, + Patch: vr.Version.Patch, + Pre: vr.Version.Pre, + Build: vr.Version.Build, + } + if len(vr.Release) > 0 { + v.Build = make([]string, len(vr.Release)) + for i, pr := range vr.Release { + v.Build[i] = pr.String() + } } + return v } diff --git a/internal/operator-controller/bundleutil/bundle_test.go b/internal/operator-controller/bundleutil/bundle_test.go index 925604a8bf..05b528fe76 100644 --- a/internal/operator-controller/bundleutil/bundle_test.go +++ b/internal/operator-controller/bundleutil/bundle_test.go @@ -11,7 +11,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) @@ -20,7 +19,7 @@ func TestGetVersionAndRelease(t *testing.T) { tests := []struct { name string pkgProperty *property.Property - wantVersionRelease *bundle.VersionRelease + wantVersionRelease *declcfg.VersionRelease wantErr bool }{ { @@ -29,9 +28,9 @@ func TestGetVersionAndRelease(t *testing.T) { Type: property.TypePackage, Value: json.RawMessage(`{"version": "1.0.0-pre+1.alpha.2"}`), }, - wantVersionRelease: &bundle.VersionRelease{ + wantVersionRelease: &declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0-pre"), - Release: bundle.Release([]bsemver.PRVersion{ + Release: declcfg.Release([]bsemver.PRVersion{ {VersionNum: 1, IsNum: true}, {VersionStr: "alpha"}, {VersionNum: 2, IsNum: true}, @@ -53,7 +52,7 @@ func TestGetVersionAndRelease(t *testing.T) { Type: property.TypePackage, Value: json.RawMessage(`{"version": "1.0.0+001"}`), }, - wantVersionRelease: &bundle.VersionRelease{ + wantVersionRelease: &declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0+001"), }, wantErr: false, @@ -110,7 +109,7 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { tests := []struct { name string pkgProperty *property.Property - wantVersionRelease *bundle.VersionRelease + wantVersionRelease *declcfg.VersionRelease wantErr bool }{ { @@ -119,9 +118,9 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { Type: property.TypePackage, Value: json.RawMessage(`{"version": "1.0.0+ignored", "release": "2"}`), }, - wantVersionRelease: &bundle.VersionRelease{ + wantVersionRelease: &declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0+ignored"), // Build metadata preserved - serves its proper semver purpose - Release: bundle.Release([]bsemver.PRVersion{ + Release: declcfg.Release([]bsemver.PRVersion{ {VersionNum: 2, IsNum: true}, }), }, @@ -133,9 +132,9 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { Type: property.TypePackage, Value: json.RawMessage(`{"version": "2.1.0", "release": "1.alpha.3"}`), }, - wantVersionRelease: &bundle.VersionRelease{ + wantVersionRelease: &declcfg.VersionRelease{ Version: bsemver.MustParse("2.1.0"), - Release: bundle.Release([]bsemver.PRVersion{ + Release: declcfg.Release([]bsemver.PRVersion{ {VersionNum: 1, IsNum: true}, {VersionStr: "alpha"}, {VersionNum: 3, IsNum: true}, @@ -157,9 +156,9 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { 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 + wantVersionRelease: &declcfg.VersionRelease{ + Version: bsemver.MustParse("1.0.0+2"), // Build metadata preserved (not extracted as release) + Release: declcfg.Release([]bsemver.PRVersion{}), // Explicit empty release }, wantErr: false, }, @@ -206,9 +205,9 @@ func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { require.NoError(t, err) // Should parse build metadata (+2), not explicit release field (999) - expected := &bundle.VersionRelease{ + expected := &declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), - Release: bundle.Release([]bsemver.PRVersion{ + Release: declcfg.Release([]bsemver.PRVersion{ {VersionNum: 2, IsNum: true}, }), } @@ -225,9 +224,9 @@ func TestMetadataFor(t *testing.T) { }) t.Run("with release", func(t *testing.T) { - vr := bundle.VersionRelease{ + vr := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), - Release: bundle.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}), + Release: declcfg.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}), } result := bundleutil.MetadataFor("test-bundle", vr) require.Equal(t, "test-bundle", result.Name) @@ -237,7 +236,7 @@ func TestMetadataFor(t *testing.T) { }) t.Run("without release", func(t *testing.T) { - vr := bundle.VersionRelease{ + vr := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), Release: nil, } @@ -248,9 +247,9 @@ func TestMetadataFor(t *testing.T) { }) t.Run("with explicit empty release", func(t *testing.T) { - vr := bundle.VersionRelease{ + vr := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), - Release: bundle.Release([]bsemver.PRVersion{}), + Release: declcfg.Release([]bsemver.PRVersion{}), } result := bundleutil.MetadataFor("test-bundle", vr) require.Equal(t, "test-bundle", result.Name) @@ -268,9 +267,9 @@ func TestMetadataFor(t *testing.T) { }) t.Run("reconstitutes build metadata in version", func(t *testing.T) { - vr := bundle.VersionRelease{ + vr := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), - Release: bundle.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}), + Release: declcfg.Release([]bsemver.PRVersion{{VersionNum: 2, IsNum: true}}), } result := bundleutil.MetadataFor("test-bundle", vr) require.Equal(t, "test-bundle", result.Name) @@ -279,7 +278,7 @@ func TestMetadataFor(t *testing.T) { }) t.Run("preserves original build metadata when no release", func(t *testing.T) { - vr := bundle.VersionRelease{ + vr := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), Release: nil, } diff --git a/internal/operator-controller/catalogmetadata/compare/compare.go b/internal/operator-controller/catalogmetadata/compare/compare.go index decab58953..1386735a5f 100644 --- a/internal/operator-controller/catalogmetadata/compare/compare.go +++ b/internal/operator-controller/catalogmetadata/compare/compare.go @@ -51,7 +51,7 @@ func ByVersionAndRelease(b1, b2 declcfg.Bundle) int { if err1 != nil || err2 != nil { return compareErrors(err2, err1) } - return vr2.Compare(*vr1) + return vr2.Compare(vr1) } func ByDeprecationFunc(deprecation declcfg.Deprecation) func(a, b declcfg.Bundle) int { diff --git a/internal/operator-controller/catalogmetadata/filter/bundle_predicates.go b/internal/operator-controller/catalogmetadata/filter/bundle_predicates.go index 56a17ff8ce..f9c6bb2240 100644 --- a/internal/operator-controller/catalogmetadata/filter/bundle_predicates.go +++ b/internal/operator-controller/catalogmetadata/filter/bundle_predicates.go @@ -5,7 +5,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/shared/util/filter" ) @@ -13,13 +12,13 @@ import ( // ExactVersionRelease returns a predicate that matches bundles with an exact // version and release match. Both the semver version and the release must match // exactly for the predicate to return true. -func ExactVersionRelease(expect bundle.VersionRelease) filter.Predicate[declcfg.Bundle] { +func ExactVersionRelease(expect declcfg.VersionRelease) filter.Predicate[declcfg.Bundle] { return func(b declcfg.Bundle) bool { actual, err := bundleutil.GetVersionAndRelease(b) if err != nil { return false } - return expect.Compare(*actual) == 0 + return expect.Compare(actual) == 0 } } diff --git a/internal/operator-controller/catalogmetadata/filter/successors.go b/internal/operator-controller/catalogmetadata/filter/successors.go index 969beea72d..6846fcc0b9 100644 --- a/internal/operator-controller/catalogmetadata/filter/successors.go +++ b/internal/operator-controller/catalogmetadata/filter/successors.go @@ -8,25 +8,20 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/shared/util/filter" ) // 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 not nil, 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) { +func parseInstalledBundleVersionRelease(installedBundle ocv1.BundleMetadata) (*declcfg.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 get version and release of installed bundle: %w", err) - } - return vr, nil + return newLegacyRegistryV1VersionRelease(installedBundle.Version) } // 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 + // 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". @@ -35,24 +30,53 @@ func parseInstalledBundleVersionRelease(installedBundle ocv1.BundleMetadata) (*b return nil, fmt.Errorf("failed to parse installed bundle version %q: %w", installedBundle.Version, err) } - var release bundle.Release + var release declcfg.Release if *installedBundle.Release == "" { // Explicit empty release: use empty slice (not nil) to match catalog parsing behavior. // NewRelease("") returns nil, but we need empty slice for roundtrip correctness. - release = bundle.Release([]bsemver.PRVersion{}) + release = declcfg.Release([]bsemver.PRVersion{}) } else { - release, err = bundle.NewRelease(*installedBundle.Release) + release, err = declcfg.NewRelease(*installedBundle.Release) if err != nil { return nil, fmt.Errorf("failed to parse installed bundle release %q: %w", *installedBundle.Release, err) } } - return &bundle.VersionRelease{ + return &declcfg.VersionRelease{ Version: version, Release: release, }, nil } +// newLegacyRegistryV1VersionRelease parses a registry+v1 bundle version string and returns a +// VersionRelease. Some registry+v1 bundles utilize the build metadata field of the semver version +// as release information (a semver spec violation maintained for backward compatibility). +func newLegacyRegistryV1VersionRelease(vStr string) (*declcfg.VersionRelease, error) { + vers, err := bsemver.Parse(vStr) + if err != nil { + return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err) + } + + vr := &declcfg.VersionRelease{ + Version: vers, + } + + buildMetadata := "" + if len(vr.Version.Build) > 0 { + buildMetadata = vr.Version.Build[0] + for i := 1; i < len(vr.Version.Build); i++ { + buildMetadata += "." + vr.Version.Build[i] + } + } + + rel, err := declcfg.NewRelease(buildMetadata) + if err == nil && len(rel) > 0 { + vr.Release = rel + vr.Version.Build = nil + } + return vr, nil +} + func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Channel) (filter.Predicate[declcfg.Bundle], error) { installedVersionRelease, err := parseInstalledBundleVersionRelease(installedBundle) if err != nil { diff --git a/internal/operator-controller/catalogmetadata/filter/successors_test.go b/internal/operator-controller/catalogmetadata/filter/successors_test.go index ed2508a7b5..360546c6ff 100644 --- a/internal/operator-controller/catalogmetadata/filter/successors_test.go +++ b/internal/operator-controller/catalogmetadata/filter/successors_test.go @@ -13,7 +13,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare" "github.com/operator-framework/operator-controller/internal/shared/util/filter" @@ -21,8 +20,8 @@ import ( // mustVersionRelease is a test helper that parses a version string into a VersionRelease. // For registry+v1 bundles, build metadata is interpreted as release (e.g., "1.0.0+2" -> Version: 1.0.0, Release: 2). -func mustVersionRelease(versionStr string) bundle.VersionRelease { - vr, err := bundle.NewLegacyRegistryV1VersionRelease(versionStr) +func mustVersionRelease(versionStr string) declcfg.VersionRelease { + vr, err := newLegacyRegistryV1VersionRelease(versionStr) if err != nil { panic(err) } diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index cec683db8a..b976bffbe6 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -31,7 +31,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/features" @@ -130,7 +129,7 @@ func TestClusterExtensionShortCircuitsReconcileDuringDeletion(t *testing.T) { func TestClusterExtensionResolutionFails(t *testing.T) { pkgName := fmt.Sprintf("non-existent-%s", rand.String(6)) cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return nil, nil, nil, fmt.Errorf("no package %q found", pkgName) }) }) @@ -195,7 +194,7 @@ func TestClusterExtensionResolutionFailsWithDeprecationData(t *testing.T) { pkgName := fmt.Sprintf("deprecated-%s", rand.String(6)) deprecationMessage := "package marked deprecated in catalog" cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return nil, nil, &declcfg.Deprecation{ Entries: []declcfg.DeprecationEntry{{ Reference: declcfg.PackageScopedReference{Schema: declcfg.SchemaPackage}, @@ -261,8 +260,8 @@ func TestClusterExtensionUpgradeShowsInstalledBundleDeprecation(t *testing.T) { deprecationMessage := "v1.0.0 is deprecated, please upgrade to v2.0.0" cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("2.0.0"), } // Catalog has deprecation for v1.0.0 (installed), but v2.0.0 (resolved) is NOT deprecated @@ -362,8 +361,8 @@ func TestClusterExtensionUpgradeFromDeprecatedBundleClearsDeprecation(t *testing deprecationMessage := fmt.Sprintf("%s is deprecated. Uninstall and install v1.0.3 for support.", installedBundleName) cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.3"), } return &declcfg.Bundle{ @@ -459,7 +458,7 @@ func TestClusterExtensionResolutionFailsWithoutCatalogDeprecationData(t *testing catalogName := fmt.Sprintf("test-catalog-%s", rand.String(6)) installedBundleName := fmt.Sprintf("%s.v1.0.0", pkgName) cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return nil, nil, nil, fmt.Errorf("no bundles found for package %q", pkgName) }) @@ -585,8 +584,8 @@ func TestClusterExtensionResolutionSuccessfulUnpackFails(t *testing.T) { } }, func(d *deps) { - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -662,8 +661,8 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -775,11 +774,13 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te cl, reconciler := newClientAndReconciler(t, func(d *deps) { // Boxcutter keeps a rolling revision when apply fails. We mirror that state so the test uses // the same inputs the runtime would see. - d.RevisionStatesGetter = newMockRevisionStatesGetter(gomock.NewController(t), &controllers.RevisionStates{ - RollingOut: []*controllers.RevisionMetadata{{}}, - }, nil) - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{}}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -1044,8 +1045,8 @@ func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -1134,8 +1135,8 @@ func TestClusterExtensionManagerFailed(t *testing.T) { d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -1210,8 +1211,8 @@ func TestClusterExtensionManagedContentCacheWatchFail(t *testing.T) { d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -1288,8 +1289,8 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -1367,8 +1368,8 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { d.ImagePuller = &imageutil.FakePuller{ ImageFS: fstest.MapFS{}, } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { + v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), } return &declcfg.Bundle{ @@ -2613,11 +2614,11 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { resolveAttempt := 0 cl, reconciler := newClientAndReconciler(t, func(d *deps) { // First reconcile: catalog available, second reconcile: catalog unavailable - d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { resolveAttempt++ if resolveAttempt == 1 { // First reconcile: catalog available, resolve to version 1.0.0 - v := bundle.VersionRelease{Version: bsemver.MustParse("1.0.0")} + v := declcfg.VersionRelease{Version: bsemver.MustParse("1.0.0")} return &declcfg.Bundle{ Name: "test.1.0.0", Package: "test-pkg", @@ -2711,7 +2712,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { t.Run("fails when version upgrade requested without catalog", func(t *testing.T) { cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return nil, nil, nil, fmt.Errorf("catalog unavailable") }) d.RevisionStatesGetter = newMockRevisionStatesGetter(gomock.NewController(t), &controllers.RevisionStates{ @@ -2769,14 +2770,14 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { resolveAttempt := 0 cl, reconciler := newClientAndReconciler(t, func(d *deps) { // First attempt: catalog unavailable, then becomes available - d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { resolveAttempt++ if resolveAttempt == 1 { // First reconcile: catalog unavailable return nil, nil, nil, fmt.Errorf("catalog temporarily unavailable") } // Second reconcile (triggered by catalog watch): catalog available with new version - v := bundle.VersionRelease{Version: bsemver.MustParse("2.0.0")} + v := declcfg.VersionRelease{Version: bsemver.MustParse("2.0.0")} return &declcfg.Bundle{ Name: "test.2.0.0", Package: "test-pkg", @@ -2865,7 +2866,7 @@ func TestResolutionFallbackToInstalledBundle(t *testing.T) { t.Run("retries when catalogs exist but resolution fails", func(t *testing.T) { cl, reconciler := newClientAndReconciler(t, func(d *deps) { // Resolver fails (transient issue) - d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + d.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return nil, nil, nil, fmt.Errorf("transient catalog issue: cache stale") }) d.RevisionStatesGetter = newMockRevisionStatesGetter(gomock.NewController(t), &controllers.RevisionStates{ diff --git a/internal/operator-controller/resolve/catalog.go b/internal/operator-controller/resolve/catalog.go index f4cb38071d..98a34b5ca4 100644 --- a/internal/operator-controller/resolve/catalog.go +++ b/internal/operator-controller/resolve/catalog.go @@ -17,7 +17,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/filter" @@ -39,7 +38,7 @@ type foundBundle struct { } // Resolve returns a Bundle from a catalog that needs to get installed on the cluster. -func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { +func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { l := log.FromContext(ctx) packageName := ext.Spec.Source.Catalog.PackageName versionRange := ext.Spec.Source.Catalog.Version diff --git a/internal/operator-controller/resolve/catalog_test.go b/internal/operator-controller/resolve/catalog_test.go index 27f158dd53..a20f6619f6 100644 --- a/internal/operator-controller/resolve/catalog_test.go +++ b/internal/operator-controller/resolve/catalog_test.go @@ -19,7 +19,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" ) func TestInvalidClusterExtensionVersionRange(t *testing.T) { @@ -90,7 +89,7 @@ func TestPackageExists(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "3.0.0"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("3.0.0")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("3.0.0")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -157,7 +156,7 @@ func TestVersionExists(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "1.0.2"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -198,7 +197,7 @@ func TestChannelExists(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "1.0.2"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -258,7 +257,7 @@ func TestChannelAndVersionExist(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "0.1.0"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -280,7 +279,7 @@ func TestPreferNonDeprecated(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "0.1.0"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -302,7 +301,7 @@ func TestAcceptDeprecated(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "1.0.1"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.1")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.1")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -386,7 +385,7 @@ func TestPackageVariationsBetweenCatalogs(t *testing.T) { require.NoError(t, err) // We choose the only non-deprecated package assert.Equal(t, genBundle(pkgName, "1.0.2").Name, gotBundle.Name) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) assert.Equal(t, (*declcfg.Deprecation)(nil), gotDeprecation) }) @@ -418,7 +417,7 @@ func TestPackageVariationsBetweenCatalogs(t *testing.T) { require.NoError(t, err) // Bundles within one catalog for a package will be sorted by semver and deprecation and the best is returned assert.Equal(t, genBundle(pkgName, "1.0.5").Name, gotBundle.Name) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.5")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.5")}, *gotVersion) assert.Equal(t, (*declcfg.Deprecation)(nil), gotDeprecation) }) } @@ -446,7 +445,7 @@ func TestUpgradeFoundLegacy(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, installedBundle) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "1.0.2"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.2")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -498,7 +497,7 @@ func TestDowngradeFound(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, installedBundle) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "0.1.0"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("0.1.0")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -855,7 +854,7 @@ func TestUnequalPriority(t *testing.T) { ce := buildFooClusterExtension(pkgName, []string{}, "", ocv1.UpgradeConstraintPolicyCatalogProvided) _, gotVersion, _, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) - require.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("1.0.0")}, *gotVersion) + require.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("1.0.0")}, *gotVersion) } func TestMultiplePriority(t *testing.T) { @@ -900,7 +899,7 @@ func TestMultipleChannels(t *testing.T) { gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) assert.Equal(t, genBundle(pkgName, "2.0.0"), *gotBundle) - assert.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("2.0.0")}, *gotVersion) + assert.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("2.0.0")}, *gotVersion) assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation) } @@ -973,5 +972,5 @@ func TestSomeCatalogsDisabled(t *testing.T) { gotBundle, gotVersion, _, err := r.Resolve(context.Background(), ce, nil) require.NoError(t, err) require.NotNil(t, gotBundle) - require.Equal(t, bundle.VersionRelease{Version: bsemver.MustParse("3.0.0")}, *gotVersion) + require.Equal(t, declcfg.VersionRelease{Version: bsemver.MustParse("3.0.0")}, *gotVersion) } diff --git a/internal/operator-controller/resolve/resolver.go b/internal/operator-controller/resolve/resolver.go index 1fbde0fdea..ef7543b5c8 100644 --- a/internal/operator-controller/resolve/resolver.go +++ b/internal/operator-controller/resolve/resolver.go @@ -6,15 +6,14 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" ) type Resolver interface { - Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) + Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) } -type Func func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) +type Func func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) -func (f Func) Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { +func (f Func) Resolve(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { return f(ctx, ext, installedBundle) } From 21e1ef3d734d7add3d4a9de745b2f9ef2b8bf1a9 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Tue, 23 Jun 2026 23:20:33 -0400 Subject: [PATCH 2/3] address review comments Signed-off-by: Rashmi Gottipati --- .../operator-controller/bundleutil/bundle.go | 17 +++------ .../catalogmetadata/filter/successors.go | 38 ++++--------------- .../catalogmetadata/filter/successors_test.go | 2 +- .../clusterextension_controller_test.go | 8 ++-- 4 files changed, 17 insertions(+), 48 deletions(-) diff --git a/internal/operator-controller/bundleutil/bundle.go b/internal/operator-controller/bundleutil/bundle.go index bf0dbdbe02..4ee873a6af 100644 --- a/internal/operator-controller/bundleutil/bundle.go +++ b/internal/operator-controller/bundleutil/bundle.go @@ -3,6 +3,7 @@ package bundleutil import ( "encoding/json" "fmt" + "strings" bsemver "github.com/blang/semver/v4" @@ -22,7 +23,7 @@ func GetVersionAndRelease(b declcfg.Bundle) (*declcfg.VersionRelease, error) { return nil, fmt.Errorf("no package property found in bundle %q", b.Name) } -// newLegacyRegistryV1VersionRelease parses a registry+v1 bundle version string and returns a +// ParseLegacyVersionRelease parses a registry+v1 bundle version string and returns a // VersionRelease. Some registry+v1 bundles utilize the build metadata field of the semver version // as release information (a semver spec violation maintained for backward compatibility). When the // bundle version includes build metadata that is parsable as a release, the returned @@ -30,7 +31,7 @@ func GetVersionAndRelease(b declcfg.Bundle) (*declcfg.VersionRelease, error) { // has its Build metadata cleared. When the bundle version includes build metadata that is NOT // parseable as a release, the returned VersionRelease has its Version set to the full semver // version (with build metadata) and its Release left empty. -func newLegacyRegistryV1VersionRelease(vStr string) (*declcfg.VersionRelease, error) { +func ParseLegacyVersionRelease(vStr string) (*declcfg.VersionRelease, error) { vers, err := bsemver.Parse(vStr) if err != nil { return nil, err @@ -40,13 +41,7 @@ func newLegacyRegistryV1VersionRelease(vStr string) (*declcfg.VersionRelease, er Version: vers, } - buildMetadata := "" - if len(vr.Version.Build) > 0 { - buildMetadata = vr.Version.Build[0] - for i := 1; i < len(vr.Version.Build); i++ { - buildMetadata += "." + vr.Version.Build[i] - } - } + buildMetadata := strings.Join(vr.Version.Build, ".") rel, err := declcfg.NewRelease(buildMetadata) if err == nil && len(rel) > 0 { @@ -93,13 +88,13 @@ func parseVersionRelease(pkgData json.RawMessage) (*declcfg.VersionRelease, erro // In the future, for supporting other bundle formats, we should not // use the legacy registry+v1 mechanism (i.e. using build metadata in // the version) to determine the bundle's release. - return newLegacyRegistryV1VersionRelease(pkg.Version) + return ParseLegacyVersionRelease(pkg.Version) } // 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 +// In contrast, ParseLegacyVersionRelease clears build metadata because it // interprets build metadata AS the release value for registry+v1 bundles. func parseExplicitRelease(version, releaseStr string) (*declcfg.VersionRelease, error) { vers, err := bsemver.Parse(version) diff --git a/internal/operator-controller/catalogmetadata/filter/successors.go b/internal/operator-controller/catalogmetadata/filter/successors.go index 6846fcc0b9..c5e6ecc7f6 100644 --- a/internal/operator-controller/catalogmetadata/filter/successors.go +++ b/internal/operator-controller/catalogmetadata/filter/successors.go @@ -8,6 +8,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/shared/util/filter" ) @@ -17,11 +18,15 @@ import ( func parseInstalledBundleVersionRelease(installedBundle ocv1.BundleMetadata) (*declcfg.VersionRelease, error) { // Handle legacy registry+v1 format: release embedded in version's build metadata if installedBundle.Release == nil { - return newLegacyRegistryV1VersionRelease(installedBundle.Version) + vr, err := bundleutil.ParseLegacyVersionRelease(installedBundle.Version) + if err != nil { + return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err) + } + 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 + // Note: We can't use ParseLegacyVersionRelease 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". @@ -48,35 +53,6 @@ func parseInstalledBundleVersionRelease(installedBundle ocv1.BundleMetadata) (*d }, nil } -// newLegacyRegistryV1VersionRelease parses a registry+v1 bundle version string and returns a -// VersionRelease. Some registry+v1 bundles utilize the build metadata field of the semver version -// as release information (a semver spec violation maintained for backward compatibility). -func newLegacyRegistryV1VersionRelease(vStr string) (*declcfg.VersionRelease, error) { - vers, err := bsemver.Parse(vStr) - if err != nil { - return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err) - } - - vr := &declcfg.VersionRelease{ - Version: vers, - } - - buildMetadata := "" - if len(vr.Version.Build) > 0 { - buildMetadata = vr.Version.Build[0] - for i := 1; i < len(vr.Version.Build); i++ { - buildMetadata += "." + vr.Version.Build[i] - } - } - - rel, err := declcfg.NewRelease(buildMetadata) - if err == nil && len(rel) > 0 { - vr.Release = rel - vr.Version.Build = nil - } - return vr, nil -} - func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Channel) (filter.Predicate[declcfg.Bundle], error) { installedVersionRelease, err := parseInstalledBundleVersionRelease(installedBundle) if err != nil { diff --git a/internal/operator-controller/catalogmetadata/filter/successors_test.go b/internal/operator-controller/catalogmetadata/filter/successors_test.go index 360546c6ff..b1e6028a73 100644 --- a/internal/operator-controller/catalogmetadata/filter/successors_test.go +++ b/internal/operator-controller/catalogmetadata/filter/successors_test.go @@ -21,7 +21,7 @@ import ( // mustVersionRelease is a test helper that parses a version string into a VersionRelease. // For registry+v1 bundles, build metadata is interpreted as release (e.g., "1.0.0+2" -> Version: 1.0.0, Release: 2). func mustVersionRelease(versionStr string) declcfg.VersionRelease { - vr, err := newLegacyRegistryV1VersionRelease(versionStr) + vr, err := bundleutil.ParseLegacyVersionRelease(versionStr) if err != nil { panic(err) } diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index b976bffbe6..675305e858 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -774,11 +774,9 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te cl, reconciler := newClientAndReconciler(t, func(d *deps) { // Boxcutter keeps a rolling revision when apply fails. We mirror that state so the test uses // the same inputs the runtime would see. - d.RevisionStatesGetter = &MockRevisionStatesGetter{ - RevisionStates: &controllers.RevisionStates{ - RollingOut: []*controllers.RevisionMetadata{{}}, - }, - } + d.RevisionStatesGetter = newMockRevisionStatesGetter(gomock.NewController(t), &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{}}, + }, nil) d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *declcfg.VersionRelease, *declcfg.Deprecation, error) { v := declcfg.VersionRelease{ Version: bsemver.MustParse("1.0.0"), From 518364a64ee943cfd5a06d9268349681d420ea2e Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Wed, 24 Jun 2026 10:56:07 +0200 Subject: [PATCH 3/3] :seedling: Address review: move collision e2e test to update.feature, assert original CE stays installed - Move the higher-revision collision test from install.feature to update.feature where it fits better alongside the existing collision scenario. - Add assertions that the original ClusterExtension remains Installed=True after both collision points. - Add NamedClusterExtensionReportsCondition and ClusterExtensionHasClusterObjectSets step definitions. Co-Authored-By: Claude Opus 4.6 (1M context) --- test/e2e/features/update.feature | 94 ++++++++++++++++++++++++++++++++ test/e2e/steps/steps.go | 28 ++++++++++ 2 files changed, 122 insertions(+) diff --git a/test/e2e/features/update.feature b/test/e2e/features/update.feature index 1fa5016977..f22a141b1f 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -338,3 +338,97 @@ Feature: Update ClusterExtension Then ClusterExtension reports "${NAME}-1, ${NAME}-2" as active revisions And ClusterObjectSet "${NAME}-2" reports Progressing as True with Reason RollingOut And ClusterObjectSet "${NAME}-2" reports Available as False with Reason ProbeFailure + + @BoxcutterRuntime + @DeploymentConfig + Scenario: A conflicting ClusterExtension with a higher revision does not take over resources from the original owner + Given ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.0.0 + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + And ClusterExtension is rolled out + And ClusterExtension is available + And ClusterExtension "${NAME}" has 1 ClusterObjectSet + And the current ClusterExtension is tracked for cleanup + # Apply a second ClusterExtension pointing to the same package and version + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME}-dup + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.0.0 + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + revision object collisions + """ + # Verify the original ClusterExtension remains installed and unaffected + And ClusterExtension "ce-${SCENARIO_ID}" reports Installed as True + # Update the conflicting ClusterExtension with a deployment config env var. At the time + # of writing, there is no other way to get the operator controller to generate another + # ClusterObjectSet by simply targeting another bundle version in the ClusterExtension. + # Since nothing has been installed yet for this CE, the resolver returns the currently + # installed bundle. Injecting an environment variable via DeploymentConfig changes the + # desired state, which causes the controller to create a second ClusterObjectSet + # (revision 2) after the first one collides. + When ClusterExtension is updated + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + config: + configType: Inline + inline: + deploymentConfig: + env: + - name: MY_VAR + value: "my-value" + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.0.0 + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + Then ClusterExtension "${NAME}" has 2 ClusterObjectSets + # The higher-revision COS (revision 2) should also collide, not take over resources + And ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + revision object collisions + """ + # Verify the original ClusterExtension is still installed after the higher-revision collision + And ClusterExtension "ce-${SCENARIO_ID}" reports Installed as True diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 00a10ea64f..e62f9160eb 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -124,6 +124,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterExtensionReportsConditionWithMessageFragment) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterExtensionReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) as ([[:alnum:]]+)$`, ClusterExtensionReportsConditionWithoutReason) + sc.Step(`^(?i)ClusterExtension "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+)$`, NamedClusterExtensionReportsCondition) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterObjectSetReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message:$`, ClusterObjectSetReportsConditionWithMsg) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterObjectSetReportsConditionWithMessageFragment) @@ -183,6 +184,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)min value for (ClusterExtension|ClusterObjectSet) ((?:\.[a-zA-Z]+)+) is set to (\d+)$`, SetCRDFieldMinValue) sc.Step(`^(?i)the current ClusterExtension is tracked for cleanup$`, TrackCurrentClusterExtensionForCleanup) + sc.Step(`^(?i)ClusterExtension "([^"]+)" has (\d+) ClusterObjectSets?$`, ClusterExtensionHasClusterObjectSets) // TLS profile enforcement steps — deployment configuration sc.Step(`^(?i)the "([^"]+)" deployment is configured with custom TLS minimum version "([^"]+)"$`, ConfigureDeploymentWithCustomTLSVersion) @@ -393,6 +395,24 @@ func TrackCurrentClusterExtensionForCleanup(ctx context.Context) error { return nil } +// ClusterExtensionHasClusterObjectSets waits for the named ClusterExtension to own exactly the +// expected number of ClusterObjectSets. Polls with timeout. +func ClusterExtensionHasClusterObjectSets(ctx context.Context, extName string, expectedCount int) error { + sc := scenarioCtx(ctx) + extName = substituteScenarioVars(extName, sc) + waitFor(ctx, func() bool { + out, err := k8sClient("get", "clusterobjectsets", + "-l", fmt.Sprintf("olm.operatorframework.io/owner-name=%s", extName), + "-o", "jsonpath={.items[*].metadata.name}") + if err != nil { + return false + } + names := strings.Fields(strings.TrimSpace(out)) + return len(names) == expectedCount + }) + return nil +} + // ClusterExtensionVersionUpdate patches the ClusterExtension's catalog version to the specified value. func ClusterExtensionVersionUpdate(ctx context.Context, version string) error { sc := scenarioCtx(ctx) @@ -669,6 +689,14 @@ func ClusterExtensionReportsConditionWithoutReason(ctx context.Context, conditio return waitForExtensionCondition(ctx, conditionType, conditionStatus, nil, nil) } +// NamedClusterExtensionReportsCondition waits for a specific ClusterExtension (by name) to have a condition +// matching type and status. Polls with timeout. +func NamedClusterExtensionReportsCondition(ctx context.Context, extName, conditionType, conditionStatus string) error { + sc := scenarioCtx(ctx) + extName = substituteScenarioVars(extName, sc) + return waitForCondition(ctx, "clusterextension", extName, conditionType, conditionStatus, nil, nil) +} + // ClusterExtensionReportsConditionTransitionTime asserts that a condition's lastTransitionTime falls within // the specified minute range since the ClusterExtension's creation. func ClusterExtensionReportsConditionTransitionTime(ctx context.Context, conditionType string, minMinutes, maxMinutes int) error {