Add parsing of openapi oneof/union semantic into smd #152

apelisse · 2019-04-02T18:02:09Z

Implements first part of kubernetes/enhancements#926.
We still need to change the OpenAPI generation to add these extensions.

apelisse · 2019-04-02T18:02:28Z

/assign @lavalamp
/cc @kwiesmueller @sttts @jennybuckley

apelisse · 2019-04-02T18:02:57Z

pkg/schemaconv/smd.go

 	return tr
 }

+func parseOneOfDiscriminator(v interface{}) (bool, error) {


these two functions shouldn't be here.

apelisse · 2019-04-02T18:11:24Z

tests are expected to fail, there is a dependency to kubernetes-sigs/structured-merge-diff#72

sttts · 2019-04-03T07:21:26Z

pkg/schemaconv/smd.go

+		Fields: map[string]string{},
+	}
+
+	if idiscriminator, ok := extensions["x-kubernetes-oneof-discriminator"]; ok {


is this a new extension or do we publish it already now?

pkg/schemaconv/testdata/swagger.json

lavalamp · 2019-04-03T16:49:43Z

pkg/schemaconv/smd.go

+	if idiscriminator, ok := extensions["x-kubernetes-oneof-discriminator"]; ok {
+		discriminator, ok := idiscriminator.(string)
+		if !ok {
+			return nil, fmt.Errorf(`"x-kubernetes-oneof-discriminator" is of invalid type: %T`, idiscriminator)


Might be nice to print the value, too. I think %#v includes both.

lavalamp · 2019-04-03T16:50:17Z

pkg/schemaconv/smd.go

+		for ifield, value := range fields {
+			field, ok := ifield.(string)
+			if !ok {
+				return nil, fmt.Errorf(`"x-kubernetes-oneof-fields": field is of invalid type: %T`, ifield)


Definitely need to print the value to make this error actionable.

lavalamp · 2019-04-03T16:51:27Z

pkg/schemaconv/smd.go

+	}
+
+	if union.Discriminator != nil && len(union.Fields) == 0 {
+		return nil, fmt.Errorf("discriminator set to %v, but no fields in union", *union.Discriminator)


Is this actually an error?

How does one indicate whether or not "None" is a permitted value?

Is this actually an error?

What's the point? if the only thing you have a discriminator, then it's just an enum. I would suspect that they happen because of a user-error.

lavalamp · 2019-04-03T16:55:30Z

pkg/schemaconv/testdata/new-schema.yaml

      type:
        namedType: io.k8s.api.core.v1.VsphereVirtualDiskVolumeSource
+    union:
+      fields:


Sorry to leave this comment here and not in an earlier change, but...
would this make more sense as "choices", where it's a list of structs which set "field" and "discriminatedBy" or something? e.g.

union: discriminator: type options: - fieldName: fc discriminatedBy: FC - fieldName: azureDisk discriminatedBy: AzureDisk ...

I think it is more understandable that way, what do you think?

You could even indicate a "none" option that way by permitting a single entry to have no fieldName and just a discriminatedBy.

(no strong opinion about the exact names intended)

Yeah, Jenny made a similar comment on the other PR, I agree. That's also much more kubernetes-style.

apelisse · 2019-04-22T17:32:55Z

Updated this pull-request and rebased it on top of #155 so that it compiles, PTAL @sttts @lavalamp

sttts · 2019-04-24T08:59:14Z

pkg/schemaconv/smd.go

+	if idiscriminator, ok := extensions["x-kubernetes-union-discriminator"]; ok {
+		discriminator, ok := idiscriminator.(string)
+		if !ok {
+			return nil, fmt.Errorf(`"x-kubernetes-union-discriminator" is of invalid type: %#v`, idiscriminator)


nit: write which type you expect

Very good point, I just fixed them all! Thanks, PTAL

jennybuckley · 2019-04-26T22:41:27Z

pkg/schemaconv/smd.go

+
+		}
+		sort.Strings(keys)
+		reverseMap := map[string]string{}


What's the point of keeping the values in here?

jennybuckley · 2019-04-26T22:53:33Z

pkg/schemaconv/smd.go

+			if err != nil {
+				return nil, err
+			}
+			if schemaUnion != nil {


Why would it be nil, should that be an error also?

apelisse · 2019-04-26T23:18:43Z

Fixed, PTAL

jennybuckley · 2019-04-26T23:24:21Z

/lgtm

apelisse · 2019-04-26T23:30:33Z

/approve

k8s-ci-robot · 2019-04-26T23:30:41Z

[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

Details

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

sttts · 2019-04-27T08:51:50Z

pkg/schemaconv/testdata/swagger.json

    "description": "DeploymentStrategy describes how to replace existing pods with new ones.",
+    "x-kubernetes-unions": [{
+     "discriminator": "type",
+     "fields-discriminated": {


does this mean we don't support non-discriminator unions? Would rather call it fields instead of fields-discriminated.

as the type is different, we would in fact need fields []string and discriminatedFields map[string]string.

Yeah I'll probably do that ... if discriminator is set, then fields must be unset and fields-discriminated must be set. and the other way around.

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 Apr 2, 2019

k8s-ci-robot assigned lavalamp Apr 2, 2019

k8s-ci-robot requested review from jennybuckley, kwiesmueller and sttts April 2, 2019 18:02

apelisse commented Apr 2, 2019

View reviewed changes

k8s-ci-robot requested a review from roycaihw April 2, 2019 18:03

apelisse force-pushed the add-union-parsing branch from e630821 to 4f55afd Compare April 2, 2019 18:03

sttts reviewed Apr 3, 2019

View reviewed changes

lavalamp reviewed Apr 3, 2019

View reviewed changes

pkg/schemaconv/testdata/swagger.json Outdated Show resolved Hide resolved

lavalamp reviewed Apr 3, 2019

View reviewed changes

apelisse force-pushed the add-union-parsing branch from 4f55afd to a2590a3 Compare April 22, 2019 17:29

sttts reviewed Apr 24, 2019

View reviewed changes

apelisse force-pushed the add-union-parsing branch 2 times, most recently from 9ae72f6 to f30e972 Compare April 25, 2019 23:05

Update sigs.k8s.io/smd to latest version

de93f9f

apelisse force-pushed the add-union-parsing branch from f30e972 to 0a103ed Compare April 26, 2019 20:46

jennybuckley reviewed Apr 26, 2019

View reviewed changes

Add parsing of openapi oneof/union semantic into smd

62f1ac3

apelisse force-pushed the add-union-parsing branch from 0a103ed to 62f1ac3 Compare April 26, 2019 23:15

k8s-ci-robot assigned jennybuckley Apr 26, 2019

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

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

k8s-ci-robot merged commit 787f6ef into kubernetes:master Apr 26, 2019

sttts reviewed Apr 27, 2019

View reviewed changes

Add parsing of openapi oneof/union semantic into smd #152

Add parsing of openapi oneof/union semantic into smd #152

Uh oh!

Conversation

apelisse commented Apr 2, 2019

Uh oh!

apelisse commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavalamp Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse commented Apr 26, 2019

Uh oh!

jennybuckley commented Apr 26, 2019

Uh oh!

apelisse commented Apr 26, 2019

Uh oh!

k8s-ci-robot commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lavalamp Apr 3, 2019 •

edited

Loading