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.

Sign up for GitHub

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

Jump to bottom

Remove int support from json patches #63522

Merged
merged 1 commit into from
Jul 3, 2018
Merged

Remove int support from json patches #63522

merged 1 commit into from
Jul 3, 2018
+32 −34

Conversation

ash2k
Copy link
Member

@ash2k ash2k commented May 8, 2018

What this PR does / why we need it:
JSON only contains int64 and float64 types for numbers so int is not needed.

Special notes for your reviewer:
This is a follow up for #62981.

Release note:

NONE

/sig api-machinery
/kind cleanup
/cc liggitt sttts

@k8s-ci-robot k8s-ci-robot requested a review from liggitt May 8, 2018 00:43
@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 8, 2018
@k8s-ci-robot k8s-ci-robot requested a review from sttts May 8, 2018 00:43
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 8, 2018
@ash2k
Copy link
Member Author

ash2k commented May 8, 2018

/retest

@ash2k ash2k mentioned this pull request May 8, 2018
Closed
@ash2k
Copy link
Member Author

ash2k commented May 8, 2018

/retest

@lavalamp
Copy link
Member

/assign @mbohlool

JSON doesn't reliably transport numbers bigger than 54 bits...

@ash2k ash2k force-pushed the remove-json-int branch from ed5c287 to 433b448 Compare May 11, 2018 09:16
@ash2k
Copy link
Member Author

ash2k commented May 11, 2018

/retest

@lavalamp
Copy link
Member

Why would we remove int? int64 is the thing that's not reliably transmitted by JSON?

@mbohlool
Copy link
Contributor

mbohlool commented May 11, 2018 •
edited
Loading

JSON only contains int64 and float64 types for numbers

JSON number does not have type actually. It is safer to use int32 vs int64 as most parsers support up to 53 bits. Anyhow I don't see any reasoning in the referenced issue why we should remove int.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2018
@ash2k
Copy link
Member Author

ash2k commented May 11, 2018

Originally JSON parser produces only float64 for numbers when marshalled into interface{}. JSON parser we use only ever produces either int64 or float64 for numbers. int is just not needed.

@ash2k
Copy link
Member Author

ash2k commented May 17, 2018

@mbohlool there is a misunderstanding. I'm talking about parsed JSON (map[string]interface{} etc), not JSON itself. I know that JSON does not have integers, only floats.

@ash2k
Copy link
Member Author

ash2k commented May 17, 2018

Anyhow I don't see any reasoning in the referenced issue why we should remove int.

What is the reasoning to keep it? Why have int but not, say, int32 and/or int16?

@mbohlool
Copy link
Contributor

/unhold

@ash2k
Copy link
Member Author

ash2k commented May 18, 2018

@mbohlool /hold cancel please :)
@liggitt PTAL

@mbohlool
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2018
@ash2k
Copy link
Member Author

ash2k commented Jun 8, 2018

/retest

@sttts
Copy link
Contributor

sttts commented Jun 11, 2018

Sgtm. Can you blame who owns the json patch code?

@sttts
Copy link
Contributor

sttts commented Jun 11, 2018

JSON number does not have type actually. It is safer to use int32 vs int64 as most parsers support up to 53 bits. Anyhow I don't see any reasoning in the referenced issue why we should remove int.

@mbohlool in all our JSON data structure code we have an invariant on the valid types (which do not include int). We have code paths which even panic (intentionally) if they happen to hit an unknown type. So this PR is correct and desired from this point of view.

@ash2k
Copy link
Member Author

ash2k commented Jun 20, 2018

@sttts
Code in k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go was introduced in #40666

k8s.io/apimachinery/pkg/util/mergepatch/util.go was refactored in #40942, originally introduced in #16980

k8s.io/apimachinery/pkg/util/strategicpatch/patch.go was also introduced in #16980

FYI @jackg @mengqiy

@sttts
Copy link
Contributor

sttts commented Jun 21, 2018

Lgtm. Giving @jackg @mengqiy some time to take a look.

@sttts
Copy link
Contributor

sttts commented Jul 2, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, 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 Jul 2, 2018
@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65094, 65533, 63522, 65694, 65702). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 74b7642 into kubernetes:master Jul 3, 2018
@ash2k ash2k deleted the remove-json-int branch July 3, 2018 01:24
@ash2k ash2k mentioned this pull request Jan 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt Awaiting requested review from liggitt

@sttts sttts Awaiting requested review from sttts

Assignees

@sttts sttts

@mbohlool mbohlool

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ash2k @lavalamp @mbohlool @sttts @k8s-ci-robot @k8s-github-robot