Skip to content
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

Jump to bottom

Update example for conditions to use SSA go tags #7477

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Update example for conditions to use SSA go tags #7477

merged 1 commit into from
Aug 25, 2023

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Aug 18, 2023

Which issue(s) this PR fixes:

Fixes kubernetes/autoscaler#5848 (comment)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/developer-guide Issues or PRs related to the developer guide labels Aug 18, 2023
@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 18, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 18, 2023

cc @liggitt

@jpbetz jpbetz mentioned this pull request Aug 18, 2023
Merged
liggitt
liggitt reviewed Aug 18, 2023
View reviewed changes
contributors/devel/sig-architecture/api-conventions.md Outdated
@@ -428,6 +428,15 @@ It should be included as a top level element in status, similar to
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
Copy link
Member

Choose a reason for hiding this comment

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

do we need the listType / listMapKey comments in built-in types as well, or is the patchStrategy/patchMergeKey inherited by SSA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"If listType is missing, the API server interprets a patchMergeStrategy=merge marker as a listType=map and the corresponding patchMergeKey marker as a listMapKey." (xref)

That said, it looks like Conditions for built-in types are often defined as:

         // +listType=map                                                                                                           
         // +listMapKey=type

I can update this doc for that as well.

Copy link
Member

@liggitt liggitt Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, if we include that and patchStrategy/patchMergeKey, that works for built-in and kubebuilder types, so we can drop the kubebuilder language

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apelisse is there any reason to recommend +patchStrategy and +patchMergeKey on new built-in types? I see a mix of "only patchStrategy/patchMergeKey", "both patchStrategy/patchMergeKey and SSA listType/listMapKey" and "only SSA listType/listMapKey" in the built-in types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified to just use SSA in the built-in example form

Copy link
Member

Choose a reason for hiding this comment

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

Well… don't we want CSA to still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found another patch strategy ref in the doc. I've updated that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well… don't we want CSA to still work?

I'm not sure. We have a couple cases like: https://github.com/kubernetes/kubernetes/blob/f8b5f1a77bbab217159b1d024000770f8c95ed97/staging/src/k8s.io/api/flowcontrol/v1beta2/types.go#L360

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we audit all of this? I'm seeing a wide range of annotation patterns. There are some surprising things in here like JobCondition where SSA uses atomic.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, a sweep is probably good

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 18, 2023
@jpbetz jpbetz changed the title Add kubebuilder example for conditions Update example for conditions to use SSA go tags Aug 18, 2023
liggitt
liggitt reviewed Aug 18, 2023
View reviewed changes
contributors/devel/sig-architecture/api-conventions.md
@@ -425,7 +425,10 @@ Conditions are most useful when they follow some consistent conventions:
Conditions should follow the standard schema included in [k8s.io/apimachinery/pkg/apis/meta/v1/types.go](https://github.com/kubernetes/apimachinery/blob/release-1.23/pkg/apis/meta/v1/types.go#L1432-L1492).
It should be included as a top level element in status, similar to
```go
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
// +listType=map
// +listMapKey=type
Copy link
Member

Choose a reason for hiding this comment

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

I would include these as well to keep client-side apply working as we want

	// +patchStrategy=merge
	// +patchMergeKey=type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'll do a sweep separately.

@liggitt
Copy link
Member

liggitt commented Aug 18, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2023
@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 25, 2023

/assign @johnbelamaric
For approval.

johnbelamaric
johnbelamaric reviewed Aug 25, 2023
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, jpbetz, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1f17841 into kubernetes:master Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

@liggitt liggitt liggitt left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@liggitt liggitt

@johnbelamaric johnbelamaric

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpbetz @liggitt @k8s-ci-robot @johnbelamaric