KEP-2887: OpenAPI Enum Types to Beta #3184

jiahuif · 2022-01-25T21:47:12Z

One-line PR description:

This PR revises KEP-2887 for its beta graduation.

Issue link:

Other comments:

jiahuif · 2022-01-25T23:47:44Z

/assign @deads2k

for PRR

k8s-ci-robot · 2022-01-25T23:47:45Z

@jiahuif: GitHub didn't allow me to assign the following users: PRR, for.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @deads2k for PRR

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.

jiahuif · 2022-01-25T23:48:05Z

/assign @jpbetz @apelisse

deads2k · 2022-01-27T23:15:22Z

keps/sig-api-machinery/2887-openapi-enum-types/README.md

+type Foo string
+type FooAlias = Foo
+```
+would result in the parser to treat FooAlias as a duplicated type. A workaround will be implemented during alpha-to-beta graduation.


What is the workaround and how long do you anticipate the workaround existing?

deads2k · 2022-01-27T23:16:44Z

keps/sig-api-machinery/2887-openapi-enum-types/README.md

@@ -580,13 +581,17 @@ feature flags will be enabled on some API servers and not others during the
 rollout. Similarly, consider large clusters and how enablement/disablement
 will rollout across nodes.
 -->
+If the API-server has multiple replicas, with some instances not yet enabling this feature,
+whether the returned OpenAPI Spec contains enum types will depend on which instance holds the leader lock. 


what leader lock?

Oops, I've been working too much with leader election recently. I meant depends on which instance handled the request.

deads2k · 2022-01-27T23:18:27Z

I have questions about the design updates. the PRR lgtm

sftim

Two small nits

keps/sig-api-machinery/2887-openapi-enum-types/README.md

Co-authored-by: Tim Bannister <tim@scalefactory.com>

jiahuif · 2022-02-02T22:46:29Z

/label tide/merge-method-squash

apelisse · 2022-02-02T22:59:51Z

keps/sig-api-machinery/2887-openapi-enum-types/README.md

@@ -351,6 +351,16 @@ const (

 Here, `StorageMedium` can have infinite number of possible values, which disqualify it as an enum type.

+The Go parser has a limitation on parsing type aliases. The parser cannot distinguish 


What is "the Go parser"?

changed to "The parser of Go compiler"

is this really what we use for gengo to parse the types.go files?

Yes. the imported package was go/parser

apelisse · 2022-02-02T23:01:18Z

keps/sig-api-machinery/2887-openapi-enum-types/README.md

+```
+would result in the parser to treat FooAlias and Foo as the same type. 
+As a result, `gengo` produce either `Foo` or `FooAlias` but not both.
+As a workaround, during beta graduation, the enum parser will be updated to accept any name of the type.


I'm not super clear on what this means, what's the exact behavior that you think we can have? Maybe include the bug too where a lot of the discussion has been going on.

Or can we try to be less hand-wavy on what we'll do here? I'm not sure I understand what you're proposing

I agree. Externalizing this to an issue in kubernetes/gengo

I'd still be happy if we could discuss here what we can expect as a solution.

I just realized that this issue should affect other markers too. I will keep it short in the KEP and describe it in a separate issue

sttts · 2022-02-03T15:10:07Z

Sgtm

deads2k · 2022-02-03T15:12:58Z

keps/sig-api-machinery/2887-openapi-enum-types/README.md

+However, as of 1.23, the enum marker is the only marker to be added to a type declaration, and would be the first marker to be affected.
+Until there is a fix to `gengo`, the enum generator has the following limitations:
+- the enum marker must not be added to aliases
+- an aliased enum type or value SHOULD NOT have comments. Otherwise, the comments will be squashed with these of the original with undefined ordering. 


being able to detect this case is important so people's verify scripts work consistently, right?.

Correct. This should be detectable with simple regex matching. I will add a note that in this release cycle either gengo gets a fix or I will add a check script to detect this.

deads2k · 2022-02-03T15:14:41Z

PRR is good, but the discussion about how to handle the generator edge cases looks to need to resolution.

/approve

@apelisse has good questions about handling the generator. @apelisse please lgtm when appropriate.
/assign @apelisse

k8s-ci-robot · 2022-02-03T15:15:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jiahuif

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:

~~keps/prod-readiness/OWNERS~~ [deads2k]
~~keps/sig-api-machinery/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apelisse · 2022-02-03T18:30:15Z

That's still a little hand-wavy, but it's also fairly minor and somewhat unrelated to the specific feature. I'm also confident we can figure out a reasonable solution.

Thanks.
/lgtm

KEP-2887: PRR for beta.

f7ac22a

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 25, 2022

k8s-ci-robot requested review from ehashman and wojtek-t January 25, 2022 21:47

revise for Beta.

de991a9

jiahuif force-pushed the kep/2887/to-beta branch from b4342c4 to de991a9 Compare January 25, 2022 22:51

k8s-ci-robot assigned deads2k Jan 25, 2022

k8s-ci-robot assigned apelisse and jpbetz Jan 25, 2022

Priyankasaggu11929 mentioned this pull request Jan 27, 2022

OpenAPI Enum Types #2887

Open

7 tasks

deads2k reviewed Jan 27, 2022

View reviewed changes

sftim reviewed Jan 31, 2022

View reviewed changes

keps/sig-api-machinery/2887-openapi-enum-types/README.md Outdated Show resolved Hide resolved

keps/sig-api-machinery/2887-openapi-enum-types/README.md Outdated Show resolved Hide resolved

jiahuif and others added 3 commits February 2, 2022 14:40

clarify workaround of type aliases.

5bd8959

Apply suggestions from code review

30e2ea2

Co-authored-by: Tim Bannister <tim@scalefactory.com>

fix failure mode of replicated API server.

cf3137e

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 2, 2022

apelisse reviewed Feb 2, 2022

View reviewed changes

jiahuif force-pushed the kep/2887/to-beta branch from 27fa9bb to 2c0ba07 Compare February 3, 2022 00:23

externalize gengo alias issue.

daebe14

jiahuif force-pushed the kep/2887/to-beta branch from 2c0ba07 to daebe14 Compare February 3, 2022 00:24

deads2k reviewed Feb 3, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2022

mention enforcement of aliases rules.

80eb0fb

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2022

k8s-ci-robot merged commit daa57ff into kubernetes:master Feb 3, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 3, 2022

jiahuif deleted the kep/2887/to-beta branch March 2, 2022 22:49

palnabarun mentioned this pull request Jun 12, 2022

canonical kep number json field palnabarun/enhancements#1

Closed

palnabarun mentioned this pull request Jul 26, 2022

KEP Manifest Generator palnabarun/enhancements#2

Closed

jpbetz mentioned this pull request Feb 9, 2024

Fix status of OpenAPI Enum Types KEP #4507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2887: OpenAPI Enum Types to Beta #3184

KEP-2887: OpenAPI Enum Types to Beta #3184

jiahuif commented Jan 25, 2022

jiahuif commented Jan 25, 2022 •

edited

Loading

k8s-ci-robot commented Jan 25, 2022

jiahuif commented Jan 25, 2022

deads2k Jan 27, 2022

deads2k Jan 27, 2022

jiahuif Feb 2, 2022

jiahuif Feb 2, 2022

deads2k commented Jan 27, 2022

sftim left a comment

jiahuif commented Feb 2, 2022

apelisse Feb 2, 2022

jiahuif Feb 2, 2022

apelisse Feb 2, 2022

jiahuif Feb 2, 2022

apelisse Feb 2, 2022

apelisse Feb 2, 2022

jiahuif Feb 2, 2022

apelisse Feb 2, 2022

jiahuif Feb 2, 2022

sttts commented Feb 3, 2022

deads2k Feb 3, 2022

jiahuif Feb 3, 2022

deads2k commented Feb 3, 2022

k8s-ci-robot commented Feb 3, 2022

apelisse commented Feb 3, 2022

		@@ -351,6 +351,16 @@ const (

		Here, `StorageMedium` can have infinite number of possible values, which disqualify it as an enum type.

		The Go parser has a limitation on parsing type aliases. The parser cannot distinguish

KEP-2887: OpenAPI Enum Types to Beta #3184

KEP-2887: OpenAPI Enum Types to Beta #3184

Conversation

jiahuif commented Jan 25, 2022

jiahuif commented Jan 25, 2022 • edited Loading

k8s-ci-robot commented Jan 25, 2022

jiahuif commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 27, 2022

sftim left a comment

Choose a reason for hiding this comment

jiahuif commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 3, 2022

k8s-ci-robot commented Feb 3, 2022

apelisse commented Feb 3, 2022

jiahuif commented Jan 25, 2022 •

edited

Loading