🌱 consolidate Release types with operator-registry#2771
🌱 consolidate Release types with operator-registry#2771rashmigottipati wants to merge 2 commits into
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 removes operator-controller’s duplicate bundle.Release / bundle.VersionRelease definitions and standardizes on declcfg.Release / declcfg.VersionRelease from operator-registry, keeping the API boundary (BundleMetadata.Release as *string) intact via conversion helpers.
Changes:
- Replaced internal
bundle.VersionReleaseusage across resolver, catalogmetadata filtering/comparison, and tests withdeclcfg.VersionRelease. - Moved legacy registry+v1 “release-in-build-metadata” parsing logic into package-local helpers where needed, and updated metadata conversion (
bundleutil.MetadataFor) accordingly. - Removed the now-redundant
internal/operator-controller/bundle/versionrelease.goand its unit tests; updated generated CRD artifacts touched by regeneration.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| manifests/experimental.yaml | Updates generated experimental manifests (includes CRD generator version annotation change for ClusterExtension CRD). |
| manifests/experimental-e2e.yaml | Updates generated experimental e2e manifests (includes CRD generator version annotation change for ClusterExtension CRD). |
| internal/operator-controller/resolve/resolver.go | Changes resolver interface to return *declcfg.VersionRelease instead of the internal type. |
| internal/operator-controller/resolve/catalog.go | Propagates declcfg.VersionRelease through catalog resolution return types. |
| internal/operator-controller/resolve/catalog_test.go | Updates resolver unit tests to assert against declcfg.VersionRelease. |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Updates controller tests to use declcfg.VersionRelease in mocked resolver responses. |
| internal/operator-controller/catalogmetadata/filter/successors.go | Converts installed bundle metadata parsing to produce *declcfg.VersionRelease, including legacy registry+v1 parsing helper. |
| internal/operator-controller/catalogmetadata/filter/successors_test.go | Adjusts successor/filter tests to use declcfg.VersionRelease parsing helper. |
| internal/operator-controller/catalogmetadata/filter/bundle_predicates.go | Updates version/release predicate API to take declcfg.VersionRelease and compare via declcfg’s Compare semantics. |
| internal/operator-controller/catalogmetadata/compare/compare.go | Updates bundle version/release comparison to use declcfg.VersionRelease. |
| internal/operator-controller/bundleutil/bundle.go | Updates bundle property parsing and MetadataFor conversion to use declcfg.Release / declcfg.VersionRelease and new legacy parsing/conversion helpers. |
| internal/operator-controller/bundleutil/bundle_test.go | Updates bundleutil tests for declcfg.VersionRelease expectations. |
| internal/operator-controller/bundle/versionrelease.go | Deletes the duplicated internal release/version types and logic. |
| internal/operator-controller/bundle/versionrelease_test.go | Deletes unit tests for the removed internal release/version types. |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml | Updates generated Helm base CRD for ClusterExtension (includes generator version annotation change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2771 +/- ##
==========================================
- Coverage 66.78% 66.75% -0.04%
==========================================
Files 149 148 -1
Lines 11382 11367 -15
==========================================
- Hits 7602 7588 -14
+ Misses 3221 3217 -4
- Partials 559 562 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rashmigottipati
force-pushed
the
consolidate-release-type
branch
2 times, most recently
from
2865724 to
ccc8f3b
Compare
tmshort
left a comment
tmshort
left a comment
There was a problem hiding this comment.
Most of this is changing the namespace of an API, but there is some de-duplication that could be done.
openshift-ci
Bot
added
the
needs-rebase
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
force-pushed
the
consolidate-release-type
branch
from
ac0cad4 to
e0fe939
Compare
openshift-ci
Bot
removed
the
needs-rebase
rashmigottipati
force-pushed
the
consolidate-release-type
branch
from
e0fe939 to
da8ab9c
Compare
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
force-pushed
the
consolidate-release-type
branch
from
da8ab9c to
21e1ef3
Compare
|
|
||
| buildMetadata := strings.Join(vr.Version.Build, ".") | ||
|
|
||
| rel, err := declcfg.NewRelease(buildMetadata) |
There was a problem hiding this comment.
NewRelease here introduces a length limit on buildMetadata (https://github.com/operator-framework/operator-registry/blob/master/alpha/model/versionrelease.go#L84) - this is technically a breaking change since we didn't have a limit before. Are we concerned about this at all? Could it break existing bundles? Should we call this out in docs somewhere or mark this PR as a breaking change? (cc @grokspawn)
There was a problem hiding this comment.
All available catalogs were assessed before establishing the limit, and the limit is sufficient with some breathing room (operator-framework/operator-registry#1852).
IMHO it was an oops on our part to allow an unbounded construct in the first place (pointing finger at myself).
It's not impossible that there is an unknown case where someone has become dependent on using this in a way which would violate the length rule, but it would have to have been in the last six months, so I don't think I'm concerned here. Happy to discuss further, in case there are specific cases/conditions of concern.
There was a problem hiding this comment.
... and great question. I failed to show all my work here I guess.
There was a problem hiding this comment.
@grokspawn thanks for clarifying! Good to know all catalogs were already assessed for this.
@perdasilva can we proceed with this PR since this concern has been addressed?
perdasilva
force-pushed
the
consolidate-release-type
branch
from
518364a to
21e1ef3
Compare
Description
Removes duplicate
bundle.Releaseandbundle.VersionReleasetype definitions in favor of usingdeclcfg.Releaseanddeclcfg.VersionReleasefrom operator-registry.This consolidates type definitions between operator-controller and operator-registry, eliminating code duplication and aligning with the vision from OPRUN-4548.
Changes:
internal/operator-controller/bundle/versionrelease.goand the test filedeclcfg.VersionReleaseanddeclcfg.Releasefrom operator-registryBundleMetadata.Release) remains*stringwith conversion at:bundleutil.MetadataFor(declcfg -> API)catalogmetadata/filter.parseInstalledBundleVersionRelease(API -> declcfg)Reviewer Checklist