-
Notifications
You must be signed in to change notification settings - Fork 440
✨ metadata field changes for structural schema #263
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
Conversation
This PR handles generating schema for metadata as per structural schema. https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190425-structural-openapi.md
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droot 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 |
ident.Package.ID == metav1Pkg && ident.Name == "ObjectMeta": | ||
return &apiext.JSONSchemaProps{ | ||
Type: "object", | ||
Properties: map[string]apiext.JSONSchemaProps{ |
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.
these two are possible, but not needed in a structural schema.
return nil | ||
// TODO(droot): enable it once we move k8s 1.15 | ||
// return &apiext.JSONSchemaProps{ | ||
// // XEmbeddedResource: true, |
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.
for this @DirectXMan12 brought up the point of partial ObjectMeta. There is no requirement that the metadata is complete for an embedded resource. /cc @liggitt
We either want a way to mark an embedded ObjectMeta as partial, or we relax the validation we do today (if that is possible now in beta). @liggitt wdyt?
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.
what is currently required that we would want to relax?
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.
as far as I remember, only kind/apiVersion were required
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's about ObjectMeta: name, namespace as far as I see.
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.
name must be optional in openapi because of generateName, and iirc our server-side implementation put a dummy name in place when validating if no name was provided
namespace must already be optional (both server-side and in openapi) because of cluster-scoped resources
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.
ok, I think I misunderstood @DirectXMan12 in Slack. He was talking about TypeMeta
(and I remember the dummy name value we put it to work around the check).
So the question is whether without TypeMeta it is really an embedded object.
@sttts @DirectXMan12 @mengqiy I reevaluated my approach and thought of another approach: By default, do not generate any validation for ObjectMeta (irrespective of it being a root or embedded). Let users specify marker |
sgtm on the marker |
@droot: 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. |
closing this in favor of #266 |
This PR handles generating schema for metadata as per structural schema.
https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190425-structural-openapi.md
Partial fix for #216