add Release version as an optional field in the CSV #454
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| package release | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| semver "github.com/blang/semver/v4" | ||
| ) | ||
|
|
||
| // +k8s:openapi-gen=true | ||
| // OperatorRelease is a wrapper around a slice of semver.PRVersion which supports correct | ||
| // marshaling to YAML and JSON. | ||
| // +kubebuilder:validation:Type=string | ||
| // +kubebuilder:validation:MaxLength=20 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[0-9A-Za-z-]+(\\\\.[0-9A-Za-z-]+)*$')",message="release version must be composed of dot-separated identifiers containing only alphanumerics and hyphens" | ||
| // +kubebuilder:validation:XValidation:rule="!self.split('.').exists(x, x.matches('^0[0-9]+$'))",message="numeric identifiers in release version must not have leading zeros" | ||
| type OperatorRelease struct { | ||
| Release []semver.PRVersion `json:"-"` | ||
| } | ||
|
|
||
| // DeepCopyInto creates a deep-copy of the Version value. | ||
| func (v *OperatorRelease) DeepCopyInto(out *OperatorRelease) { | ||
| out.Release = slices.Clone(v.Release) | ||
| } | ||
|
|
||
| // MarshalJSON implements the encoding/json.Marshaler interface. | ||
| func (v OperatorRelease) MarshalJSON() ([]byte, error) { | ||
| segments := []string{} | ||
| for _, segment := range v.Release { | ||
| segments = append(segments, segment.String()) | ||
| } | ||
| return json.Marshal(strings.Join(segments, ".")) | ||
| } | ||
|
|
||
| // UnmarshalJSON implements the encoding/json.Unmarshaler interface. | ||
| func (v *OperatorRelease) UnmarshalJSON(data []byte) (err error) { | ||
| var versionString string | ||
|
|
||
| if err = json.Unmarshal(data, &versionString); err != nil { | ||
| return | ||
| } | ||
|
|
||
| segments := strings.Split(versionString, ".") | ||
| for _, segment := range segments { | ||
| release, err := semver.NewPRVersion(segment) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| v.Release = append(v.Release, release) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // OpenAPISchemaType is used by the kube-openapi generator when constructing | ||
| // the OpenAPI spec of this type. | ||
| // | ||
| // See: https://github.com/kubernetes/kube-openapi/tree/master/pkg/generators | ||
| func (_ OperatorRelease) OpenAPISchemaType() []string { return []string{"string"} } | ||
|
|
||
| // OpenAPISchemaFormat is used by the kube-openapi generator when constructing | ||
| // the OpenAPI spec of this type. | ||
| // "semver" is not a standard openapi format but tooling may use the value regardless | ||
| func (_ OperatorRelease) OpenAPISchemaFormat() string { return "semver" } | ||
|
|
||
| func (r OperatorRelease) String() string { | ||
| segments := []string{} | ||
| for _, segment := range r.Release { | ||
| segments = append(segments, segment.String()) | ||
| } | ||
| return strings.Join(segments, ".") | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| package release | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| semver "github.com/blang/semver/v4" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestOperatorReleaseMarshal(t *testing.T) { | ||
grokspawn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| tests := []struct { | ||
| name string | ||
| in OperatorRelease | ||
| out []byte | ||
| err error | ||
| }{ | ||
| { | ||
| name: "single-segment", | ||
| in: OperatorRelease{Release: []semver.PRVersion{mustNewPRVersion("1")}}, | ||
| out: []byte(`"1"`), | ||
| }, | ||
| { | ||
| name: "two-segments", | ||
| in: OperatorRelease{Release: []semver.PRVersion{mustNewPRVersion("1"), mustNewPRVersion("0")}}, | ||
| out: []byte(`"1.0"`), | ||
| }, | ||
| { | ||
| name: "multi-segment", | ||
| in: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("1"), | ||
| mustNewPRVersion("2"), | ||
| mustNewPRVersion("3"), | ||
| }}, | ||
| out: []byte(`"1.2.3"`), | ||
| }, | ||
| { | ||
| name: "numeric-segments", | ||
| in: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("20240101"), | ||
| mustNewPRVersion("12345"), | ||
| }}, | ||
| out: []byte(`"20240101.12345"`), | ||
| }, | ||
| { | ||
| name: "alphanumeric-segments", | ||
| in: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("alpha"), | ||
| mustNewPRVersion("beta"), | ||
| mustNewPRVersion("1"), | ||
| }}, | ||
| out: []byte(`"alpha.beta.1"`), | ||
| }, | ||
| { | ||
| name: "alphanumeric-with-hyphens", | ||
| in: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("rc-1"), | ||
| mustNewPRVersion("build-123"), | ||
| }}, | ||
| out: []byte(`"rc-1.build-123"`), | ||
| }, | ||
| { | ||
| name: "mixed-alphanumeric", | ||
| in: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("1"), | ||
| mustNewPRVersion("2-beta"), | ||
| mustNewPRVersion("x86-64"), | ||
| }}, | ||
| out: []byte(`"1.2-beta.x86-64"`), | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| m, err := tt.in.MarshalJSON() | ||
| require.Equal(t, tt.out, m, string(m)) | ||
| require.Equal(t, tt.err, err) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestOperatorReleaseUnmarshal(t *testing.T) { | ||
| type TestStruct struct { | ||
| Release OperatorRelease `json:"r"` | ||
| } | ||
| tests := []struct { | ||
| name string | ||
| in []byte | ||
| out TestStruct | ||
| err error | ||
| }{ | ||
| { | ||
| name: "single-segment", | ||
| in: []byte(`{"r": "1"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{mustNewPRVersion("1")}}}, | ||
| }, | ||
| { | ||
| name: "two-segments", | ||
| in: []byte(`{"r": "1.0"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{mustNewPRVersion("1"), mustNewPRVersion("0")}}}, | ||
| }, | ||
| { | ||
| name: "multi-segment", | ||
| in: []byte(`{"r": "1.2.3"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("1"), | ||
| mustNewPRVersion("2"), | ||
| mustNewPRVersion("3"), | ||
| }}}, | ||
| }, | ||
| { | ||
| name: "numeric-segments", | ||
| in: []byte(`{"r": "20240101.12345"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("20240101"), | ||
| mustNewPRVersion("12345"), | ||
| }}}, | ||
| }, | ||
| { | ||
| name: "alphanumeric-segments", | ||
| in: []byte(`{"r": "alpha.beta.1"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("alpha"), | ||
| mustNewPRVersion("beta"), | ||
| mustNewPRVersion("1"), | ||
| }}}, | ||
| }, | ||
| { | ||
| name: "alphanumeric-with-hyphens", | ||
| in: []byte(`{"r": "rc-1.build-123"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("rc-1"), | ||
| mustNewPRVersion("build-123"), | ||
| }}}, | ||
| }, | ||
| { | ||
| name: "mixed-alphanumeric", | ||
| in: []byte(`{"r": "1.2-beta.x86-64"}`), | ||
| out: TestStruct{Release: OperatorRelease{Release: []semver.PRVersion{ | ||
| mustNewPRVersion("1"), | ||
| mustNewPRVersion("2-beta"), | ||
| mustNewPRVersion("x86-64"), | ||
| }}}, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| s := TestStruct{} | ||
| err := json.Unmarshal(tt.in, &s) | ||
| require.Equal(t, tt.out, s) | ||
| require.Equal(t, tt.err, err) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func mustNewPRVersion(s string) semver.PRVersion { | ||
| v, err := semver.NewPRVersion(s) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return v | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ import ( | |||||
| "k8s.io/apimachinery/pkg/labels" | ||||||
| "k8s.io/apimachinery/pkg/util/intstr" | ||||||
|
|
||||||
| "github.com/operator-framework/api/pkg/lib/release" | ||||||
| "github.com/operator-framework/api/pkg/lib/version" | ||||||
| ) | ||||||
|
|
||||||
|
|
@@ -274,8 +275,25 @@ type APIServiceDefinitions struct { | |||||
| // that can manage apps for a given version. | ||||||
| // +k8s:openapi-gen=true | ||||||
| type ClusterServiceVersionSpec struct { | ||||||
| InstallStrategy NamedInstallStrategy `json:"install"` | ||||||
| Version version.OperatorVersion `json:"version,omitempty"` | ||||||
| // release specifies the packaging version of the operator, defaulting to empty | ||||||
| // release is optional | ||||||
| // | ||||||
| // A ClusterServiceVersion's release field is used to provide a secondary ordering | ||||||
| // for operators that share the same semantic version. This is useful for | ||||||
| // operators that need to make changes to the CSV which don't affect their functionality, | ||||||
| // for example: | ||||||
| // - to fix a typo in their description | ||||||
| // - to add/amend annotations or labels | ||||||
| // - to amend examples or documentation | ||||||
| // | ||||||
| // It is up to operator authors to determine the semantics of release versions they use | ||||||
| // for their operator. All release versions must conform to the semver prerelease format | ||||||
| // (dot-separated identifiers containing only alphanumerics and hyphens) and are limited | ||||||
| // to a maximum length of 20 characters. | ||||||
| // +optional | ||||||
| Release release.OperatorRelease `json:"release,omitzero"` | ||||||
grokspawn marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment.
Suggested change
Member
There was a problem hiding this comment. Make it a pointer and use omitempty instead of omitzero?
Contributor
Author
There was a problem hiding this comment. To solve what problem?
Member
There was a problem hiding this comment. I guess it's a question of whether we want to differentiate between "field not set" which is the nil case vs "field explicitly set but empty"?
Contributor
There was a problem hiding this comment. Is the empty string a valid value? What does it mean for this to be set to the empty string vs being omitted entirely?
Contributor
Author
There was a problem hiding this comment. The important thing is the backwards-compatibility aspects of this change. If the value is an instance, protected by the
Contributor
Author
There was a problem hiding this comment. Other than that, this reduces the number of required checks. With a pointer, it is:
with the instance, it's just
For more discussion on the feature, check out this article.
Member
There was a problem hiding this comment.
I would say both of those would mean "there is no release". @grokspawn that's a convincing argument. @everettraven I always forget the API-specific considerations here. Does the somewhat new The type definition here is a bit atypical, which may be a complicating factor. Usually an object has fields that end up being serialized. But here, we are using custom (de)serialization to convert between a string (in the CRD schema to a We need the wrapper object in order to be able to implement
Contributor
Author
There was a problem hiding this comment. omitzero is new in go1.24, so our conventions docs do not address it. |
||||||
| Maturity string `json:"maturity,omitempty"` | ||||||
| CustomResourceDefinitions CustomResourceDefinitions `json:"customresourcedefinitions,omitempty"` | ||||||
| APIServiceDefinitions APIServiceDefinitions `json:"apiservicedefinitions,omitempty"` | ||||||
|
|
@@ -595,6 +613,7 @@ type ResourceInstance struct { | |||||
| // +kubebuilder:subresource:status | ||||||
| // +kubebuilder:printcolumn:name="Display",type=string,JSONPath=`.spec.displayName`,description="The name of the CSV" | ||||||
| // +kubebuilder:printcolumn:name="Version",type=string,JSONPath=`.spec.version`,description="The version of the CSV" | ||||||
| // +kubebuilder:printcolumn:name="Release",type=string,JSONPath=`.spec.release`,description="The release of this version of the CSV" | ||||||
| // +kubebuilder:printcolumn:name="Replaces",type=string,JSONPath=`.spec.replaces`,description="The name of a CSV that this one replaces" | ||||||
| // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` | ||||||
|
|
||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should add more validation here:
Was there any API review on this front? They'd be better at pointing out all the things.
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.
This PR was provided as part of the internal enhancement review, yes.
Links are in epic
Uh oh!
There was an error while loading. Please reload this page.
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.
Added some new CEL validations, including a max length of 20 chars. This was a nice, round number which was big enough for all known uses of analogous features (Freshmaker) as well as a minimal-precision date/time-stamp, and should leave ~40 chars to remain valid when composited to a metadata.name with the format
<package-name>-v<version>-<release-version>.Also added to the bundle validators to enforce the case where the author provides a noncompliant name for the release version case.
Uh oh!
There was an error while loading. Please reload this page.
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.
Any chance you looked for bundles that have the
olm.substitutesForannotation and checked to see if the overall<package-name>-v<version>-<release-version>format that we would require if they released again using a release field that followed the format of their version build metadata would overflow themetadata.namelength requirement of a CSV?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 evaluated all available FBCs I could get my hand on, and all existing entries would satisfy the naming requirements for bundles with release versions while continuing to follow their current release version schema use.
https://github.com/grokspawn/operator-registry/blob/validate-freshmaker/cmd/opm/validate-freshmaker/cmd.go is a single-use tool which performed this validation.