add vendor extensions to marshal openapi v2 spec.Header #282
Conversation
k8s-ci-robot
added
size/S
Discovered while fuzzing a type converter to gnostic. Seems like it was forgotten to include this when originally implemented.
alexzielenski
force-pushed
the
header_vendorext_marshal
branch
from
cf3229a to
1cd4920
Compare
k8s-ci-robot
added
size/M
| b4, err := json.Marshal(h.VendorExtensible) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
are we using json marshaling of this type anywhere in kubernetes today (in CRD validation, or openapi v2 generation, etc)? just wanting to make sure this isn't an API-facing change in current use.
(if it is, this may still be ok, I just wanted to understand the implications)
There was a problem hiding this comment.
Looking at the instances of Response through https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json none of the default types define headers for their responses.
In the case of CRDs, it may be possible that CRDs populate response headers for their open api spec. I am wholly unfamiliar with that part of the infrastructure, though. @apelisse do you know whether existing CRDs are able to populate response Headers?
There was a problem hiding this comment.
CRDs don't have the ability to do anything specific to their response header, if I understand correctly. I don't think it's being used at all.
Also, I'm not sure what this would break, or how this would be an api-facing change.
There was a problem hiding this comment.
yeah, I swept k8s.io/kubernetes and couldn't find any places we construct this
Also, I'm not sure what this would break, or how this would be an api-facing change.
just wanted to know if this was going to start including directives places we hadn't before, and think if any known clients would be unable to handle those (think kubectl's fragile handling of openapi v2)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexzielenski, 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 |
k8s-ci-robot
added
the
approved
4 participants
I noticed while fuzzing a type converter to gnostic that the
spec.Headertype does not include itsx-vendor extensions when marshaled to JSON (they were already being properly unmarshaled from JSON). This PR adds that functionality.