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

update exit code to 0 if patch not needed #66725

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Jul 27, 2018

Release note:

The `kubectl patch` command no longer exits with exit code 1 when a redundant patch results in a no-op

The specific logic in the patch command that exited with code 1, was only doing so when there was no diff between an existing object and its patched counterpart. (In case of errors, we just return those, which eventually ends up exiting with code 1 anyway). This patch removes this block, as we should not be treating patch no-ops as errors.

Fixes #58212

cc @soltysh

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 27, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 27, 2018
@neolit123
Copy link
Member

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jul 27, 2018
@@ -245,14 +245,8 @@ func (o *PatchOptions) RunPatch() error {
if err != nil {
return err
}
printer.PrintObj(info.Object, o.Out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a tests verifying:

  1. that exit code is 0, when the patch was empty
  2. that exit code is non-zero, when the patch command actually failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soltysh Added a test. Although I realize after adding it, that only semantic or type errors will result in the error being printed out and an exit code of 1. If someone were to, for example, update an immutable field on a pod, or set a value for a non-existent spec field on a pod (like replicas), the server would just return the old object with no error, and the command would exit with the message "not patched" (as before), but with exit code 0 (since the old and new object were the same)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's reasonable.

@soltysh soltysh self-assigned this Jul 30, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 30, 2018
Copy link
Contributor

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

/approve

Please squash your changes into single commit and I push this forward.

@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 31, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/update-patch-return-code-logic branch 2 times, most recently from 91c0f0d to 578ab90 Compare July 31, 2018 17:17
@neolit123
Copy link
Member

/test pull-kubernetes-integration

@juanvallejo juanvallejo force-pushed the jvallejo/update-patch-return-code-logic branch from 578ab90 to ad11a1b Compare July 31, 2018 18:41
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2018
Copy link
Contributor

@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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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-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. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 007bf90 into kubernetes:master Aug 1, 2018
@juanvallejo juanvallejo deleted the jvallejo/update-patch-return-code-logic branch August 1, 2018 14:08
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants