Support x-kubernetes-v2-spec extension #166

yue9944882 · 2019-07-11T09:22:31Z

now in v2 builder, any schema marked the v2-spec extension will be shadowed to the extension value.

k8s-ci-robot · 2019-07-11T09:23:07Z

Welcome @yue9944882!

It looks like this is your first PR to kubernetes/kube-openapi 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-openapi has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

yue9944882 · 2019-07-11T09:27:08Z

@sttts we dont need changes in the generator. e.g. for IntOrString, we will add this extension into it's OpenAPIDefinition method. the v3 builder will filter out the extension, the v2 builder will shadow the schema w/ extension value.

sttts · 2019-07-11T10:10:59Z

pkg/builder/openapi.go

@@ -154,6 +155,11 @@ func (o *openAPI) buildDefinitionRecursively(name string) error {
 				schema.Extensions[k] = v
 			}
 		}
+		if v, ok := item.Schema.Extensions[extensionV2Spec]; ok {


wondering, can we mirror OpenAPIV2Definition() output there instead? so we will assume that OpenAPIDefinition() will output v3 schemas (which we down-convert by default), with OpenAPIV2Definition for cases where the down-conversion is not good enough.

or we go the opposite direction: OpenAPIDefinition will stay v2, and OpenAPIV3Definition can give an advanced v3 schema. That sounds more compatible. We won't have many cases where we need this.

the latter sounds better and i took a shot. updated the integration test a bit cover v2 spec generation.

the next step is to start v3 builder and verify the v2-extension by its integration test

@yue9944882 I started working on builderv3, I should issue a WIP PR by the end of the week.

pkg/common/common.go

sttts · 2019-07-12T10:42:37Z

pkg/generators/openapi.go

+		if mn != "OpenAPIV3Definition" {
+			continue
+		}
+		return methodReturnsValue(mt, openAPICommonPackagePath, "OpenAPIDefinition")


OpenAPIDefinition or OpenAPIV3Definition?

it's the name of struct OpenAPIDefinition under "pkg/common" package, instead of the function name

sttts · 2019-07-12T10:46:35Z

Looks good.

/assign @p0lyn0mial

for some more eyes on this.

yue9944882 · 2019-07-12T11:43:17Z

pkg/common/common.go

@@ -172,3 +182,11 @@ func EscapeJsonPointer(p string) string {
 	p = strings.Replace(p, "/", "~1", -1)
 	return p
 }
+
+func EmbedOpenAPIDefinitionIntoV2Extension(main OpenAPIDefinition, embedded OpenAPIDefinition) OpenAPIDefinition {


for now, i cant think of a better name for this

p0lyn0mial · 2019-07-14T13:45:34Z

test/integration/pkg/generated/openapi_generated.go

 )

 func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition {
 	return map[string]common.OpenAPIDefinition{
+		"k8s.io/kube-openapi/test/integration/testdata/custom.Bac":              common.EmbedOpenAPIDefinitionIntoV2Extension(custom.Bac{}.OpenAPIV3Definition(), custom.Bac{}.OpenAPIDefinition()),


so what does it mean?

it means that in the future (once we have oas v3) we could have a way to keep and generate both v2 and v2 schemas (spec.Schema) in just one place, is that correct?

do we expect them to be different (v2 vs v3) ?

from what I have seen they are quite compatible:
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#schemaObject

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#schemaObject

it means that in the future (once we have oas v3) we could have a way to keep and generate both v2 and v2 schemas (spec.Schema) in just one place, is that correct?

exactly, but only for those types have differences in v2 and v3 schema. for now, it's basically meant for IntOrString. in v2 spec, we made walkaround for those types we have to denote in v3 grammar e.g.anyOf. the pull will keep those v2 walkarounds in the builder in the extension so that:

for v2 publishing endpoint: we shadow the generated schema using the new extension.

for v3 publishing endpoint: we filter out the extension from the generated schema.

// OpenAPISchemaType is used by the kube-openapi generator when constructing // the OpenAPI spec of this type. // // See: https://github.com/kubernetes/kube-openapi/tree/master/pkg/generators func (IntOrString) OpenAPISchemaType() []string { return []string{"string"} } // OpenAPISchemaFormat is used by the kube-openapi generator when constructing // the OpenAPI spec of this type. func (IntOrString) OpenAPISchemaFormat() string { return "int-or-string" }

for IntOrString, the v3 spec will be an anyOf

p0lyn0mial

/lgtm

p0lyn0mial · 2019-07-17T14:20:07Z

pkg/generators/openapi.go

 			// already invoked directly
 			return nil
 		}

 		args := argsFromType(t)
 		g.Do("func "+nameTmpl+"(ref $.ReferenceCallback|raw$) $.OpenAPIDefinition|raw$ {\n", args)
-		if hasOpenAPIDefinitionMethods(t) {
+		switch {


no separate case for hasV3Definition?

no separate case for hasV3Definition?

hasV3Definition case is short-circuited at L329, do you think about adding a case for hasV3Definition and commenting // this line will never reach for readability? the branches is getting more complex tho

no, we can leave it as it is now.

test/integration/pkg/generated/openapi_generated.go

test/integration/testdata/custom/v3.go

test/integration/testdata/custom/v2.go

update test to reflect how custom types works

yue9944882 · 2019-07-18T05:26:32Z

@sttts @p0lyn0mial PTAL

p0lyn0mial · 2019-07-22T07:17:05Z

/lgtm

leaving approval to @sttts

sttts · 2019-07-22T07:37:49Z

/approve

k8s-ci-robot · 2019-07-22T07:37:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [sttts]

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

sttts · 2019-07-22T07:39:41Z

@yue9944882 now we need a follow-up which vendors this into kube and which does not lead to any change for the published spec, right?

yue9944882 · 2019-07-22T07:44:26Z

@yue9944882 now we need a follow-up which vendors this into kube and which does not lead to any change for the published spec, right?

yep, this extension is opt-in and backward-compatible. will send the follow-up

k8s-ci-robot requested a review from sttts July 11, 2019 09:22

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 11, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 11, 2019

sttts reviewed Jul 11, 2019

View reviewed changes

yue9944882 force-pushed the feat/v2-spec-extension branch from 5f2986b to 3d0b6eb Compare July 11, 2019 13:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 11, 2019

yue9944882 force-pushed the feat/v2-spec-extension branch from 3d0b6eb to 9bd9b46 Compare July 11, 2019 15:21

sttts reviewed Jul 12, 2019

View reviewed changes

pkg/common/common.go Outdated Show resolved Hide resolved

sttts reviewed Jul 12, 2019

View reviewed changes

k8s-ci-robot assigned p0lyn0mial Jul 12, 2019

yue9944882 commented Jul 12, 2019

View reviewed changes

p0lyn0mial reviewed Jul 14, 2019

View reviewed changes

p0lyn0mial reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2019

yue9944882 commented Jul 17, 2019

View reviewed changes

test/integration/pkg/generated/openapi_generated.go Outdated Show resolved Hide resolved

yue9944882 commented Jul 17, 2019

View reviewed changes

test/integration/testdata/custom/v3.go Show resolved Hide resolved

yue9944882 commented Jul 17, 2019

View reviewed changes

test/integration/testdata/custom/v2.go Show resolved Hide resolved

provide OpenAPIV3Definition for overriding specs

6610e5a

update test to reflect how custom types works

yue9944882 force-pushed the feat/v2-spec-extension branch from 9bd9b46 to 6610e5a Compare July 18, 2019 03:47

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2019

k8s-ci-robot merged commit 5e22f3d into kubernetes:master Jul 22, 2019

mariantalla mentioned this pull request Nov 6, 2019

Support +mapType, +structType markers #178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support x-kubernetes-v2-spec extension #166

Support x-kubernetes-v2-spec extension #166

yue9944882 commented Jul 11, 2019

k8s-ci-robot commented Jul 11, 2019

yue9944882 commented Jul 11, 2019

sttts Jul 11, 2019

sttts Jul 11, 2019

yue9944882 Jul 11, 2019

yue9944882 Jul 11, 2019 •

edited

Loading

p0lyn0mial Jul 14, 2019

sttts Jul 12, 2019

yue9944882 Jul 12, 2019

sttts commented Jul 12, 2019

yue9944882 Jul 12, 2019

p0lyn0mial Jul 14, 2019

yue9944882 Jul 15, 2019

p0lyn0mial left a comment

p0lyn0mial Jul 17, 2019

yue9944882 Jul 17, 2019

p0lyn0mial Jul 22, 2019

yue9944882 commented Jul 18, 2019

p0lyn0mial commented Jul 22, 2019

sttts commented Jul 22, 2019

k8s-ci-robot commented Jul 22, 2019

sttts commented Jul 22, 2019

yue9944882 commented Jul 22, 2019

Support x-kubernetes-v2-spec extension #166

Support x-kubernetes-v2-spec extension #166

Conversation

yue9944882 commented Jul 11, 2019

k8s-ci-robot commented Jul 11, 2019

yue9944882 commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yue9944882 Jul 11, 2019 • edited Loading

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 Jul 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial left a 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

yue9944882 commented Jul 18, 2019

p0lyn0mial commented Jul 22, 2019

sttts commented Jul 22, 2019

k8s-ci-robot commented Jul 22, 2019

sttts commented Jul 22, 2019

yue9944882 commented Jul 22, 2019

yue9944882 Jul 11, 2019 •

edited

Loading