🐛 Add support for build metadata precedence in bundle version comparison by joelanford · Pull Request #2273 · operator-framework/operator-controller

joelanford · 2025-10-16T17:22:08Z

Description

This change fixes an issue to ensure that operator-controller properly handles and compares registry+v1 bundle versions that include build metadata as specified in the semver version.

The intention is that we only treat build metadata as a release value for registry+v1 bundles, which already have this precedent set. If/when operator-controller gains support for new bundle types, the intention is to avoid continuing the practice (and semver violation) of treating version build metadata as comparable/orderable.

Key changes:

Introduce VersionRelease type combining semver version with release metadata
Update bundle comparison logic to use build metadata for ordering within registry+v1 bundles
Version field matching ignores build metadata (consistent with semver specification)
Build metadata is only considered for precedence when comparing otherwise equal versions
VersionRelease parsing tolerates non-release build metadata while preserving the original version
Replace GetVersion with GetVersionAndRelease across the codebase

This is particularly important for registry+v1 bundles that encode release information in the build metadata field of their version strings.

The ClusterExtension version field continues to work semantically as before. It ignores build metadata when matching versions (consistent with semver specification), even when build metadata is explicitly included in the version field. Build metadata is only used internally to determine precedence when ordering registry+v1 bundles with otherwise equal versions.

🤖 Generated with Claude Code

The bug this fixes was originally reported by Red Hat in https://issues.redhat.com/browse/OCPBUGS-60424

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-10-16T17:22:18Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c8e7316`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69387eefc4e440000732aca6
😎 Deploy Preview	https://deploy-preview-2273--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull Request Overview

This PR fixes bundle version comparison logic to properly handle registry+v1 bundles that use build metadata in their version strings. The implementation introduces a VersionRelease type that treats build metadata as comparable/orderable release information for registry+v1 bundles (a semver spec violation that exists for backward compatibility). The fix ensures exact version+release matching for pinned versions and successor determination.

Key changes:

Introduced VersionRelease type combining semver version with release metadata parsed from build metadata
Updated comparison and filtering logic to use exact version+release matching instead of semver-only matching
Replaced GetVersion with GetVersionAndRelease throughout the codebase

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/shared/util/slices/slices.go	Added generic Map utility function
internal/operator-controller/resolve/resolver.go	Updated interface to use VersionRelease instead of semver.Version
internal/operator-controller/resolve/catalog.go	Updated to use new comparison functions and moved Map function to shared utilities
internal/operator-controller/controllers/clusterextension_controller.go	Added conversion to legacy registry+v1 version format when creating bundle metadata
internal/operator-controller/catalogmetadata/filter/successors_test.go	Added test case for exact version matching with build metadata
internal/operator-controller/catalogmetadata/filter/successors.go	Implemented exact version+release matching for installed bundle comparison
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go	Updated test to use new InSemverRange function
internal/operator-controller/catalogmetadata/filter/bundle_predicates.go	Replaced Masterminds semver with blang semver and added exact version matching
internal/operator-controller/catalogmetadata/compare/compare_test.go	Added tests for NewVersionRange and updated existing tests for new comparison logic
internal/operator-controller/catalogmetadata/compare/compare.go	Implemented NewVersionRange with exact build metadata matching and renamed ByVersion to ByVersionAndRelease
internal/operator-controller/bundleutil/bundle_test.go	Added comprehensive tests for VersionRelease comparison logic
internal/operator-controller/bundleutil/bundle.go	Introduced VersionRelease, Release types and legacy registry+v1 conversion logic

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pedjak · 2025-10-16T22:02:51Z

@@ -0,0 +1,9 @@
+package slices
+
+func Map[I any, O any](in []I, f func(I) O) []O {


this function is available in https://github.com/samber/lo/blob/master/slice.go#L26 - how about to use it instead?

Not strongly opinionated. I do notice that that would be a new dependency to pull in to the project, so I could go either way.
Anyone else have opinions?

Hearing no other opinions, my vote is to leave as is. With this commit moving this function into a new package in the shared/utils space, hopefully future contributions will find this location to add new generic slice methods, and we can revisit the question again when there is more to be gained from pulling in a new dep.

I'm of two minds on the subject. The reference is to what looks like a personal library, which normally garners a 'no' from me. But it has a mess of contributions, releases, and some kind of community, which relaxes that somewhat.
I'd prefer that this package didn't reuse the same name as a standard library (https://pkg.go.dev/slices).
But otherwise I'm fine with having a minimal utility library here.

But it has a mess of contributions, releases, and some kind of community, which relaxes that somewhat.
The library has 20.5k stars which is really a lot and is a very good signal.

Agreed. I'm wondering why it looks like "some guy", but the support/longevity/utility appears to be there.
From a user perspective, I might be more interested in what dependencies it might bring along, but I see no cause for alarm so far.

codecov · 2025-10-17T04:06:46Z

Codecov Report

❌ Patch coverage is 96.90722% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.32%. Comparing base (f17f3c5) to head (c8e7316).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ator-controller/catalogmetadata/compare/compare.go	94.44%	0 Missing and 1 partial ⚠️
...or-controller/catalogmetadata/filter/successors.go	66.66%	1 Missing ⚠️
internal/operator-controller/resolve/resolver.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2273 +/- ##
==========================================
+ Coverage 71.10% 71.32% +0.22% 
==========================================
Files 95 97 +2 Lines 7323 7383 +60 ==========================================
+ Hits 5207 5266 +59 
Misses 1683 1683 - Partials 433 434 +1

Flag	Coverage Δ
e2e	`45.14% <68.04%> (+0.11%)`	⬆️
experimental-e2e	`14.33% <0.00%> (?)`
unit	`59.10% <91.75%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pedjak · 2025-10-20T08:29:49Z

+// ByVersionAndRelease is a comparison function that compares bundles by
+// version and release. Bundles with lower versions/releases are
+// considered less than bundles with higher versions/releases.
+func ByVersionAndRelease(b1, b2 declcfg.Bundle) int {


nit: how about that we implement Compare method on declcfg.Bundle instead?

The reason we don't have that right now is because declcfg.Bundle does not actually embed its own deprecation information. So we can't directly compare declcfg.Bundle along deprecation lines.
I suppose we could essentially combine ByVersionAndRelease and ByDeprecationFunc into a single comparison function, but:
It still wouldn't be a method on declcfg.Bundle
It would still require extra declcfg.Deprecation metadata to do the comparison.

Unfortunately, I've been fighting that concern in operator-registry space as well. I'd like for declcfg.Bundle to have the reusable comparators there, but the historical pattern is that both declcfg and SQLite graph contributions are ultimately converted to Model objects, where final type conversions occur, and validation considers the objects' full scope (including deprecations).
Right now the box feels like "all users must convert to Model objects" to me. 😞

camilamacedo86 · 2025-10-21T07:25:11Z

This one seems related to the bug that @grokspawn has been looking on.
I think it would be nice get his review here.

joelanford · 2025-10-22T18:43:40Z

crd-diff is failing due to the change in the description of the spec.source.catalog.version field. Assuming folks agree to the updated API semantics, we either need to override the failure or update the crd-diff check to ignore the description change.

I'm personally inclined to specifically override so that future description changes continue to be flagged as potentially breaking so that those changes get extra scrutiny.

pedjak

/lgtm a few more functions could be documented.

grokspawn · 2025-10-23T13:39:57Z

+ metadata (e.g., "1.0.0+20230101"). This ensures an exact match of both
+ the semver version and the release. If you specify a version without build
+ metadata (e.g., "1.0.0"), it will match all bundles with that version
+ regardless of their release information.


We'll need to update this once the bundle release version feature lands, since we will need to mention both approaches, including how one is preferred in future. Right now the bundle release version approach is to use [version]-[release] to express the attribute combo in a string (though the source of truth is always the discrete version and release fields in the olm.package property).
Perhaps this is better resolved in a doc where we can go into more depth?

pedjak

/lgtm

Copilot

Pull Request Overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

camilamacedo86

@joelanford it looks fine for me.
It seems to match with the @grokspawn PEP to solve the issue
Unless I missed something.

Just one thing — I think there’s a scenario we might need to handle in AsLegacyRegistryV1Version.

Also, wdyt about using an AI tool to double-check that we have tests covering all possible cases. This logic is pretty complex, and AI could help make sure we don’t miss any edge cases or weird combos. I think how much more we ensure here with unit test to cover all possible edge cases is better.

camilamacedo86 · 2025-10-28T16:21:36Z

@@ -36,6 +36,7 @@ func TestSuccessorsPredicate(t *testing.T) {
 {


@joelanford, I think we can cover a few more cases
Parse a valid semver value with build metadata like 3.0.0-pre+2.beta.1.
Check if we properly deal with invalid builds values (5.0.0+041).

A bunch of those cases are already covered in the operator-registry lib which would be where all these fields are introduced into the FBC.
(for e.g. alpha/property/property_test.go)
I don't think there's a lot of utility in duplicating those tests here.

But the tests that exist in the other project currently will not cover the same logic implemented here.
Ideally, we would have a shared library that provides this functionality so both projects could consume it directly.
However, since the implementation currently lives here and the checks are performed at runtime in this project, we should still have test coverage here. ( and there ; not centralized )
In my opinion, this is important because any change made in this code could break those runtime scenarios, and the tests in the other project would not be able to catch such regressions.

I have all of these cases implemented in internal/operator-controller/bundle/versionrelease_test.go.

This change fixes an issue to ensure that operator-controller properly handles and compares registry+v1 bundle versions that include build metadata as specified in the semver version. The intention is that we only treat build metadata as a release value for registry+v1 bundles, which already have this precedent set. If/when operator-controller gains support for new bundle types, the intention is to avoid continuing the practice (and semver violation) of treating version build metadata as comparable/orderable. Key changes: - Introduce VersionRelease type combining semver version with release metadata - Update bundle comparison logic to consider build metadata when present - Add exact version matching for pinned versions with build metadata - Replace GetVersion with GetVersionAndRelease across the codebase - Ensure successors are determined based on exact version+release matching This is particularly important for registry+v1 bundles that encode release information in the build metadata field of their version strings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…rove VersionRelease parsing This commit reverts the user-facing semantic changes to the ClusterExtension version field that were introduced to support exact version pinning with build metadata. The version field now ignores build metadata when matching versions, consistent with semver specification. Additionally, this commit modifies the VersionRelease parsing logic to be more tolerant of semver versions whose build metadata is not a valid release. When build metadata cannot be parsed as a release, the full version (including build metadata) is preserved in the Version field, with an empty Release field. Changes include: - Removed documentation about pinning to exact versions with build metadata - Removed exactVersionMatcher logic that enforced build metadata equality - Updated NewLegacyRegistryV1VersionRelease to tolerate non-release build metadata - Updated test expectations to reflect new behavior

openshift-ci · 2025-12-09T14:59:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [pedjak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/operator-controller/bundleutil/bundle_test.go:92

The test doesn't verify the expected wantVersionRelease value for test cases where wantErr is false. Consider adding an assertion to check that the returned value matches tc.wantVersionRelease when no error is expected:

vr, err:=bundleutil.GetVersionAndRelease(bundle)
iftc.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
require.Equal(t, tc.wantVersionRelease, vr)
}

 _, err := bundleutil.GetVersionAndRelease(bundle)
if tc.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add documentation for AsLegacyRegistryV1Version method explaining the build metadata conversion logic - Fix AsLegacyRegistryV1Version to preserve original build metadata when Release field is not set - Add comprehensive test coverage for NewLegacyRegistryV1VersionRelease including edge cases for build metadata and release parsing - Add test coverage for AsLegacyRegistryV1Version conversion logic - Improve compare_test.go test structure with descriptive test names and assertion functions for better clarity - Add test case for non-release build metadata comparison

grokspawn · 2025-12-11T20:37:02Z

/lgtm

… comparison (operator-framework#2273)" This reverts commit d66afb6.

joelanford requested a review from a team as a code owner October 16, 2025 17:22

joelanford requested review from Copilot and removed request for a teamOctober 16, 2025 17:22

openshift-ciBot requested review from anik120 and dtfranz October 16, 2025 17:22

CopilotAI reviewed Oct 16, 2025
View reviewed changes

Comment threadinternal/operator-controller/catalogmetadata/compare/compare.go

joelanford force-pushed the build-metadata-precedence branch 3 times, most recently from c2cd023 to d352bc0Compare October 16, 2025 18:24

pedjak reviewed Oct 16, 2025
View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from d352bc0 to e8571b6Compare October 17, 2025 03:54

joelanford force-pushed the build-metadata-precedence branch from e8571b6 to b398c34Compare October 17, 2025 17:24

pedjak reviewed Oct 20, 2025
View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from b398c34 to 2cf7680Compare October 22, 2025 18:05

pedjak reviewed Oct 23, 2025
View reviewed changes

Comment threadinternal/operator-controller/bundle/versionrelease.go

pedjak reviewed Oct 23, 2025
View reviewed changes

Comment threadinternal/operator-controller/bundle/versionrelease.go

pedjak approved these changes Oct 23, 2025
View reviewed changes

openshift-ciBot assigned pedjak Oct 23, 2025

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

grokspawn reviewed Oct 23, 2025
View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from 2cf7680 to 72c3a34Compare October 23, 2025 14:45

openshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

pedjak approved these changes Oct 23, 2025
View reviewed changes

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

joelanford commented Oct 23, 2025
View reviewed changes

Comment threadinternal/operator-controller/bundle/versionrelease.go

CopilotAI review requested due to automatic review settings October 23, 2025 18:04

openshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025

CopilotAI reviewed Oct 23, 2025
View reviewed changes

Comment threadinternal/operator-controller/catalogmetadata/compare/compare.go

camilamacedo86 reviewed Oct 28, 2025
View reviewed changes

Comment threadinternal/operator-controller/bundle/versionrelease.go

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2025

joelanfordand others added 2 commits December 9, 2025 09:57

joelanford force-pushed the build-metadata-precedence branch from 6372625 to fb24d4eCompare December 9, 2025 14:59

openshift-ciBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2025

CopilotAI review requested due to automatic review settings December 9, 2025 16:10

Copilotstarted reviewing on behalf of joelanford December 9, 2025 16:10 View session

CopilotAI reviewed Dec 9, 2025
View reviewed changes

joelanford force-pushed the build-metadata-precedence branch from 7025aa5 to c8e7316Compare December 9, 2025 19:56

openshift-ciBot assigned grokspawn Dec 11, 2025

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025

openshift-merge-botBot merged commit d66afb6 into operator-framework:mainDec 11, 2025
30 checks passed

grokspawn mentioned this pull request Mar 9, 2026
✨ Add support for parsing and persisting explicit pkg.Release field #2543
Merged
4 tasks

grokspawn added a commit to grokspawn/operator-controller that referenced this pull request Mar 12, 2026
Revert "🐛 Add support for build metadata precedence in bundle version…
8108ff8
… comparison (operator-framework#2273)" This reverts commit d66afb6.

		@@ -0,0 +1,9 @@
		package slices

		func Map[I any, O any](in []I, f func(I) O) []O {

		@@ -36,6 +36,7 @@ func TestSuccessorsPredicate(t *testing.T) {
		{

Uh oh!

Conversation

joelanford commented Oct 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlifyBot commented Oct 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanfordOct 17, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Oct 17, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Oct 21, 2025

Uh oh!

joelanford commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Oct 16, 2025•
edited
Loading

netlifyBot commented Oct 16, 2025•
edited
Loading

joelanfordOct 17, 2025•
edited
Loading

codecovBot commented Oct 17, 2025•
edited
Loading

camilamacedo86Oct 30, 2025•
edited
Loading