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
Deprecate default conversions #90018
Deprecate default conversions #90018
Conversation
// conversions | ||
|
||
if pair.source == pair.dest { | ||
// TODO: Ideally, to avoid this hack, we should auto-generate self-conversions. |
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.
include the EnforcePtr(src) and EnforcePtr(dest)/CanAddr/CanSet checks 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.
done
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
3573976
to
c71ee41
Compare
eb9bb82
to
7b6e7b8
Compare
7b6e7b8
to
4b6105e
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wojtek-t 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 |
4b6105e
to
f624314
Compare
/retest |
/hold I'd like a chance to have a look at this and judge impact. I expect to have an answer back this week. The PR itself looks pretty contained, but it's relying on a fair number of 1.18 PRs so it will take me a bit. |
Well, it's not great, but it looks tractable. Looks like a dozen unique bits or so openshift/openshift-apiserver#95 /hold cancel |
/lgtm |
this needs a release note for |
@liggitt - Added something - PTAL |
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion in unit tests. The manual conversion and wash through JSON seems like a terrible hack, but I haven't been able to figure out a more elegant approach yet. From [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion in unit tests. The manual conversion and wash through JSON seems like a terrible hack, but I haven't been able to figure out a more elegant approach yet. From [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion in unit tests. The manual conversion and wash through JSON seems like a terrible hack, but I haven't been able to figure out a more elegant approach yet. From [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. Also add similar logic for apiregistration, since that's the other group where we register multiple schema versions. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion Because merging and application are also version-dependent, it's hard to paper over this in resourceread with automatic type conversion. Although from [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. Anyhow, with this commit, I'm doubling down on the approach from 4ee7b07 (Add apiextensions.k8s.io/v1 support for CRDs, 2019-10-22, openshift#259) and collapsing the readers into two helpers that support all of our types and return runtime.Object. From 0a255ab (cvo: Use protobuf for sending events and other basic API commands, 2019-01-18, openshift#90), protobuf is more efficient, so we should use it where possible. And because all of this is very tedious to maintain by hand, there's now a Python generator to spit out all the boilerplate. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Protecting us from [1,2]: converting (v1beta1.Role) to (v1.Role): unknown conversion Because merging and application are also version-dependent, it's hard to paper over this in resourceread with automatic type conversion. Although from [3]: Promotes the rbac.authorization.k8s.io/v1beta1 API to v1 with no changes so all we really need is the apiVersion bump. Anyhow, with this commit, I'm doubling down on the approach from 4ee7b07 (Add apiextensions.k8s.io/v1 support for CRDs, 2019-10-22, openshift#259) and collapsing the readers into two helpers that support all of our types and return runtime.Object. From 0a255ab (cvo: Use protobuf for sending events and other basic API commands, 2019-01-18, openshift#90), protobuf is more efficient, so we should use it where possible. And because all of this is very tedious to maintain by hand, there's now a Python generator to spit out all the boilerplate. [1]: kubernetes/kubernetes#90018 [2]: openshift#420 (comment) [3]: kubernetes/kubernetes#49642
Ref #87006
This switches off the path for default conversions - from now on, only explicitly registered conversions work.