Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support +mapType, +structType markers #178

Merged
merged 7 commits into from
Jan 30, 2020
Merged

Support +mapType, +structType markers #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
23431b4
Support +mapType, +structType markers
mariantalla Nov 6, 2019
7c6c06e
Correct valid values for mapType, structType
mariantalla Dec 16, 2019
a0384dd
Add routes for structType and mapType in integration tests
mariantalla Dec 17, 2019
e01d666
Apply structType on struct
mariantalla Jan 21, 2020
8230c67
Document that a defaulted mapType generates openapi extension
mariantalla Jan 23, 2020
d17e71d
Don't commit openapi_generated.go
mariantalla Jan 29, 2020
b567921
Add testdata/custom to the API rule violations check
mariantalla Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/generators/extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ var tagToExtension = map[string]extensionAttributes{
kind: types.Slice,
allowedValues: sets.NewString("atomic", "set", "map"),
},
"mapType": {
xName: "x-kubernetes-map-type",
kind: types.Map,
allowedValues: sets.NewString("atomic", "granular"),
},
"structType": {
xName: "x-kubernetes-map-type",
kind: types.Struct,
allowedValues: sets.NewString("atomic", "granular"),
},
}

// Extension encapsulates information necessary to generate an OpenAPI extension.
Expand Down
58 changes: 58 additions & 0 deletions pkg/generators/extension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ func TestSingleTagExtension(t *testing.T) {
extensionName: "x-kubernetes-list-map-keys",
extensionValues: []string{"port"},
},
{
comments: []string{"+mapType=granular"},
extensionTag: "mapType",
extensionName: "x-kubernetes-map-type",
extensionValues: []string{"granular"},
},
{
comments: []string{"+k8s:openapi-gen=x-kubernetes-member-tag:member_test"},
extensionTag: "k8s:openapi-gen",
Expand Down Expand Up @@ -205,6 +211,18 @@ func TestExtensionAllowedValues(t *testing.T) {
},
allowedValues: nil,
},
{
e: extension{
idlTag: "mapType",
},
allowedValues: sets.NewString("atomic", "granular"),
},
{
e: extension{
idlTag: "structType",
},
allowedValues: sets.NewString("atomic", "granular"),
},
{
e: extension{
idlTag: "k8s:openapi-gen",
Expand Down Expand Up @@ -259,6 +277,20 @@ func TestExtensionAllowedValues(t *testing.T) {
values: []string{"atomic"},
},
},
{
e: extension{
idlTag: "mapType",
xName: "x-kubernetes-map-type",
values: []string{"atomic"},
},
},
{
e: extension{
idlTag: "structType",
xName: "x-kubernetes-map-type",
values: []string{"granular"},
},
},
}
for _, test := range successTests {
actualErr := test.e.validateAllowedValues()
Expand Down Expand Up @@ -292,6 +324,20 @@ func TestExtensionAllowedValues(t *testing.T) {
values: []string{"not-allowed"},
},
},
{
e: extension{
idlTag: "mapType",
xName: "x-kubernetes-map-type",
values: []string{"something-pretty-wrong"},
},
},
{
e: extension{
idlTag: "structType",
xName: "x-kubernetes-map-type",
values: []string{"not-quite-right"},
},
},
}
for _, test := range failureTests {
actualErr := test.e.validateAllowedValues()
Expand Down Expand Up @@ -326,6 +372,18 @@ func TestExtensionKind(t *testing.T) {
},
kind: types.Slice,
},
{
e: extension{
idlTag: "mapType",
},
kind: types.Map,
},
{
e: extension{
idlTag: "structType",
},
kind: types.Struct,
},
{
e: extension{
idlTag: "listMapKey",
Expand Down
24 changes: 13 additions & 11 deletions pkg/idl/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,14 @@ type ListType string
type ListMapKey string

// MapType annotates a map to further describe its topology. It may
// have only one value: "atomic". Atomic means that the entire map is
// considered as a whole, rather than as distinct values.
// have one of two values: `atomic` or `granular`. `atomic` means that the entire map is
// considered as a whole; actors that wish to update the map can only
// entirely replace it. `granular` means that specific values in the map can be
// updated separately from other fields.
//
// By default, a map will be considered as a set of distinct values that
// can be updated individually. This default WILL NOT generate any
// 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 still generate an OpenAPI extension with key: "x-kubernetes-map-type".
//
// This tag MUST only be used on maps, or the generation step will fail.
//
Expand Down Expand Up @@ -114,13 +115,14 @@ type PatchMergeKey string
type PatchStrategy string

// StructType annotates a struct to further describe its topology. It may
// have only one value: "atomic". Atomic means that the entire struct is
// considered as a whole, rather than as distinct values.
// have one of two values: `atomic` or `granular`. `atomic` means that the entire struct is
// considered as a whole; actors that wish to update the struct can only
// entirely replace it. `granular` means that specific fields in the struct can be
// updated separately from other fields.
//
// By default, a struct will be considered as a set of distinct values that
// can be updated individually. This default WILL NOT generate any
// openapi extension, as this will also be interpreted as the default
// behavior in the openapi definition.
// can be updated individually (`granular`).
// This default will still generate an OpenAPI extension with key: "x-kubernetes-map-type".
//
// This tag MUST only be used on structs, or the generation step will fail.
//
Expand All @@ -133,7 +135,7 @@ type PatchStrategy string
// used on any struct.
//
// Using this tag will generate the following OpenAPI extension:
// "x-kubernetes-struct-type": "atomic"
// "x-kubernetes-map-type": "atomic"
mariantalla marked this conversation as resolved.
Show resolved Hide resolved
type StructType string

// Union is TBD.
Expand Down
12 changes: 6 additions & 6 deletions test/integration/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,27 @@ $ go test -v .

First, run the generator to create `openapi_generated.go` file which specifies
the `OpenAPIDefinition` for each type, and generate the golden API rule
violation report file . Note that if you do not pass a report
filename (`./testdata/golden.report` in the command below) to let the generator
violation report file. Note that if you do not pass a report
filename (`./testdata/golden.v2.report` in the command below) to let the generator
mariantalla marked this conversation as resolved.
Show resolved Hide resolved
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" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

-o pkg \
-p generated \
-O openapi_generated \
-r ./testdata/golden.report
-r ./testdata/golden.v2.report
```
The generated file `pkg/generated/openapi_generated.go` should have been created.

Next, run the OpenAPI builder to create the Swagger file which includes
the definitions. The output file named `golden.json` will be output in
the definitions. The output file named `golden.v2.json` will be output in
the current directory.

```bash
$ go run builder/main.go testdata/golden.json
$ go run builder/main.go testdata/golden.v2.json
```

After the golden spec is generated, please clean up the generated file
Expand Down
4 changes: 4 additions & 0 deletions test/integration/builder/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ func createWebServices() []*restful.WebService {
w.Route(buildRouteForType(w, "custom", "Bak"))
w.Route(buildRouteForType(w, "custom", "Bac"))
w.Route(buildRouteForType(w, "custom", "Bah"))
w.Route(buildRouteForType(w, "maptype", "GranularMap"))
w.Route(buildRouteForType(w, "maptype", "AtomicMap"))
w.Route(buildRouteForType(w, "structtype", "GranularStruct"))
w.Route(buildRouteForType(w, "structtype", "AtomicStruct"))
return []*restful.WebService{w}
}

Expand Down
2 changes: 2 additions & 0 deletions test/integration/integration_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ const (
testdataDir = "./testdata"
testPkgDir = "k8s.io/kube-openapi/test/integration/testdata"
inputDir = testPkgDir + "/listtype" +
"," + testPkgDir + "/maptype" +
"," + testPkgDir + "/structtype" +
"," + testPkgDir + "/dummytype" +
"," + testPkgDir + "/uniontype" +
"," + testPkgDir + "/custom"
Expand Down
143 changes: 143 additions & 0 deletions test/integration/testdata/golden.v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,82 @@
}
}
},
"/test/maptype/atomicmap": {
"get": {
"schemes": [
"https"
],
"operationId": "func15",
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/maptype.AtomicMap"
}
},
"404": {
"$ref": "#/responses/NotFound"
}
}
}
},
"/test/maptype/granularmap": {
"get": {
"schemes": [
"https"
],
"operationId": "func14",
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/maptype.GranularMap"
}
},
"404": {
"$ref": "#/responses/NotFound"
}
}
}
},
"/test/structtype/atomicstruct": {
"get": {
"schemes": [
"https"
],
"operationId": "func17",
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/structtype.AtomicStruct"
}
},
"404": {
"$ref": "#/responses/NotFound"
}
}
}
},
"/test/structtype/granularstruct": {
"get": {
"schemes": [
"https"
],
"operationId": "func16",
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/structtype.GranularStruct"
}
},
"404": {
"$ref": "#/responses/NotFound"
}
}
}
},
"/test/uniontype/inlinedunion": {
"get": {
"schemes": [
Expand Down Expand Up @@ -403,6 +479,73 @@
}
}
},
"maptype.AtomicMap": {
"type": "object",
"required": [
"KeyValue"
],
"properties": {
"KeyValue": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"x-kubernetes-map-type": "atomic"
}
}
},
"maptype.GranularMap": {
"type": "object",
"required": [
"KeyValue"
],
"properties": {
"KeyValue": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"x-kubernetes-map-type": "granular"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
},
"structtype.AtomicStruct": {
"type": "object",
"required": [
"Field",
"OtherField"
],
"properties": {
"Field": {
"x-kubernetes-map-type": "atomic",
"$ref": "#/definitions/structtype.ContainedStruct"
},
"OtherField": {
"type": "integer",
"format": "int32"
}
}
},
"structtype.ContainedStruct": {
"type": "object"
},
"structtype.GranularStruct": {
"type": "object",
"required": [
"Field",
"OtherField"
],
"properties": {
"Field": {
"x-kubernetes-map-type": "granular",
"$ref": "#/definitions/structtype.ContainedStruct"
},
"OtherField": {
"type": "integer",
"format": "int32"
}
}
},
"uniontype.InlinedUnion": {
"type": "object",
"required": [
Expand Down
6 changes: 6 additions & 0 deletions test/integration/testdata/golden.v2.report
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,10 @@ API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/li
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/listtype,MapList,Field
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/listtype,SetList,Field
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/listtype,UntypedList,Field
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/maptype,AtomicMap,KeyValue
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/maptype,GranularMap,KeyValue
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/structtype,AtomicStruct,Field
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/structtype,AtomicStruct,OtherField
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/structtype,GranularStruct,Field
API rule violation: names_match,k8s.io/kube-openapi/test/integration/testdata/structtype,GranularStruct,OtherField
API rule violation: omitempty_match_case,k8s.io/kube-openapi/test/integration/testdata/listtype,Item,C
7 changes: 7 additions & 0 deletions test/integration/testdata/maptype/atomic-map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package maptype

// +k8s:openapi-gen=true
type AtomicMap struct {
Copy link
Member

@apelisse apelisse Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait no it passes now ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// +mapType=atomic
KeyValue map[string]string
}
7 changes: 7 additions & 0 deletions test/integration/testdata/maptype/granular-map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package maptype

// +k8s:openapi-gen=true
type GranularMap struct {
// +mapType=granular
KeyValue map[string]string
}
Loading