WIP - apiextensions: low-level converter wiring – towards versioning #63802
Conversation
sttts
commented
May 14, 2018
sttts
commented
- add crdConversionRESTOptionsGetter to wire in custom conversions in storage path
- wire through custom conversions in serializers for request de- and encoding.
|
@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot
added
size/M
sttts
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts 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 |
k8s-ci-robot
added
the
approved
sttts
force-pushed
the
sttts-crd-versioning-low-level-plumbing
branch
from
6124d37
to
393bdc6
Compare
k8s-ci-robot
added
size/L
sttts
force-pushed
the
sttts-crd-versioning-low-level-plumbing
branch
2 times, most recently
from
40ed457
to
8834b09
Compare
|
/cc @mbohlool |
| } | ||
| if targetGVK == objGVK { | ||
| return c.encoder.Encode(obj, w) | ||
| // Only avoid conversion if unstructured is not a list, giving the converter a chance to convert list items. |
There was a problem hiding this comment.
When did you find a list that asserted the "correct" group/version, but actually contained types in the wrong group/version?
| @@ -59,7 +59,7 @@ func newStorage(t *testing.T) (customresource.CustomResourceStorage, *etcdtestin | |||
| &metav1.DeleteOptions{}, | |||
| ) | |||
|
|
|||
| typer := apiserver.UnstructuredObjectTyper{ | |||
| typer := apiserver.unstructuredObjectTyper{ | |||
There was a problem hiding this comment.
This doesn't compile, does it?
There was a problem hiding this comment.
No, it doesn't. We shouldn't leak from the apiserver into the registry in the first place though.
| @@ -126,6 +126,8 @@ func NewCRDRESTOptionsGetter(etcdOptions genericoptions.EtcdOptions) genericregi | |||
| EnableGarbageCollection: etcdOptions.EnableGarbageCollection, | |||
| DeleteCollectionWorkers: etcdOptions.DeleteCollectionWorkers, | |||
| } | |||
|
|
|||
| // the codec is going to be overridden with the actual CRD converter based codec | |||
There was a problem hiding this comment.
on a per-group basis? Please specify the granularity.
| ret.StorageConfig.Codec, | ||
| t.converter, | ||
| &unstructuredCreator{}, | ||
| discovery.NewUnstructuredObjectTyper(nil), |
There was a problem hiding this comment.
reminder to myself, I promised to move this to the extensions apiserver because it's the only consumer and it's misnamed.
There was a problem hiding this comment.
We might want to move all of these into a scheme package here.
| // and custom encoder and decoder version. | ||
| type crdConversionRESTOptionsGetter struct { | ||
| delegate generic.RESTOptionsGetter | ||
| converter runtime.ObjectConvertor |
There was a problem hiding this comment.
please doc granularity. All CRDs or one group? or something else?
| in.GetObjectKind().SetGroupVersionKind(gvk) | ||
|
|
||
| for i := range unstructList.Items { | ||
| out, err := c.unstructuredDelegate.ConvertToVersion(&unstructList.Items[i], target) |
There was a problem hiding this comment.
Seems weird to directly delegate instead of recursing.
There was a problem hiding this comment.
We can't recurse because the underlying converter will be different (think of declarative conversion some day). Hence, we make this composition, so the generic crd converter can call the delegate converter doing the actual work on non-lists.
| func (c crdObjectConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) { | ||
| kind := in.GetObjectKind().GroupVersionKind() | ||
| if kind.Empty() { | ||
| return in, nil |
There was a problem hiding this comment.
why does an empty kind result in no action instead of an error?
There was a problem hiding this comment.
here the analysis and a possible fix: mbohlool#5
| unstructured.UnstructuredObjectConverter | ||
| clusterScoped bool | ||
| unstructuredDelegate unstructured.UnstructuredObjectConverter | ||
| clusterScoped bool |
| UnstructuredTyper: discovery.NewUnstructuredObjectTyper(), | ||
| typer := unstructuredObjectTyper{ | ||
| delegate: parameterScheme, | ||
| unstructuredTyper: discovery.NewUnstructuredObjectTyper(), |
There was a problem hiding this comment.
This is on its way to being a lot more opinionated. How about starting a scheme package or some such?
|
I think I see where you're going with it. It makes sense. Can you summarized the intent in the description? It appears to me that you're working towards building a mutable object converter that is going to be used inside of the "normal" codec flow and will thus allow you to take a lock and change the converter as needed. |
|
@kubernetes/sig-api-machinery-misc @lavalamp |
k8s-ci-robot
added
the
sig/api-machinery
sttts
force-pushed
the
sttts-crd-versioning-low-level-plumbing
branch
from
8834b09
to
b0f33ff
Compare
|
/assign @mbohlool |
|
@sttts: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Can this be rebased on top of #63830? Or is it trying to be an alternative approach? |
sttts
changed the title
|
@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot
added
do-not-merge/work-in-progress
Yes, this is 90% the same code as in #63830, minus adding the multiple versions. The goal was to iterate quicker with reviews than mixing lower level plumbing with API discussion in a big API implementing PR. The later takes a magnitude longer for review and to get it merged. Also having some soak time for those low-level changes at the heart of CRDs wouldn't harm. These changes here are also blocking work on pruning and defaulting, which puts it on the critical path towards code-freeze (and with pruning being prerequisite for GA also for promotion of CRDs). I don't feel strongly about which PR we finally merge (code is mostly the same), as long as we get it in this week. So if we put enough priority on #63830 both for review and for addressing comments, that's fine as well. |
|
@sttts: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
