pkg/registry: use v1beta1.CustomResourceDefinition, as the apiextensions type is internal#86
Conversation
openshift-ci-robot
added
the
size/S
estroz
force-pushed
the
fix-crd-encoding
branch
from
24bffec to
118e6b8
Compare
| return err | ||
| crd := &v1beta1.CustomResourceDefinition{} | ||
| dec := serializer.NewCodecFactory(Scheme).UniversalDeserializer() | ||
| _, _, err = dec.Decode(cb, nil, crd) |
There was a problem hiding this comment.
This will throw an error if a CRD version created in the future wasn't registered with Scheme. This is could be considered a feature ("we don't explicitly support that CRD version yet, throw an error"); simply using json.Unmarshal may cause silent marshaling errors like the one I describe in the PR comments. Thoughts?
There was a problem hiding this comment.
That was the reasoning behind using the internal types in the first place, but it's reasonable to only support certain types.
the apiextensions type is internal
estroz
force-pushed
the
fix-crd-encoding
branch
from
118e6b8 to
78e83c9
Compare
ecordell
left a comment
ecordell
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, estroz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
4 participants
The type
apiextensions.CustomResourceDefinitionis an internal type that should not be used for unmarshaling. Insteadv1beta1.CustomResourceDefinitionshould be used. When unmarshaling the former type, fields are not properly populated, ex.objectmeta.nameis missing. Using aruntime.DecoderwithSchemewill not cause JSON number issues (at least with one manual test)./cc @ecordell @njhale