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

Add support for --dry-run to kubectl patch. #60675

Merged
merged 1 commit into from Mar 25, 2018

Conversation

Projects
None yet
6 participants
@timoreimann
Contributor

timoreimann commented Mar 2, 2018

What this PR does / why we need it:

Add support for the --dry-run flag to kubectl patch. This is helpful to be able to preview patches prior to applying them.

Which issue(s) this PR fixes:
Refs #11488

Special notes for your reviewer:

This PR carries #45712.

Release note:

`kubectl patch` now supports `--dry-run`.
@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 2, 2018

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli label Mar 2, 2018

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 2, 2018

Pinging some involved parties from the carried PR: @fabianofranz @dims

@jdumars

This comment has been minimized.

Member

jdumars commented Mar 2, 2018

/ok-to-test

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 11, 2018

/test

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 14, 2018

@dshulyak @soltysh quick ping to check if this PR is on your radar. Totally understand if you're very busy right now. Appreciate the heads up.

// TODO: if we ever want to go generic, this allows a clean -o yaml without trying to print columns or anything
// rawExtension := &runtime.Unknown{
// Raw: originalPatchedObjJS,
// }
if err := info.Refresh(targetObj, true); err != nil {
return err
if dataChangedMsg == "patched" {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 16, 2018

Member

nit: rather than comparing strings here, just declare a bool before the DeepEqual in the block above and check that here

This comment has been minimized.

@timoreimann

timoreimann Mar 19, 2018

Contributor

Thanks, good catch!

I moved the didPatch variable definition up one scope to allow reuse. Also created a tiny helper function to centralize the generation of the operation string.

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 19, 2018

/retest

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 20, 2018

Not sure if the CI failures are on me or just flakiness...

/retest

@soltysh

Two nits, and it's good to go.

return err
}
if !reflect.DeepEqual(oldData, newData) {
if !reflect.DeepEqual(info.Object, patchedObj) {

This comment has been minimized.

@soltysh

soltysh Mar 20, 2018

Contributor
didPatch := !reflect.DeepEqual(info.Object, patchedObj)

And make the didPatch variable local to each scope.

This comment has been minimized.

@timoreimann

timoreimann Mar 20, 2018

Contributor

Done.

@@ -239,14 +231,26 @@ func RunPatch(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
return err
}
if !reflect.DeepEqual(info.Object, targetObj) {

This comment has been minimized.

@soltysh

soltysh Mar 20, 2018

Contributor
didPatch := !reflect.DeepEqual(info.Object, patchedObj)

This comment has been minimized.

@timoreimann

timoreimann Mar 20, 2018

Contributor

Done.

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 21, 2018

/retest

1 similar comment
@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 21, 2018

/retest

@soltysh

The code lgtm, but please squash your changes into a single commit.

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 21, 2018

@soltysh

The code lgtm, but please squash your changes into a single commit.

done!

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 22, 2018

/retest
/cross-fingers

@soltysh

/lgtm
/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, timoreimann

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

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 23, 2018

@soltysh is there any further action needed from my end?

The CI indication looks good and your LGTM is there, but I don't see the merge happening. What am I missing?

@juanvallejo

This comment has been minimized.

Member

juanvallejo commented Mar 23, 2018

@timoreimann No further action needed. PR is in merge queue: https://submit-queue.k8s.io/#/queue

@timoreimann

This comment has been minimized.

Contributor

timoreimann commented Mar 23, 2018

Thanks for the info, @juanvallejo. 👍

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 25, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 25, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit fba61b0 into kubernetes:master Mar 25, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation timoreimann authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment