-
Notifications
You must be signed in to change notification settings - Fork 502
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
Bump controller-gen to v0.9.2. #1203
Bump controller-gen to v0.9.2. #1203
Conversation
openshift/build-machinery-go#67 /retest-required |
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.
/lgtm
/approve
/label qe-approved
/label docs-approved
/label px-approved
/test verify |
/hold |
4c8bf2e
to
173e285
Compare
Need to understand the impact of this topology marker fix first (https://kubernetes.io/docs/reference/using-api/server-side-apply/#compatibility-across-topology-changes). /hold |
@benluddy I was looking into making this bump as a part of some other changes I'm making, did you work out what the impact is likely to be of adding these markers? IIUC all lists are atomic by default, so anyone using SSA who would observe a map type can't have been using it yet right? The entire list will currently have a single owner but may then in the future have multiple owners. That said, if the list is currently atomic, none of our consumers can have any logic assuming it is a map or otherwise so will end up preserving the entire list right now Perhaps if we can enumerate the places this has changed we can determine how many consumers there are and where it is safe and where it is not safe to make the change |
173e285
to
a0b8971
Compare
I think this is ok. The markers only affect clients using SSA. We don't currently have generated SSA clients so it wouldn't be easy for any consumers of these types to be using SSA yet anyway. Add to that that the default list type behaviour is atomic, this shouldn't be a breaking change for any of these instances. /lgtm |
a0b8971
to
52df1c3
Compare
52df1c3
to
9bcd547
Compare
With the latest controller-gen, leading and trailing whitespace is stripped from the generated description text, and topology markers (e.g. +structType, +mapType) are now being recognized when applied to types as well as fields.
9bcd547
to
b630f11
Compare
we need to sweep all slices before 4.12. /approve |
/hold cancel |
/lgtm We need the updated generator so that we can continue with the introduction of tech preview fields for openshift APIs |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, deads2k, JoelSpeed, soltysh 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 |
@benluddy: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
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.
/lgtm
/approve
/hold
for other reviewers.
Sorry, wrong PR. |
With the latest controller-gen, leading and trailing whitespace is
stripped from the generated description text (kubernetes-sigs/controller-tools#517), and topology
markers (e.g. +structType, +mapType) are now being recognized when
applied to types as well as fields (kubernetes-sigs/controller-tools#692).