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

Extract util used by jsonmergepatch and SMPatch #40942

Merged
merged 1 commit into from
Feb 4, 2017
Merged

Extract util used by jsonmergepatch and SMPatch #40942

merged 1 commit into from
Feb 4, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Feb 3, 2017

followup #40666 (comment)

Extract some util out of the strategicMergePatch to make jsonMergePatch doesn't depend on strategicMergePatch.

None

cc: @liggitt

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 3, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@mengqiy
Copy link
Member Author

mengqiy commented Feb 3, 2017

@k8s-bot bazel test this

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 3, 2017
liggitt
liggitt reviewed Feb 3, 2017
View reviewed changes
pkg/kubectl/cmd/BUILD Outdated
@@ -114,6 +114,7 @@ go_library(
"//vendor:k8s.io/apimachinery/pkg/util/intstr",
"//vendor:k8s.io/apimachinery/pkg/util/json",
"//vendor:k8s.io/apimachinery/pkg/util/jsonmergepatch",
"//vendor:k8s.io/apimachinery/pkg/util/mergepatchutil",
Copy link
Member

Choose a reason for hiding this comment

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

suggest pkg/util/patch... util is already in the name

Copy link
Member Author

Choose a reason for hiding this comment

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

Change to pkg/util/mergepatch, since patch has been used as a variable everywhere.

liggitt
liggitt reviewed Feb 3, 2017
View reviewed changes
pkg/kubectl/cmd/edit.go Outdated
@@ -46,6 +46,7 @@ import (

"github.com/golang/glog"
"github.com/spf13/cobra"
"k8s.io/apimachinery/pkg/util/mergepatchutil"
Copy link
Member

Choose a reason for hiding this comment

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

group with other apimachinery imports

@liggitt
Copy link
Member

liggitt commented Feb 3, 2017

nits on package name and imports, LGTM otherwise

@liggitt
Copy link
Member

liggitt commented Feb 3, 2017

also, bazel was unhappy with cmd/util/jsonmerge/BUILD

@mengqiy mengqiy force-pushed the extract_util_from_SMPatch branch 2 times, most recently from aeadb84 to e923610 Compare February 3, 2017 21:37
@mengqiy
Copy link
Member Author

mengqiy commented Feb 3, 2017

Tests are passing. PTAL

lavalamp
lavalamp reviewed Feb 3, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go Outdated
"fmt"
)

var ErrBadJSONDoc = fmt.Errorf("Invalid JSON document")
Copy link
Member

Choose a reason for hiding this comment

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

Use errors.New, not fmt.Errorf for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change that. Why we prefer errors.New than fmt.Errorf?

Copy link
Member

Choose a reason for hiding this comment

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

I don't trust the output of fmt.Errorf to always compare correctly (but maybe I'm just paranoid). When I see fmt.Errorf, I think of a class of errors (and one needs to make a separate type to check membership in this class), whereas errors.New is clearly a single error.

lavalamp
lavalamp reviewed Feb 3, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go Outdated
return ok
}

// PreconditionFunc asserts that an incompatible change is not present within a patch.
Copy link
Member

Choose a reason for hiding this comment

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

Everything from here down doesn't seem to belong in an "errors.go" file?

@mengqiy
Copy link
Member Author

mengqiy commented Feb 4, 2017

@lavalamp @liggitt PTAL. Thanks.

@lavalamp
Copy link
Member

lavalamp commented Feb 4, 2017

/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 Feb 4, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: lavalamp, ymqytw

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 40289, 40877, 40879, 39972, 40942)

@k8s-github-robot k8s-github-robot merged commit 2541c16 into kubernetes:master Feb 4, 2017
@mengqiy mengqiy deleted the extract_util_from_SMPatch branch February 4, 2017 21:01
@ash2k ash2k mentioned this pull request Jun 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@liggitt liggitt liggitt left review comments

Assignees

@lavalamp lavalamp

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. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mengqiy @k8s-reviewable @liggitt @lavalamp @k8s-github-robot @k8s-ci-robot