Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ $(YQ): $(LOCALBIN)
GOBIN=$(LOCALBIN) go install $(GO_INSTALL_OPTS) github.com/mikefarah/yq/v4@$(YQ_VERSION)

.PHONY: kind
kind: $(KIND) ## Download yq locally if necessary.
kind: $(KIND) ## Download kind locally if necessary.
$(KIND): $(LOCALBIN)
GOBIN=$(LOCALBIN) go install $(GO_INSTALL_OPTS) sigs.k8s.io/kind@latest

Expand Down
12 changes: 12 additions & 0 deletions crds/operators.coreos.com_clusterserviceversions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ spec:
jsonPath: .spec.version
name: Version
type: string
- description: The release version of the CSV
jsonPath: .spec.release
name: Release
type: string
- description: The name of a CSV that this one replaces
jsonPath: .spec.replaces
name: Replaces
Expand Down Expand Up @@ -8988,6 +8992,14 @@ spec:
type: string
name:
type: string
release:
type: string
maxLength: 20
x-kubernetes-validations:
- rule: self.matches('^[0-9A-Za-z-]+(\\.[0-9A-Za-z-]+)*$')
message: release version must be a valid semver prerelease format (dot-separated identifiers containing only alphanumerics and hyphens)
- rule: '!self.split(''.'').exists(x, x.matches(''^0[0-9]+$''))'
message: numeric identifiers in release version must not have leading zeros
replaces:
description: The name of a CSV this one replaces. Should match the `metadata.Name` field of the old CSV.
type: string
Expand Down
16 changes: 7 additions & 9 deletions crds/zz_defs.go

Large diffs are not rendered by default.

65 changes: 65 additions & 0 deletions pkg/lib/release/release.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
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
Copy link
Member

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:

  • CEL expression that does a regex match to make sure it is a valid prerelease string
  • Max length (keeping in mind that we expect the release to be included in the CSV metadata.name, which has its own length restrictions.

Was there any API review on this front? They'd be better at pointing out all the things.

Copy link
Contributor Author

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

// +kubebuilder:validation:MaxLength=20
// +kubebuilder:validation:XValidation:rule="self.matches('^[0-9A-Za-z-]+(\\\\.[0-9A-Za-z-]+)*$')",message="release version must be a valid semver prerelease format (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" }
161 changes: 161 additions & 0 deletions pkg/lib/release/release_test.go
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) {
Copy link
Contributor

@tmshort tmshort Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing any tests for alpha-numeric?

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
}
8 changes: 6 additions & 2 deletions pkg/operators/v1alpha1/clusterserviceversion_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -274,8 +275,10 @@ 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"`
InstallStrategy NamedInstallStrategy `json:"install"`
Version version.OperatorVersion `json:"version,omitempty"`
// +optional
Release release.OperatorRelease `json:"release,omitzero"`
Comment on lines +280 to +281
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better late than never to add all the nice GoDoc, similar to what we've done in OLMv1 APIs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please include user-friendly GoDoc here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Release release.OperatorRelease `json:"release,omitzero"`
Release *release.OperatorRelease `json:"release,omitempty"`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it a pointer and use omitempty instead of omitzero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To solve what problem?
If we make it a pointer, we get the same behavior but now also have to perform nil checking, which isn't necessary with this approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the empty string a valid value? What does it mean for this to be set to the empty string vs being omitted entirely?

Maturity string `json:"maturity,omitempty"`
CustomResourceDefinitions CustomResourceDefinitions `json:"customresourcedefinitions,omitempty"`
APIServiceDefinitions APIServiceDefinitions `json:"apiservicedefinitions,omitempty"`
Expand Down Expand Up @@ -595,6 +598,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 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`

Expand Down
1 change: 1 addition & 0 deletions pkg/operators/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/validation/internal/typecheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func checkEmptyFields(result *errors.ManifestResult, v reflect.Value, parentStru

// Omitted field tags will contain ",omitempty", and ignored tags will
// match "-" exactly, respectively.
isOptionalField := strings.Contains(tag, ",omitempty") || tag == "-"
isOptionalField := strings.Contains(tag, ",omitempty") || strings.Contains(tag, ",omitzero") || tag == "-"
emptyVal := fieldValue.IsZero()

newParentStructName := fieldType.Name
Expand Down
Loading