-
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
KEP-2887: Enum Types for OpenAPI #2888
KEP-2887: Enum Types for OpenAPI #2888
Conversation
cc9d300
to
377b574
Compare
377b574
to
260d1ec
Compare
260d1ec
to
3d73130
Compare
/assign @deads2k For PRR. |
3d73130
to
2eeafee
Compare
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.
One minor comment. Looks good to me otherwise.
(disclaimer: I've seen a preview of this, so already got a chance to provide some feedback).
2eeafee
to
c7dc0ff
Compare
c7dc0ff
to
9e09352
Compare
61fe574
to
747dee2
Compare
/lgtm |
Co-authored-by: Daniel Smith <dbsmith@google.com>
@@ -0,0 +1,3 @@ | |||
kep-number: 2887 | |||
alpha: | |||
approver: "@deads2k" |
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.
How come you're missing so many fields here?
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.
That's the PRR, scroll down a bit for the main KEP YAML.
- Add a marker that indicates a field as an enum. | ||
- Make the Kubernetes OpenAPI generator recognize the marker. | ||
- Deduce values of an enum type from its Go type definition. | ||
- Detect and annotate enum types in all built-in types. |
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.
is this a one time thing? We'll use a one time script to annotation to all types once, review the PR and then forget about the script?
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.
There is the "linter" thing that is also under discussion. The plan is to run the linter for the first time expecting tons of warnings, then check each to see if it is right or false positive. Afterwards, it checks for enums in new types.
When generating OpenAPI schema, the de facto enum types should be reflected as enums in the resulting schema. | ||
|
||
We define enum types with following properties: | ||
- All enum types are closed, i.e., the type includes a finite number of possible values. |
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.
They are closed from a server-side perspective (server won't allow any unknown value). Clients, for backwards compatibility reasons, should consider it open (new values are possible).
/lgtm Hold for minor language tweaks, lgtm because I'll be afk for a few hours and don't want to hold anything up. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jiahuif, lavalamp 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 |
Co-authored-by: Daniel Smith <dbsmith@google.com>
Co-authored-by: Daniel Smith <dbsmith@google.com>
may add it back later.
/lgtm |
/hold cancel For having got approval from all requested reviewers. |
* [PRR] KEP-2887 Enum Type for OpenAPI * KEP-2887: OpenAPI Enum Types * annotation -> marker * reword value auto-detection. * kube-builder syntax compatibility. * re-target to OpenAPI v2. * field pruning for disabling this feature. * goal: unify with kubebuilder. * Update keps/sig-api-machinery/2887-openapi-enum-types/README.md Co-authored-by: Daniel Smith <dbsmith@google.com> * rewrite non-goals. * Update keps/sig-api-machinery/2887-openapi-enum-types/README.md Co-authored-by: Daniel Smith <dbsmith@google.com> * reword enum field pruning. * preprocessor for these don't want enums. * update TOC. * Update keps/sig-api-machinery/2887-openapi-enum-types/README.md Co-authored-by: Daniel Smith <dbsmith@google.com> * polish assumptions on impl. * Update keps/sig-api-machinery/2887-openapi-enum-types/README.md Co-authored-by: Daniel Smith <dbsmith@google.com> * reword non-goal * allow client open enum. * omit the enum linter. may add it back later. * update TOC. Co-authored-by: Daniel Smith <dbsmith@google.com>
/label tide/merge-method-squash