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.

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

Fork github.com/go-openapi/{spec,validate,errors,strfmt} #211

Merged
merged 664 commits into from
Nov 5, 2020

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Oct 28, 2020

Addressing kubernetes/kubernetes#95922

  • 4.6k lines of Go
  • 3.2k lines of tests

versus 40k for the whole go-openapi suite.

Background: go-openapi has very different goal than we do. Every change in behaviour of the validation library has potential influence on our API. go-openapi is supposed to improve over time and support more and more OpenAPI features. We need a stable basis that does not change behaviour at all, or only under tight control with API back and forward compatibility in mind.

casualjim and others added 30 commits September 28, 2017 09:00
marshal unmarshal item of parameter with vendorExtensible
When the input schema is nil, `NewSchemaValidator`
will return a nil `*SchemaValidator`. This nil
`*SchemaValidator` is passed to `Validate`, which
panics when `s.validators` is called.
Don't drop empty arrays of operations security definitions
This fixes a regression introduced by kubernetes#39.
Turns out security: null breaks the validator as it expected either no field at all or an array.

OperatioProperties now have a custom marshaller that ensures that:
1. empty slices are preserved as empty arrays in JSON
2. if Security is unset/nil the key is omitted in JSON
Avoid null value when serializing operations
* 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
* fixing paths for windows

* remove debugging artifacts
Fixes kubernetes#43

Signed-off-by: Igor Zibarev <zibarev.i@gmail.com>
Consistently check if RelativeBase is empty
Fixes kubernetes#45

Signed-off-by: Igor Zibarev <zibarev.i@gmail.com>
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 5, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 5, 2020
Copy link
Contributor

@gautierdelorme gautierdelorme left a comment

Choose a reason for hiding this comment

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

Just a few comments about the new versions.

go.mod Outdated
github.com/go-openapi/spec v0.0.0-20160808142527-6aced65f8501
github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87 // indirect
github.com/go-openapi/jsonpointer v0.19.3
github.com/go-openapi/jsonreference v0.19.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't k/k uses v0.19.3?

go.mod Outdated
github.com/go-openapi/swag v0.0.0-20160704191624-1d0bd113de87 // indirect
github.com/go-openapi/jsonpointer v0.19.3
github.com/go-openapi/jsonreference v0.19.4
github.com/go-openapi/spec v0.19.12
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't k/k uses v0.19.3?

go.mod Outdated
github.com/go-openapi/jsonpointer v0.19.3
github.com/go-openapi/jsonreference v0.19.4
github.com/go-openapi/spec v0.19.12
github.com/go-openapi/swag v0.19.11
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't k/k uses v0.19.5?

go.mod Outdated
github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d
github.com/onsi/ginkgo v0.0.0-20170829012221-11459a886d9c
github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c
github.com/spf13/pflag v0.0.0-20170130214245-9ff6c6923cff
github.com/stretchr/testify v1.3.0
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e // indirect
golang.org/x/tools v0.0.0-20181011042414-1f849cf54d09 // indirect
gopkg.in/yaml.v2 v2.2.2
gopkg.in/yaml.v2 v2.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think k/k uses v2.2.8?

go.mod Outdated
github.com/googleapis/gnostic v0.4.1
github.com/json-iterator/go v1.1.6
github.com/mailru/easyjson v0.0.0-20160728113105-d5b7844b561a // indirect
github.com/kr/text v0.2.0 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

k/k uses v0.1.0 but I don't think it should matter looking at kr/text@v0.1.0...v0.2.0 and creack/pty@v1.1.7...v1.1.9.

We had contributed this upstream, but never used it due to structural schemas.
@sttts sttts force-pushed the sttts-go-openapi branch 2 times, most recently from f032082 to fc7f497 Compare November 5, 2020 10:06
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

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. I understand the commands that are listed here.

Copy link
Contributor

@gautierdelorme gautierdelorme left a comment

Choose a reason for hiding this comment

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

Thanks! go.mod looks good now 👍

@nikhita
Copy link
Member

nikhita commented Nov 5, 2020

/lgtm

I can merge this manually once @dims gives a thumbs up with his sig-arch hat on. :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2020
@dims
Copy link
Member

dims commented Nov 5, 2020

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, gautierdelorme, sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nikhita
Copy link
Member

nikhita commented Nov 5, 2020

Manually merging since we've preserved git history, copyright headers and the licenses. See discussion at https://kubernetes.slack.com/archives/C0EG7JC6T/p1604575860477400?thread_ts=1592287631.273100&cid=C0EG7JC6T

@nikhita nikhita merged commit e58175e into kubernetes:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet