Skip to content

apiextensions: prune {any,one}Of + Not recursively on OpenAPI v2 conversion#71137

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sttts:sttts-crd-openapi-spec-recursive-v2-prune
Nov 16, 2018
Merged

apiextensions: prune {any,one}Of + Not recursively on OpenAPI v2 conversion#71137
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sttts:sttts-crd-openapi-spec-recursive-v2-prune

Conversation

@sttts
Copy link
Copy Markdown
Contributor

@sttts sttts commented Nov 16, 2018

Fixes #71132.

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 16, 2018

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 16, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 16, 2018

[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

Details Needs approval from an approver in each of these files:

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 k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2018
@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Nov 16, 2018

/assign @roycaihw

@sttts sttts added the kind/bug Categorizes issue or PR as related to a bug. label Nov 16, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Nov 16, 2018
@sttts sttts added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 16, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 16, 2018
@sttts sttts added this to the v1.13 milestone Nov 16, 2018
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 16, 2018
@nikopen
Copy link
Copy Markdown
Contributor

nikopen commented Nov 16, 2018

/retest

@sttts sttts removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 16, 2018
@sttts sttts force-pushed the sttts-crd-openapi-spec-recursive-v2-prune branch from c41f44c to 017d2bd Compare November 16, 2018 17:39
@sttts sttts force-pushed the sttts-crd-openapi-spec-recursive-v2-prune branch from 017d2bd to 0d9a022 Compare November 16, 2018 17:42
roycaihw
roycaihw reviewed Nov 16, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

/lgtm

The new unit test capturing the bug has passed.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go
for k, v := range in.Enum {
out.Enum[k] = v
if in.Enum != nil {
out.Enum = make([]interface{}, len(in.Enum))
Copy link
Copy Markdown
Member

@roycaihw roycaihw Nov 16, 2018

Choose a reason for hiding this comment

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

Nice catch. This makes input and output more consistent.

Copy link
Copy Markdown
Contributor Author

@sttts sttts Nov 16, 2018

Choose a reason for hiding this comment

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

And makes tests green :-)

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go
}

// ConvertJSONSchemaProps converts the schema from apiextensions.JSONSchemaPropos to go-openapi/spec.Schema
// ConvertJSONSchemaProps converts the schema from apiextensions.JSONSchemaPropos to go-openapi/spec.Schema.
Copy link
Copy Markdown
Member

@roycaihw roycaihw Nov 16, 2018

Choose a reason for hiding this comment

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

These functions probably fit better in a util/helper package

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
roycaihw
roycaihw reviewed Nov 16, 2018
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go
type PostProcessFunc func(*spec.Schema) error

// ConvertJSONSchemaPropsWithPostProcess converts the schema from apiextensions.JSONSchemaPropos to go-openapi/spec.Schema
// and run a post process step on each JSONSchemaProps node.
Copy link
Copy Markdown
Member

@roycaihw roycaihw Nov 16, 2018

Choose a reason for hiding this comment

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

The post process option looks like a good hot-fix to me. It's a little bit too flexible/powerful IMO.

In future maybe we want a walker processing on a single type like #71132 suggested (I checked kube-openapi and it's not straight-forward to use the refWalker there). So we don't have to mix conversion and object-processing up

Copy link
Copy Markdown
Contributor Author

@sttts sttts Nov 16, 2018

Choose a reason for hiding this comment

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

Yes, a walker would be great. This is kind of a walker, but does the conversion in parallel.

@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Nov 16, 2018

/retest

@roycaihw
Copy link
Copy Markdown
Member

roycaihw commented Nov 16, 2018

/retest

[sig-api-machinery] CustomResourceDefinition resources Simple CustomResourceDefinition has OpenAPI spec served with CRD Validation chema

failed 2 out of 30 times today due to timeout. I think we can increase the timeout to 3 minutes when updating the e2e test

@AishSundar
Copy link
Copy Markdown
Contributor

AishSundar commented Nov 16, 2018

/priority critical-urgent
/remove-priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 16, 2018
@k8s-ci-robot k8s-ci-robot merged commit 3ea3cfc into kubernetes:master Nov 16, 2018
liggitt added a commit to liggitt/kubernetes that referenced this pull request Nov 16, 2018
@liggitt
Revert "Merge pull request kubernetes#71137 from sttts/sttts-crd-open…
ad2b916 
…api-spec-recursive-v2-prune"

This reverts commit 3ea3cfc, reversing
changes made to fab7009.
@roycaihw roycaihw mentioned this pull request Nov 19, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k Awaiting requested review from deads2k

@mbohlool mbohlool Awaiting requested review from mbohlool

1 more reviewer

@roycaihw roycaihw roycaihw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@roycaihw roycaihw

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v1.13

Development

Successfully merging this pull request may close these issues.

5 participants

@sttts @k8s-ci-robot @nikopen @roycaihw @AishSundar