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
Consistently check if RelativeBase is empty
@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.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

sttts added 2 commits November 5, 2020 10:59
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.