Skip to content

⚠ Add spec.group to ClusterObjectSet revision tracking #2969

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.

Sign up for GitHub

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

Open
fgiudici wants to merge 10 commits into
base: main
Choose a base branch
from
Open

⚠ Add spec.group to ClusterObjectSet revision tracking #2969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b75ac4a
Add ClusterObjectSet group API specification
fgiudici Oct 1, 2026
e90a0e7
Generate ClusterObjectSet group API artifacts
fgiudici Oct 1, 2026
77144eb
Track ClusterObjectSet revisions by group
fgiudici Oct 1, 2026
b13b91b
Cover group tracking in experimental E2E tests
fgiudici Oct 1, 2026
bddfaa9
Allow selecting ClusterObjectSets by group
fgiudici Oct 13, 2026
69e33d8
Decouple revision lookup from ClusterExtension
fgiudici Oct 13, 2026
7b7571a
Address review feedback for COS revision groups
fgiudici Oct 14, 2026
6ec1eda
Mark future archival documentation update for COD
fgiudici Oct 16, 2026
a13cd60
Address review feedback for COS group tracking
fgiudici Oct 16, 2026
4a2103d
Fix object count scenario with required revision group
fgiudici Oct 16, 2026
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
41 changes: 29 additions & 12 deletions api/v1/clusterobjectset_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ const (

// ClusterObjectSetSpec defines the desired state of ClusterObjectSet.
type ClusterObjectSetSpec struct {
// group is a required, immutable identifier that links related revisions together.
// All revisions sharing the same group form an ordered sequence, regardless of owner references.
// The value must be 1 to 52 characters long, start with a lowercase letter,
// contain only lowercase letters, digits or hyphens, and end with a letter or digit.
//
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=52
// +kubebuilder:validation:XValidation:rule=`self.matches('^[a-z]([a-z0-9-]*[a-z0-9])?$')`,message="group must start with a lowercase letter, contain only lowercase letters, digits or hyphens, and end with a letter or digit"
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="group is immutable"
Group string `json:"group,omitempty"`

@joelanford joelanford Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this field is required, no need for omitempty:

Suggested change
Group string `json:"group,omitempty"`
Group string `json:"group"`

@fgiudici fgiudici Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! The linter anyway (make api-lint-diff) enforces it for required fields:
api/v1/clusterobjectset_types.go:51:2:kubeapilinter:requiredfields: field ClusterObjectSetSpec.Group should have the omitempty tag.
Seems it is deliberate for RequiredFields in kube-api-linter to cope with serialization: serializing without omitempty would serialize and empty group with an empty string (which also violates the validation rule of at least 1 char).
https://github.com/kubernetes-sigs/kube-api-linter/blob/main/docs/linters.md#requiredfields

@joelanford joelanford Oct 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird. Seeing "omitempty" on a required field is non-sensical to me. The field is required, therefore it simply is not possible for it to be empty, at which point "omitempty" is a distraction. Maybe I'm misunderstanding the nuance that doc is trying to explain.

I don't think it technically matters since that'll never actually come into play with the field being required and between 1 and 52 chars long.

What is your perspective of this linter rule? Does it make sense to you, or should we set to Ignore?

@fgiudici fgiudici Oct 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, I agree it is a bit weird. I think the rationale is that for any reasons the group string is not set it gets serialized to the empty string which is invalid.
The point is exactly the "for any reason": I cannot spot any reason, maybe a weird scenario somewhere were the code serializing the COS is not ours and would make a deserialization to fail? 🤔
My guess is that the "omitempty" requirement is for being on the extra safe side. Since this check has been added to the kupe-api-linter by purpose and does not cause any harm to our code I would lean toward keeping the omitempty.
Anyway, I have not strong opinions, so I would not argue to wave the linter setting this specific polity to Ignore repo-wide. Still we cannot ignore the omitzero in the linter config, the option is not there 🤯

@perdasilva perdasilva Oct 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could be more permissive and allow for names that start with a number? There's, e.g., a 3scale operator

@fgiudici fgiudici Oct 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, added starting with number valid in the top commit.
CC @joelanford , WDYT? Double checking if this could be fine.

@joelanford joelanford Oct 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any particular reason we need to allow names that start with a number? Some Kubernetes types require a strict DNS 1035 label (see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#rfc-1035-label-names)

So we've always chosen to go with that so that if we ever need to manage Kubernetes objects derived from our objects' names, it is less likely for us to run into validation issues.

Always better to start out more restrictive and relax things later. It is very difficult/impossible to make things more restrictive.

@fgiudici fgiudici Oct 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok, dropped with the last push


Comment thread
coderabbitai[bot] marked this conversation as resolved.
// lifecycleState specifies the lifecycle state of the ClusterObjectSet.
//
// When set to "Active", the revision is actively managed and reconciled.
Expand All @@ -60,12 +72,11 @@ type ClusterObjectSetSpec struct {
// +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive"
LifecycleState ClusterObjectSetLifecycleState `json:"lifecycleState,omitempty"`

// revision is a required, immutable sequence number representing a specific revision
// of the parent ClusterExtension.
// revision is a required, immutable sequence number identifying a specific
// ClusterObjectSet within a sequence of related revisions.
//
// The revision field must be a positive integer.
// Each ClusterObjectSet belonging to the same parent ClusterExtension must have a unique revision number.
// The revision number must always be the previous revision number plus one, or 1 for the first revision.
// Each ClusterObjectSet in the same revision sequence must have a unique revision number.

@joelanford joelanford Oct 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just calling out that this is not actually enforced anywhere until we require metadata.name to equal <group>-<revision>.

@fgiudici fgiudici Oct 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right. Going to go with a follow-up PR addressing this.

//
// +required
// +kubebuilder:validation:Minimum:=1
Expand Down Expand Up @@ -602,20 +613,26 @@ type ObjectCounts struct {
Ready int64 `json:"ready,omitempty"`
}

// TODO(COD): Update the archival description below when archival decisions move
// to ClusterObjectDeployment reconciliation.

// +genclient
// +genclient:nonNamespaced
// +kubebuilder:object:root=true
// +kubebuilder:resource:scope=Cluster
// +kubebuilder:subresource:status
// +kubebuilder:selectablefield:JSONPath=`.spec.group`
// +kubebuilder:printcolumn:name="Group",type=string,JSONPath=`.spec.group`
Comment thread
fgiudici marked this conversation as resolved.
// +kubebuilder:printcolumn:name="Revision",type=integer,JSONPath=`.spec.revision`
// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status`
// +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp`

// ClusterObjectSet represents an immutable snapshot of Kubernetes objects
// for a specific version of a ClusterExtension. Each revision contains objects
// organized into phases that roll out sequentially. The same object can only be managed by a single revision
// at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded
// or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for
// posterity.
// ClusterObjectSet represents an immutable snapshot of Kubernetes objects to
// apply and manage on the cluster. Each revision contains objects organized into
// phases that roll out sequentially. The same object can only be managed by a
// single revision at a time. Ownership of objects is transitioned from one revision
// to the next as new revisions are rolled out. Once the latest revision has rolled
// out successfully, previous active revisions are archived for posterity.
Comment thread
fgiudici marked this conversation as resolved.
type ClusterObjectSet struct {
metav1.TypeMeta `json:",inline"`

Expand All @@ -625,8 +642,8 @@ type ClusterObjectSet struct {
metav1.ObjectMeta `json:"metadata,omitempty"`

// spec defines the desired state of the ClusterObjectSet.
// +optional
Spec ClusterObjectSetSpec `json:"spec,omitempty"`
// +required
Spec ClusterObjectSetSpec `json:"spec,omitzero"`

@joelanford joelanford Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need for omitzero since it is required.

Suggested change
Spec ClusterObjectSetSpec `json:"spec,omitzero"`
Spec ClusterObjectSetSpec `json:"spec"`

@fgiudici fgiudici Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

make lint-api-diff complains here too:
api/v1/clusterobjectset_types.go:594:2:kubeapilinter:requiredfields: field ClusterObjectSet.Spec does not allow the zero value. It must have the omitzero tag.
I would leave it.


// status is optional and defines the observed state of the ClusterObjectSet.
// +optional
Expand Down
Loading
Loading