Client-side OpenAPI schema fixes #38
Conversation
k8s-ci-robot
added
cncf-cla: yes
pkg/util/proto/document.go
Outdated
| @@ -149,11 +154,13 @@ func (d *Definitions) parsePrimitive(s *openapi_v2.Schema, path *Path) (Schema, | |||
| } | |||
| switch t { | |||
| case String: | |||
| fallthrough | |||
There was a problem hiding this comment.
This is equivalent to what it was doing before. I don't know which version I dislike the most :-)
There was a problem hiding this comment.
Yeah, I've now removed fallthrough and instead added a comment (// do nothing). Give how go doesn't fallthrough by default, a comment will help most of us that are used to how switch statements behave in other languages.
pkg/util/proto/document.go
Outdated
| objectTypes := s.GetType().GetValue() | ||
| if len(objectTypes) == 1 { | ||
| if len(objectTypes) == 0 { |
There was a problem hiding this comment.
We could avoid the repetition here I think, by extracting the objectTypes[0] into a variable, that we would default to object?
There was a problem hiding this comment.
I've replaced the if/else if with a switch. Not sure what you meant about extracting it into a variable - it wouldn't change anything as it's only used once.
pkg/util/proto/document.go
Outdated
| return d.parsePrimitive(s, path) | ||
| } else { | ||
| // the OpenAPI generator never generates (nor it ever did in the past) OpenAPI type definitions with multiple types | ||
| return nil, newSchemaError(path, "definitions with multiple types aren't supported") |
There was a problem hiding this comment.
Maybe this should be at the beginning
There was a problem hiding this comment.
This is now the default case in a switch statement.
When an object contains a property of type RawExtension, that property's definition in the OpenAPI schema only contains "type:object" and no "additionalProperties". According to the OpenAPI specification, the object's properties can be of any type (primitives, arrays or objects).
luksa
force-pushed
the
openapi_schema_fixes_clientside
branch
from
a3dee8c
to
81dd575
Compare
| @@ -132,6 +133,63 @@ var _ = Describe("Reading apps/v1beta1/Deployment from openAPIData", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| var _ = Describe("Reading apps/v1beta1/Deployment from v1.11 openAPIData", func() { | |||
There was a problem hiding this comment.
Having test for both versions is great!
|
@mbohlool Could you PTAL? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbohlool 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 |
k8s-ci-robot
added
the
approved
| @@ -27,8 +27,9 @@ import ( | |||
| ) | |||
|
|
|||
| var fakeSchema = testing.Fake{Path: filepath.Join("testing", "swagger.json")} | |||
| var fakeSchemaNext = testing.Fake{Path: filepath.Join("testing", "swagger_next.json")} | |||
There was a problem hiding this comment.
I'm also writing a change to that code, and I don't want to create a new swagger file for every new piece of feature. Can we just have a new type that uses your feature?
|
/lgtm That's a net improvement, we might want to simplify the test with the two swagger files. |
k8s-ci-robot
added
the
lgtm
* remove unnecessary debug logs * allow trailing slash * use strings.TrimRight * intermediate commit * a commit that passes the validation tests * remove refmodifier * remove comments for easier review * fixed tests * remove unnecessary debug messages * remove some previously commented code
These changes allow the client to properly handle the server-side changes planned to be introduced in kube v1.11:
"type": "object"without either theadditionalPropertiesor thepropertiesfield."type": "object"property (next to thepropertiesfield, which they already contain).