-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
sig-api-machinery KEP: Vanilla OpenAPI subset: structural schema #1002
Conversation
411ad42
to
301f222
Compare
/assign @liggitt |
/assign @apelisse @DirectXMan12 @mbohlool |
|
||
Note: the OpenAPI v2 output given above is the same we get from the [OpenAPI v2 filtering in Kubernetes 1.14](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go#L30): that filtering in the CRD OpenAPI v2 publishing pipeline supports the unrestricted range of the CRD OpenAPI v3 language and turns it into a weaker variant understandable by kubectl 1.13 and 1.14. Hence, our changes above have no influence on the OpenAPI v2 publishing other than passing through of our new `x-kubernetes-*` vendor extensions. | ||
|
||
Also note that structural schemas enforce types and properties outside of logical junctors. OpenAPI v2 publishing will preserve these and hence, structural schemas lead to a more complete OpenAPI v2 spec, client-side validation and `kubectl explain` output. |
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.
+1
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.
@roycaihw ^^ is a great improvement for your publishing code.
looks great overall. a few nits and clarifications. only one substantial question at #1002 (comment) |
lgtm, can squash when other reviewers are happy |
/lgtm |
14a6f46
to
d110b5f
Compare
Squashed |
@DirectXMan12 @liggitt please reapply the tag after squashing. |
/lgtm |
@lavalamp @deads2k please take a look for approval. @liggitt and @DirectXMan12 lgtm'ed. |
Glad to see this moving forward. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, DirectXMan12, 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 |
// restrict the embedded object. Both ObjectMeta and TypeMeta | ||
// are validated automatically. x-kubernetes-preserve-unknown-fields | ||
// must be true. | ||
XEmbeddedResource bool |
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.
It occurs to me at this point: why is this not a "format" value?
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.
format has local influence on the type it is defined in (are there exceptions?). XEmbeddedResource
causes further behaviour changes which are not local (metadata is checked and pruned; apiVersion, kind is required). Also I think this is a very kubernetes specific concept. Using format
for that doesn't feel right IMO, but is rather a cosmetical detail.
I strongly approve of the concept and don't have time to nitpick any details, so LGTM! |
This is the conversion of https://docs.google.com/document/d/1pcGlbmw-2Y0JJs9hsYnSBXamgG9TfWtHY6eh80zSTd8/edit?ts=5cbe0089#heading=h.l1xazlj9aw9f into the KEP format.