Support +mapType, +structType markers #178

mariantalla · 2019-11-06T14:14:55Z

This PR adds support for markers // +mapType=<atomic/granular>, // +structType=<atomic/granular>; they should both map to the OpenAPI extension x-kubernetes-map-type.

Also adding minor changes to the integration tests README to ensure that
it's up to date after recent changes that add v2 support (#166).

fixes #177

sttts · 2019-11-06T16:28:13Z

What is struct type? In CRDs we only have list-type and map-type.

mariantalla · 2019-11-11T17:56:15Z

structType seems to have been added as part of the server-side apply work (for native objects). Unless I got this wrong 🤔

mariantalla · 2019-11-11T17:59:06Z

/hold
there's a mistake in the implementation with structType in any case - +structType should also convert to x-kubernetes-map-type

and convert both of them to openapi extension: x-kubernetes-map-type. Also adding minor changes to the integration tests README to ensure that it's up to date after recent changes that add v2 support (PR #166). wip

mariantalla · 2019-11-12T11:01:52Z

/hold cancel

pkg/idl/doc.go

test/integration/README.md

apelisse · 2019-12-12T01:11:27Z

test/integration/testdata/maptype/atomic-map.go

+package maptype
+
+// +k8s:openapi-gen=true
+type AtomicMap struct {


Are these types supposed to generate something in some golden data? I don't see a corresponding update to some golden data

Hm, good point. Not sure. The steps in the integration tests README don't change the golden data. The only thing that edits golden.v2.json is the builder script 🤔 I'll dig into it a bit as I don't know this codebase that well - let me know if it's something obvious I'm missing!

I'm specifically concerned since the tests don't pass for me but they pass for the CI ...

OK I can reproduce the failure on this pull-request while having master work.

wait no it passes now ...

Ahah it's absolutely terrible. You have to add your objects to:

https://github.com/kubernetes/kube-openapi/blob/master/test/integration/builder/main.go#L105-L117

We should have a way to fail if this is not done somehow ...

Thanks! Added them with a0384dd - though if I'm honest I only partially understand what they're testing 🤔

The test generates the openapi corresponding to the types that you have. Now we can look at the report.v2.json and see that it generates what we expect from the tags.

apelisse · 2019-12-12T01:12:10Z

Looks good to me, a few super small nits. @sttts PTAL

It's atomic/granular, not only atomic.

apelisse · 2019-12-17T17:32:39Z

Looks good to me now!

mariantalla · 2020-01-17T13:59:02Z

Is there anything else to be done here?

/cc @sttts

apelisse

I've left a couple of nits, but this looks really really good, thanks at ton @mariantalla !

apelisse · 2020-01-17T18:32:19Z

test/integration/testdata/golden.v2.json

+      "additionalProperties": {
+       "type": "string"
+      },
+      "x-kubernetes-map-type": "granular"


The doc above says that we don't generate an openapi extension on the granular case?

Which doc are you referring to? 🤔 (happy to change it, just to make sure I understand)

apelisse · 2020-01-17T18:33:00Z

test/integration/testdata/structtype/granular-struct.go

+
+// +k8s:openapi-gen=true
+type GranularStruct struct {
+	// +structType=granular


is this in the right place? the structType is annotating a string. Should it fail some sort of validation?
(and same problem above on the AtomicStruct)

Yeah, the annotation is applied to the string; "correctly" (in that Field gets the right tag in the produced API spec) but it's super confusing the way I've written it. I've changed it so that the marker annotates a "proper" struct - let me know what you think.

Indeed interesting that the validation didn't fail though when using structType on a string 🤔 I'll experiment with it and see if there's a problem here 🤔

Thanks for fixing this.

sttts · 2020-01-20T12:09:29Z

Looks good overall. Leaving lgtm to @apelisse.

apelisse · 2020-01-21T17:06:16Z

/wg apply

apelisse · 2020-01-21T17:07:37Z

pkg/idl/doc.go

-// openapi extension, as this will also be interpreted as the default
-// behavior in the openapi definition.
+// can be updated individually (i.e. the equivalent of `granular`).
+// This default WILL NOT generate any openapi extension, as this will also


Which doc are you referring to?

This doc :-)

We talked about it during the meeting, but making my comment "official". I think it's strictly better to have the "granular" explicitly in the openapi than rely on the missing value.

Done 👌 (for structType too).

mariantalla · 2020-01-23T16:52:24Z

Let me know when all looks good and I'll squash.

apelisse · 2020-01-27T22:00:12Z

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()),


Why are these gone?

huh, no idea. Will re-run the generation and see if I can either reproduce or fix it that way.

I reset the changes in openapi_generated.go as the README in test/integration suggests. However it's still strange that the generation omits some types. Taking a look now to see if anything stands out... 🤔

Fixed (I think!) - problem was that the script that generates the API violations report did not get testdata/custom as input. User error 🙃

apelisse · 2020-01-27T22:00:56Z

I think this is good, I'm just a little concerned about some types disappearing (custom.Bac,...)?

apelisse · 2020-01-30T17:18:51Z

test/integration/README.md

 to print API rule violations to the file, the generator will return error to stderr
 on API rule violations.

 ```bash
 $ go run ../../cmd/openapi-gen/openapi-gen.go \
-  -i "k8s.io/kube-openapi/test/integration/testdata/listtype,k8s.io/kube-openapi/test/integration/testdata/dummytype,k8s.io/kube-openapi/test/integration/testdata/uniontype" \
+  -i "k8s.io/kube-openapi/test/integration/testdata/custom,k8s.io/kube-openapi/test/integration/testdata/listtype,k8s.io/kube-openapi/test/integration/testdata/maptype,k8s.io/kube-openapi/test/integration/testdata/structtype,k8s.io/kube-openapi/test/integration/testdata/dummytype,k8s.io/kube-openapi/test/integration/testdata/uniontype" \


OK So it looks like it shouldn't have worked before? These tests are requiring people to pay too much attention, we should improve that ...

apelisse · 2020-01-30T17:21:37Z

That looks really good, thank you!
/lgtm
/approve

k8s-ci-robot · 2020-01-30T17:22:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse

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~~ [apelisse]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 6, 2019

k8s-ci-robot requested review from roycaihw and yujuhong November 6, 2019 14:15

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2019

Support +mapType, +structType markers

23431b4

and convert both of them to openapi extension: x-kubernetes-map-type. Also adding minor changes to the integration tests README to ensure that it's up to date after recent changes that add v2 support (PR #166). wip

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2019

mariantalla mentioned this pull request Dec 11, 2019

Document how to use list-type and list-keys in a CRD, including in existing clusters. kubernetes-sigs/structured-merge-diff#115

Open

apelisse reviewed Dec 12, 2019

View reviewed changes

pkg/idl/doc.go Show resolved Hide resolved

apelisse reviewed Dec 12, 2019

View reviewed changes

test/integration/README.md Show resolved Hide resolved

apelisse reviewed Dec 12, 2019

View reviewed changes

mariantalla added 2 commits December 16, 2019 14:09

Correct valid values for mapType, structType

7c6c06e

It's atomic/granular, not only atomic.

Add routes for structType and mapType in integration tests

a0384dd

k8s-ci-robot requested a review from sttts January 17, 2020 13:59

apelisse reviewed Jan 17, 2020

View reviewed changes

Apply structType on struct

e01d666

k8s-ci-robot added the wg/apply label Jan 21, 2020

apelisse reviewed Jan 21, 2020

View reviewed changes

Document that a defaulted mapType generates openapi extension

8230c67

apelisse reviewed Jan 27, 2020

View reviewed changes

mariantalla added 2 commits January 29, 2020 16:27

Don't commit openapi_generated.go

d17e71d

Add testdata/custom to the API rule violations check

b567921

apelisse reviewed Jan 30, 2020

View reviewed changes

k8s-ci-robot assigned apelisse Jan 30, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2020

k8s-ci-robot merged commit cdac1c7 into kubernetes:master Jan 30, 2020

Support +mapType, +structType markers #178

Support +mapType, +structType markers #178

Conversation

mariantalla commented Nov 6, 2019 • edited Loading

sttts commented Nov 6, 2019

mariantalla commented Nov 11, 2019

mariantalla commented Nov 11, 2019

mariantalla commented Nov 12, 2019

apelisse Dec 12, 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

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

apelisse commented Dec 12, 2019

apelisse commented Dec 17, 2019

mariantalla commented Jan 17, 2020

apelisse 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

mariantalla Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jan 20, 2020

apelisse commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariantalla commented Jan 23, 2020

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

apelisse commented Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

apelisse commented Jan 30, 2020

k8s-ci-robot commented Jan 30, 2020

mariantalla commented Nov 6, 2019 •

edited

Loading

apelisse Dec 12, 2019 •

edited

Loading

mariantalla Jan 21, 2020 •

edited

Loading

apelisse commented Jan 27, 2020 •

edited

Loading