-
Notifications
You must be signed in to change notification settings - Fork 72
✨ Add support for parsing and persisting explicit pkg.Release field #2543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a97f95a
7b18170
8644489
f1985dc
ae3186a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,34 +11,99 @@ import ( | |
|
|
||
| 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) { | ||
| for _, p := range b.Properties { | ||
| if p.Type == property.TypePackage { | ||
| var pkg property.Package | ||
| if err := json.Unmarshal(p.Value, &pkg); err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling package property: %w", err) | ||
| } | ||
|
|
||
| // TODO: For now, we assume that all bundles are registry+v1 bundles. | ||
| // In the future, when we support other bundle formats, we should stop | ||
| // using the legacy 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 parseVersionRelease(p.Value) | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("no package property found in bundle %q", b.Name) | ||
| } | ||
|
|
||
| // MetadataFor returns a BundleMetadata for the given bundle name and version. | ||
| func MetadataFor(bundleName string, bundleVersion bsemver.Version) ocv1.BundleMetadata { | ||
| func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error) { | ||
| var pkg property.Package | ||
| if err := json.Unmarshal(pkgData, &pkg); err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling package property: %w", err) | ||
| } | ||
|
|
||
| // Check if release field is explicitly present in JSON (even if empty). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since If we are worried about the scenario where the feature is disabled, but a bundle with a release version is in the catalog, then we can explicitly clear the field. Is there another scenario that's important?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you agree that we need this distinction for the upgrade path based on the previous review comments, should we update |
||
| // property.Package has Release string, so we can't distinguish "field absent" from "field empty". | ||
| // We unmarshal again into a helper struct with Release *string to detect presence. | ||
| var releaseField struct { | ||
| Release *string `json:"release"` | ||
| } | ||
| if err := json.Unmarshal(pkgData, &releaseField); err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling package release field: %w", err) | ||
| } | ||
|
|
||
| // When BundleReleaseSupport is enabled and bundle has explicit release field, use it. | ||
| if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) && releaseField.Release != nil { | ||
| return parseExplicitRelease(pkg.Version, *releaseField.Release) | ||
| } | ||
|
rashmigottipati marked this conversation as resolved.
|
||
|
|
||
| // Fall back to legacy registry+v1 behavior (release in build metadata) | ||
|
rashmigottipati marked this conversation as resolved.
|
||
| // | ||
| // TODO: For now, we assume that all bundles are registry+v1 bundles. | ||
| // 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 | ||
| } | ||
|
|
||
| // 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 | ||
| // interprets build metadata AS the release value for registry+v1 bundles. | ||
| func parseExplicitRelease(version, releaseStr string) (*bundle.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 | ||
| if releaseStr == "" { | ||
| // Explicit empty release: use empty slice (not nil) | ||
| rel = bundle.Release([]bsemver.PRVersion{}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This goes against bundle.NewRelease handling, which returns nil if given an empty string.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my understanding, we need the empty slice (not nil) for roundtrip correctness. If we use nil for explicit empty release, then in MetadataFor, the check The three-state representation is:
Without this, we can't preserve the upgrade path semantics mentioned in this comment Pls correct me if I am missing something here. |
||
| } else { | ||
| rel, err = bundle.NewRelease(releaseStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing release %q: %w", releaseStr, err) | ||
| } | ||
| } | ||
|
|
||
| return &bundle.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 { | ||
| if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) { | ||
| // New behavior: separate Version and Release fields | ||
| bm := ocv1.BundleMetadata{ | ||
| Name: bundleName, | ||
| Version: vr.Version.String(), | ||
| } | ||
| if vr.Release != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like we could skip it if we we move to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I am thinking that when opreg PR merges and we do a follow-up PR in opcon to do the type convergence, we can then use |
||
| relStr := vr.Release.String() | ||
| bm.Release = &relStr | ||
| } | ||
| return bm | ||
| } | ||
| // Old behavior for backward compatibility: reconstitute build metadata in Version field | ||
| // This preserves release information (e.g., "1.0.0+2") for standard CRD users where | ||
| // the Release field is pruned by the API server. | ||
| return ocv1.BundleMetadata{ | ||
| Name: bundleName, | ||
| Version: bundleVersion.String(), | ||
| Version: vr.AsLegacyRegistryV1Version().String(), | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to do this in a follow-up PR, because I think it ultimately should be based on op-reg Release type instead of op-con Release type.
This is neither.
Do we have any concern if a later PR makes this explicitly declcfg.Release type?
Please align the type+validation with api and my follow-on PR in operator-registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there is meaning between nil and empty, we generally prefer to do
I don't think there's any difference between these cases for this type. If the release is empty or omitted then it equates to no release value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grokspawn I was going to converge the type in the follow-up PR as well but based on this review comment , I added
Releasefield to the BundleMetadata struct so we can persist it and roundtrip it properly.No concern if a later PR makes this a declcfg.Release type though.
I'll align the type+validation with api and your follow-on PR in operator-registry as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding using omitzero, @joelanford has identified an upgrade edge case when OLMv1 upgrades from a version without the Release field to one with it:
#2543 (comment)
To fix this problem, I think we need to make Release a *string type to distinguish between:
Without this distinction, when OLMv1 reconciles an existing ClusterExtension after upgrade, we can't tell if
Version: "1.2.0+1"with no Release field means "parse +1 as release" (old behavior) or "keep +1 as build metadata" (new behavior).With omitzero on a value type, both cases serialize identically, breaking the upgrade path. The pointer lets us preserve the semantic difference between these two
olm.packagevalues:{"version": "1.2.0+1"}-->{Version: "1.2.0", Release: "1"}{"version": "1.2.0+1", "release": ""}-->{Version: "1.2.0+1", Release: ""}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation is now aligned with MaxLength=20.