✨ 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.95% 69.00% +0.05%
==========================================
Files 139 139
Lines 9891 9915 +24
==========================================
+ Hits 6820 6842 +22
- Misses 2562 2563 +1
- Partials 509 510 +1
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? |
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/bundle_predicates.go
Outdated
Show resolved
Hide resolved
|
#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 |
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.
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
|
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. |
a2e5062 to
1e9cded
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/compare/compare_test.go
Outdated
Show resolved
Hide resolved
|
@grokspawn Done. Updated the PR to enable the feature gate in experimental release. |
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
3cca497 to
477accb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 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
internal/operator-controller/catalogmetadata/compare/compare_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
|
@rashmigottipati would you please resolve the copilot comments in this review? I'm trying to not duplicate guidance with them, and it's challenging to know what is still current. What I have been doing is:
WDYT? |
…upport Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
pedjak
left a comment
There was a problem hiding this comment.
Thanks for the PR! Here are some review comments on the implementation.
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/bundle_predicates.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 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/bundle_predicates.go
Outdated
Show resolved
Hide resolved
…kg.Release field Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
| // and falls back to build metadata comparison if equal. When disabled, it uses | ||
| // version+release from build metadata (backward compatible). | ||
| func ByVersionAndRelease(b1, b2 declcfg.Bundle) int { | ||
| if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) { |
There was a problem hiding this comment.
This change feels off to me.
It seems like this function shouldn't change at all. The change, as I understand it, boils down to how we build a version/release.
Without the feature gate: Only do it the legacy way by inspecting/parsing build metadata
With the feature gate: If release field is set, parse it directly. Only if unset do we fall back to legacy registry+v1 build metadata parsing.
But then once we have a VersionRelease, the comparison is the same.
Am I missing something else?
There was a problem hiding this comment.
Maybe the thing I'm missing is:
- operator-registry has a CompositeVersion type
- operator-controller has a VersionRelease type.
These represent the same exact concept, and we're trying to move in the direction of using the operator-registry variant?
IMO, we shouldn't mix them. We should pick one and delete the other.
There was a problem hiding this comment.
I completely agree that we need to consolidate to a single representative type.
This PR is not intended to force the type consistency yet, since if it adopted the CompositeVersion type right now it would lack a formal Release type which we determined was essential to encourage broad adoption.
It just provides the featuregate and the sensitivity to an explicit release version.
I'd intended to merge operator-framework/operator-registry#1938 and then we would have follow-ups to op-reg & op-con to adopt the base type consistently throughout.
I was just hoping to be able to merge the two PRs independently.
There was a problem hiding this comment.
So WDYT about this PR focusing on "if release explicitly set, load it into VersionRelease value". And then a later PR comes in a moves everything over to the operator-registry types/comparisons?
| // 2. Versions and releases are equal | ||
| // 3. Parsing errors occur (silently swallowed) | ||
| // Catalog validation should catch malformed versions before they reach this code. | ||
| result := b2.Compare(&b1) |
There was a problem hiding this comment.
Comparing at the bundle level seems incorrect to me based on the name of this function. What if the bundle comparison method considers more than the version and release?
Seems like we'd want b2.CompositeVersion.Compare(&b1.CompositeVersion) or something like that?
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Summary
Adds support for the explicit pkg.Release field in bundle metadata, enabling proper semantic versioning with release
information. When the BundleReleaseSupport feature gate is enabled, the comparison and sorting logic reads release information from the dedicated pkg.Release field (when present) instead of only using semver build metadata.
Key design decision: Re-releases require explicit channel entries (replaces/skips/skipRange) to be valid successors. The channel graph remains the sole authority on upgrade paths for registry+v1 bundles.
Changes
Feature Gate Behavior
Disabled (standard):
Enabled (experimental):
Reviewer Checklist
Link to Github Issue: #2495
Epic: #2479