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

handler: convert int types correctly like yaml2 does and gnostic expects #149

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Feb 25, 2019

Json always unmarshals to float64, yaml2 does best effort down-sizing of the returned numbers, returning int, int64, uint64 or float64. Gnostic expects the later.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2019
@sttts
Copy link
Contributor Author

sttts commented Feb 25, 2019

/assign @liggitt @mbohlool

@sttts sttts force-pushed the sttts-handler-yaml-int-types branch from cc73358 to 1d6fc57 Compare February 25, 2019 09:50
Copy link

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2019
@apelisse
Copy link
Member

I'm curious why this is not using sigs.k8s.io/yaml1 that has a JSonToYaml method built-in, which, according to a comment2 already does that properly.

@sttts
Copy link
Contributor Author

sttts commented Feb 25, 2019

I'm curious why this is not using sigs.k8s.io/yaml1 that has a JSonToYaml method built-in, which, according to a comment2 already does that properly.

Because it is slow. The one here avoids going through []byte. This was one of bottlenecks in OpenAPI merges. Hence, we optimized it.

Though I wouldn't mind moving this one over to the other repo. Wdyt?

@apelisse
Copy link
Member

is it limited to an interface problem or an implementation? I'm asking because I wonder if we can improve the situation in general as opposed to just in this repo.

@sttts
Copy link
Contributor Author

sttts commented Feb 28, 2019

@apelisse I have created kubernetes-sigs/yaml#14. Can we please merge this first to fix this critical bug? I will create a follow-up with revendoering sig.k8s.io/yaml as soon as the upper PR merged.

@sttts
Copy link
Contributor Author

sttts commented Feb 28, 2019

@mbohlool is this approved?

@sttts
Copy link
Contributor Author

sttts commented Feb 28, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[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

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit b3a7cee into kubernetes:master Feb 28, 2019
@apelisse
Copy link
Member

Can we please merge this first to fix this critical bug?

Too late, but yes :-)

@apelisse
Copy link
Member

And really appreciate that you opened that PR!

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: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants