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 incomplete uint64 support from JSON unmarshaling #62981

Merged
merged 1 commit into from
May 7, 2018
Merged

Remove incomplete uint64 support from JSON unmarshaling #62981

merged 1 commit into from
May 7, 2018

Conversation

ash2k
Copy link
Member

@ash2k ash2k commented Apr 23, 2018

What this PR does / why we need it:
Removes uint64 support from JSON unmarshaling because:

Which issue(s) this PR fixes
Fixes #62769.

Special notes for your reviewer:
This is an alternative approach to #62775.

Release note:

NONE

/kind bug
/sig api-machinery
/cc @sttts

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 23, 2018
@k8s-ci-robot k8s-ci-robot requested a review from sttts April 23, 2018 04:28
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 23, 2018
This was referenced Apr 23, 2018
Closed
Closed
@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

/retest

1 similar comment
@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

/retest

@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

/assign deads2k

@deads2k
Copy link
Contributor

deads2k commented Apr 23, 2018

/assign @liggitt

For json crazy.

liggitt
liggitt reviewed Apr 23, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go Outdated
@@ -120,7 +120,7 @@ func keepOrDeleteNullInObj(m map[string]interface{}, keepNull bool) (map[string]
if err != nil {
return nil, err
}
case []interface{}, string, float64, bool, int, int64, nil:
Copy link
Member

Choose a reason for hiding this comment

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

dropping uint handling makes sense, but I don't see the need to change this here

liggitt
liggitt reviewed Apr 23, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/mergepatch/util.go Outdated
@@ -125,7 +125,7 @@ func HasConflicts(left, right interface{}) (bool, error) {
default:
return true, nil
}
case string, float64, bool, int, int64, nil:
case string, float64, bool, int64, nil:
Copy link
Member

Choose a reason for hiding this comment

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

same here, I would restore this

liggitt
liggitt reviewed Apr 23, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go Outdated
@@ -1876,7 +1876,7 @@ func mergingMapFieldsHaveConflicts(
return true, nil
}
return slicesHaveConflicts(leftType, rightType, schema, fieldPatchStrategy, fieldPatchMergeKey)
case string, float64, bool, int, int64, nil:
case string, float64, bool, int64, nil:
Copy link
Member

Choose a reason for hiding this comment

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

restore this

@liggitt
Copy link
Member

liggitt commented Apr 23, 2018

please limit this change to the removal of uint handling... can put the removal of unsized ints in a separate PR if you want, and we can think about what benefits that provides separately

@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

@liggitt they are not used - as you can see, tests pass without them. That code works with unmarshaled JSON which is int64/float64, never int. So having int there is unnecessary, confusing and is probably an extra branch in code.
I can of course remove that change if you feel strongly... but I think this is just cruft and can be removed.

@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#primitive-types also says

All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int (which is ambiguous depending on target platform). Internal types may use (u)int.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 26, 2018
@ash2k
Copy link
Member Author

ash2k commented Apr 26, 2018

@liggitt I've removed the change. PTAL

@liggitt
Copy link
Member

liggitt commented Apr 26, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2018
@ash2k
Copy link
Member Author

ash2k commented Apr 26, 2018

@deads2k PTAL

@ash2k ash2k mentioned this pull request Apr 29, 2018
Closed
@ash2k
Copy link
Member Author

ash2k commented Apr 30, 2018

/retest

@yliaog
Copy link
Contributor

yliaog commented May 1, 2018

/lgtm

@liggitt
Copy link
Member

liggitt commented May 1, 2018

/assign @lavalamp

sttts
sttts reviewed May 7, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/runtime/converter.go
@@ -438,13 +437,15 @@ func (c *unstructuredConverter) ToUnstructured(obj interface{}) (map[string]inte
}

// DeepCopyJSON deep copies the passed value, assuming it is a valid JSON representation i.e. only contains
// types produced by json.Unmarshal().
// types produced by json.Unmarshal() and also int64.
// bool, int64, float64, string, []interface{}, map[string]interface{}, json.Number and nil
Copy link
Contributor

Choose a reason for hiding this comment

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

is this an enumeration of "types produced by json.Unmarshal() and also int64" ?

nit: put a : behind the previous sentence.

@sttts
Copy link
Contributor

sttts commented May 7, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, liggitt, sttts, yliaog

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 May 7, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 0918155 into kubernetes:master May 7, 2018
@ash2k ash2k deleted the uint64-remove branch May 7, 2018 12:43
@ash2k ash2k mentioned this pull request May 8, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 3, 2018
Merge pull request #63522 from atlassian/remove-json-int
74b7642 
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove int support from json patches

**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**:
```release-note
NONE
```
/sig api-machinery
/kind cleanup
/cc liggitt sttts
k8s-publishing-bot added a commit to kubernetes/apimachinery that referenced this pull request Jul 3, 2018
@k8s-publishing-bot
Merge pull request #63522 from atlassian/remove-json-int
9864a8e 
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove int support from json patches

**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 kubernetes/kubernetes#62981.

**Release note**:
```release-note
NONE
```
/sig api-machinery
/kind cleanup
/cc liggitt sttts

Kubernetes-commit: 74b764224a1dccdeee4b995e9b717ce9168d92e1
@ash2k ash2k mentioned this pull request Jan 31, 2020
Merged
@travisn travisn mentioned this pull request Oct 18, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@sttts sttts sttts left review comments

Assignees

@lavalamp lavalamp

@liggitt liggitt

@deads2k deads2k

@yliaog yliaog

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. 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.

DeepCopyJSON causes a panic
8 participants
@ash2k @deads2k @liggitt @yliaog @sttts @k8s-ci-robot @k8s-github-robot @lavalamp