✨ Support explicit pkg.Release field with build metadata fallback for bundle comparison#2543
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR introduces an alpha feature gate to allow OLMv1 upgrade resolution to treat “re-released” bundles (same semver, higher release/build value like 2.0.0+1 -> 2.0.0+2) as valid successors when explicitly enabled.
Changes:
- Added
ReleaseVersionPriorityfeature gate (Alpha, default disabled). - Added
SameVersionHigherRelease()predicate and integrated it intoSuccessorsOf()behind the feature gate. - Added unit tests for
SameVersionHigherRelease()and forSuccessorsOf()with the gate disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/features/features.go | Defines the new ReleaseVersionPriority feature gate and its spec. |
| internal/operator-controller/catalogmetadata/filter/successors.go | Conditionally expands successor matching to include same-version/higher-release bundles when gated on. |
| internal/operator-controller/catalogmetadata/filter/bundle_predicates.go | Adds the SameVersionHigherRelease() predicate. |
| internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go | Adds predicate unit tests including edge cases. |
| internal/operator-controller/catalogmetadata/filter/successors_test.go | Adds a regression test to ensure default (gate-off) behavior does not accept higher-release bundles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors_test.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2543 +/- ##
==========================================
- Coverage 68.89% 63.18% -5.71%
==========================================
Files 139 139
Lines 9910 9931 +21
==========================================
- Hits 6827 6275 -552
- Misses 2572 3157 +585
+ Partials 511 499 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/successors_test.go
Outdated
Show resolved
Hide resolved
|
If I understand this correctly, it looks like this introduces the new behavior that an explicit upgrade edge doesn't actually have to exist in the catalog to upgrade to a bundle that has the same version and a higher release. Is that the intent? (If so let's make that clear in the PR description). Is it also the intent that we'll inherit the upgrade edges of the first release of the version? |
| const testPackageName = "test-package" | ||
|
|
||
| // Expected bundle version 2.0.0+1 | ||
| expect, err := bundle.NewLegacyRegistryV1VersionRelease("2.0.0+1") |
There was a problem hiding this comment.
I'd suggest putting the expect value in the test case, and adding cases that start from different values. For example: from a version without a release.
Also related: do we need to add code that knows how to parse the new Release field of the olm.package property (around here:
There was a problem hiding this comment.
iirc, @grokspawn's design called for a specific precedence when handling both release and build metadata that we will need to account for.
| // If release version priority is enabled, also consider bundles | ||
| // with the same semantic version but higher release as valid successors. | ||
| // This allows upgrades to re-released bundles (e.g., 2.0.0+1 -> 2.0.0+2). |
There was a problem hiding this comment.
What if the re-release version is already in the explicit upgrade graph? In that case, should we ignore it and let its explicit channel entry take precedence? That would preserve existing behavior.
| func SameVersionHigherRelease(expect bundle.VersionRelease) filter.Predicate[declcfg.Bundle] { | ||
| return func(b declcfg.Bundle) bool { | ||
| actual, err := bundleutil.GetVersionAndRelease(b) | ||
| if err != nil { |
There was a problem hiding this comment.
could failing silently here lead to some hard to spot bugs?
|
#2273 makes the claim that future bundle types will drop this approach, not that registry+v1 bundles should. So that's my bad. |
Sadly, we have to rely on the presence of existing graph edges or we break assumptions users have with the registry+v1 bundle format, coming from v0. So even though we have the ability to prefer version+release over version, since replaces/skips use named bundles instead of bundle versions we have to support the older behavior. |
That would be a regression of a bug fix that #2273 made though, right?
Yeah, agreed. I reached the same conclusion after thinking about this more. So if I understand correctly now:
So the change we need now is "look for |
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
5681488 to
8dcab49
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Legacy registry+v1 bundles embed the release value inside their versions as build metadata | ||
| // (in violation of the semver spec). If/when we add explicit release metadata to bundles and/or | ||
| // we support a new bundle format, we need to revisit the assumption that all bundles are | ||
| // registry+v1 and embed release in build metadata. |
There was a problem hiding this comment.
This error should wrap the underlying error with %w instead of formatting it with %v, so callers can unwrap/inspect it consistently (and it matches the %w usage elsewhere in this function).
| return nil, fmt.Errorf("failed to get composite version for installed bundle %q: %w", installedBundle.Name, err) |
| // Expected bundle version 2.0.0+1 | ||
| expect, err := bundle.NewLegacyRegistryV1VersionRelease("2.0.0+1") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
SameVersionHigherRelease now takes a declcfg.CompositeVersion, but this test builds expect via bundle.NewLegacyRegistryV1VersionRelease(...) (type *bundle.VersionRelease) and passes *expect into SameVersionHigherRelease, which will not compile. Construct expect as a declcfg.CompositeVersion (e.g., by creating a declcfg.Bundle with the expected package property and calling CompositeVersion()), and remove the dependency on the internal bundle type here.
There was a problem hiding this comment.
resolve.Resolver / resolve.Func now return *declcfg.CompositeVersion instead of *bundle.VersionRelease. This change requires updating all resolver implementations and test fakes/mocks; for example, internal/operator-controller/controllers/clusterextension_controller_test.go still defines resolve.Func(...)(*declcfg.Bundle, *bundle.VersionRelease, ...), which will no longer compile. Please update those call sites to return *declcfg.CompositeVersion (and adjust any code that relied on AsLegacyRegistryV1Version()).
| successorsPredicate, | ||
| ExactVersionRelease(*installedVersionRelease), | ||
| ), nil | ||
| } | ||
|
|
||
| // If release version priority is enabled, also consider bundles | ||
| // with the same semantic version but higher release as valid successors. | ||
| // This allows upgrades to re-released bundles (e.g., 2.0.0+1 -> 2.0.0+2). | ||
| if features.OperatorControllerFeatureGate.Enabled(features.ReleaseVersionPriority) { | ||
| predicates = append(predicates, SameVersionHigherRelease(*installedVersionRelease)) | ||
| } |
There was a problem hiding this comment.
SuccessorsOf is described/tested as feature-gated to allow same-semver higher-release bundles, but the implementation currently never checks features.ReleaseVersionPriority and never composes SameVersionHigherRelease(...) into the returned predicate. As a result, enabling/disabling the new gate has no effect and the new gate tests cannot pass. Add a gate check and OR in SameVersionHigherRelease(*installedCompositeVersion) when the feature is enabled.
|
Adjusted my comments. I missed the part of the #2273 description when it said that we wouldn't divert from semver ordering for any new bundle formats. |
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
…hen comparison returns equal Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
a2e5062 to
1e9cded
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HelmChartSupport featuregate.Feature = "HelmChartSupport" | ||
| BoxcutterRuntime featuregate.Feature = "BoxcutterRuntime" | ||
| DeploymentConfig featuregate.Feature = "DeploymentConfig" | ||
| BoxcutterRuntime featuregate.Feature = "BoxcutterRuntime" | ||
| DeploymentConfig featuregate.Feature = "DeploymentConfig" | ||
| CompositeVersionComparison featuregate.Feature = "CompositeVersionComparison" | ||
| ) |
There was a problem hiding this comment.
PR title/description reference a ReleaseVersionPriority feature gate, but this code introduces and checks CompositeVersionComparison instead (and there are no references to ReleaseVersionPriority in the repo). Please align the feature gate name across the PR metadata and implementation (either rename the gate in code or update the PR title/description) so operators know which flag to set.
| // Explicitly disable the feature gate for this test | ||
| prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.CompositeVersionComparison))) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=%t", features.CompositeVersionComparison, prevEnabled))) | ||
| }) |
There was a problem hiding this comment.
These tests mutate the global features.OperatorControllerFeatureGate via Set(...). Unit tests run with go test ... across packages in parallel (and with -race), so this can create cross-package interference/flakes when another package’s tests toggle the same gate concurrently. Consider adding a mutex-protected helper in the features package for temporarily setting gates in tests (shared across packages), or refactor the code under test to accept an injected featuregate.FeatureGate so tests can use an isolated instance.
| prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=%t", features.CompositeVersionComparison, prevEnabled))) | ||
| }) | ||
|
|
||
| t.Run("feature gate disabled - uses build metadata", func(t *testing.T) { | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.CompositeVersionComparison))) | ||
| result := compare.ByVersionAndRelease(registryV1_b1, registryV1_b2) | ||
| assert.Greater(t, result, 0, "should sort by build metadata: 1.0.0+2 > 1.0.0+1") |
There was a problem hiding this comment.
This test toggles the process-global features.OperatorControllerFeatureGate. Because make test-unit runs go test over many packages in parallel (and with -race), this can race with other packages’ tests that also set CompositeVersionComparison, leading to nondeterministic failures. Please guard gate mutations with a shared lock/helper in the features package or use an injected/local FeatureGate instance for the code under test.
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
changed the title
|
@grokspawn @joelanford Updated based on your feedback. PR now implements the hybrid approach: uses Added feature gate Ready for re-review when you have a chance. Thanks! |
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func SameVersionHigherRelease(expect bundle.VersionRelease) filter.Predicate[declcfg.Bundle] { | ||
| return func(b declcfg.Bundle) bool { | ||
| actual, err := bundleutil.GetVersionAndRelease(b) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| if expect.Version.Compare(actual.Version) != 0 { | ||
| return false | ||
| } | ||
|
|
||
| return expect.Release.Compare(actual.Release) < 0 | ||
| } |
There was a problem hiding this comment.
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.
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.CompositeVersionComparison))) | ||
| result := compare.ByVersionAndRelease(b1, b2) | ||
| assert.Positive(t, result, "Bundle.Compare() returns 0, fallback to build metadata: 1.0.0+2 > 1.0.0+1") | ||
| }) |
There was a problem hiding this comment.
The new feature-gated comparison test only exercises the registry+v1 fallback case where Bundle.Compare() returns 0 and build metadata breaks ties. There’s no unit test demonstrating that, with CompositeVersionComparison enabled, bundles with the same pkg.Version but different explicit pkg.Release values sort correctly. Adding a test case for explicit pkg.Release ordering would help prevent regressions and validate the new behavior described in the PR.
| }) | |
| }) | |
| t.Run("feature gate enabled - explicit release orders bundles with the same version", func(t *testing.T) { | |
| withLowerRelease := declcfg.Bundle{ | |
| Name: "package1.v1.0.0-r1", | |
| Properties: []property.Property{ | |
| {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0", "release": "1"}`)}, | |
| }, | |
| } | |
| withHigherRelease := declcfg.Bundle{ | |
| Name: "package1.v1.0.0-r2", | |
| Properties: []property.Property{ | |
| {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0", "release": "2"}`)}, | |
| }, | |
| } | |
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.CompositeVersionComparison))) | |
| result := compare.ByVersionAndRelease(withLowerRelease, withHigherRelease) | |
| assert.Positive(t, result, "with CompositeVersionComparison enabled, bundles with the same version should sort by explicit release: release 2 > release 1") | |
| }) |
| // TestSuccessorsOf_WithCompositeVersionComparison_FeatureGateDisabled verifies higher releases | ||
| // are NOT successors when CompositeVersionComparison gate is disabled (testing the default behavior). | ||
| func TestSuccessorsOf_WithCompositeVersionComparison_FeatureGateDisabled(t *testing.T) { | ||
| // Explicitly disable the feature gate for this test | ||
| prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.CompositeVersionComparison))) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=%t", features.CompositeVersionComparison, prevEnabled))) | ||
| }) | ||
|
|
||
| channel := declcfg.Channel{ | ||
| Entries: []declcfg.ChannelEntry{ | ||
| {Name: "test-package.v1.0.0+1"}, | ||
| { | ||
| Name: "test-package.v2.0.0", | ||
| Replaces: "test-package.v1.0.0+1", | ||
| }, | ||
| }, | ||
| } | ||
| installedBundle := ocv1.BundleMetadata{ | ||
| Name: "test-package.v1.0.0+1", | ||
| Version: "1.0.0+1", | ||
| } | ||
|
|
||
| higherRelease := declcfg.Bundle{ | ||
| Name: "test-package.v1.0.0+2", | ||
| Package: "test-package", | ||
| Properties: []property.Property{ | ||
| property.MustBuildPackage("test-package", "1.0.0+2"), | ||
| }, | ||
| } | ||
|
|
||
| predicate, err := SuccessorsOf(installedBundle, channel) | ||
| require.NoError(t, err) | ||
|
|
||
| // Higher release should NOT match without feature gate | ||
| assert.False(t, predicate(higherRelease), "1.0.0+2 should NOT be a successor of 1.0.0+1 when feature gate disabled") | ||
| } | ||
|
|
||
| // TestSuccessorsOf_WithCompositeVersionComparison_FeatureGateEnabled verifies higher releases | ||
| // as valid successors when CompositeVersionComparison gate is enabled. | ||
| func TestSuccessorsOf_WithCompositeVersionComparison_FeatureGateEnabled(t *testing.T) { | ||
| // Enable the feature gate for this test | ||
| prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.CompositeVersionComparison) | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.CompositeVersionComparison))) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=%t", features.CompositeVersionComparison, prevEnabled))) | ||
| }) | ||
|
|
||
| channel := declcfg.Channel{ | ||
| Entries: []declcfg.ChannelEntry{ | ||
| {Name: "test-package.v1.0.0+1"}, | ||
| { | ||
| Name: "test-package.v2.0.0", | ||
| Replaces: "test-package.v1.0.0+1", | ||
| }, | ||
| }, | ||
| } | ||
| installedBundle := ocv1.BundleMetadata{ | ||
| Name: "test-package.v1.0.0+1", | ||
| Version: "1.0.0+1", | ||
| } | ||
|
|
||
| higherRelease := declcfg.Bundle{ | ||
| Name: "test-package.v1.0.0+2", | ||
| Package: "test-package", | ||
| Properties: []property.Property{ | ||
| property.MustBuildPackage("test-package", "1.0.0+2"), | ||
| }, | ||
| } | ||
|
|
||
| predicate, err := SuccessorsOf(installedBundle, channel) | ||
| require.NoError(t, err) | ||
|
|
||
| // Higher release should match when feature gate is enabled | ||
| assert.True(t, predicate(higherRelease), "1.0.0+2 should be a successor of 1.0.0+1 when feature gate enabled") | ||
| } |
There was a problem hiding this comment.
These SuccessorsOf feature-gate tests only cover legacy registry+v1 bundles where release is embedded in the version build metadata. Since CompositeVersionComparison is intended to support bundles with an explicit pkg.Release field, it would be good to add a test that constructs a bundle with pkg.Version "1.0.0" and pkg.Release set, and verifies SameVersionHigherRelease/SuccessorsOf behavior when the gate is enabled.
Description
Summary
Implements hybrid bundle comparison that supports both registry+v1 bundles (build metadata) and new bundle formats (explicit
pkg.Releasefield).Uses
Bundle.Compare()from operator-registry to readpkg.Releasefrom olm.package property, falling back to build metadata parsing when comparison returns equal. This enables re-release upgrades (e.g.,1.0.0+1→1.0.0+2).Changes
CompositeVersionComparisonfeature gate (alpha, disabled by default)ByVersionAndRelease()to useBundle.Compare()with build metadata fallbackSuccessorsOf()to includeSameVersionHigherReleasepredicate when feature gate enabledFeature Gate Behavior
Disabled (default): Backward compatible - uses build metadata only
Enabled: Hybrid approach
Bundle.Compare()first (reads explicitpkg.Release)Reviewer Checklist
Link to Github Issue: #2495
Epic: #2479