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

exit with correct exit code on plugin failure #54094

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Oct 17, 2017

Release note:

NONE

Correctly exits after a plugin command execution failure with the correct exit code.
Related downstream bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1501756

Before

$ kubectl plugin will-fail-plugin
ls: cannot access 'does-not-exist': No such file or directory
error: exit status 2

$ echo $?
1

After

$ kubectl plugin will-fail-plugin
ls: cannot access 'does-not-exist': No such file or directory
error: exit status 2

$ echo $?
2

cc @fabianofranz

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 17, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @juanvallejo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 17, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/exit-plugin-cmd-w-correct-exit-code branch from b4cf35c to 092b9fc Compare October 17, 2017 21:32
@fabianofranz
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2017
@fabianofranz
Copy link
Contributor

/assign

if exiterr, ok := err.(*exec.ExitError); ok {
// check for (and exit with) the correct exit code
// from a failed plugin command execution
if status, ok := exiterr.Sys().(syscall.WaitStatus); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, need to check if this works on Mac and Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will go ahead and test this on both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to work as expected on Windows:
windows_exit_code

Will test on mac tomorrow and post screenshot for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabianofranz works as expected on mac:
screen shot 2017-10-20 at 5 05 13 pm

@sg00dwin thanks for helping me test on mac

@juanvallejo
Copy link
Contributor Author

/test pull-kubernetes-unit

@juanvallejo juanvallejo force-pushed the jvallejo/exit-plugin-cmd-w-correct-exit-code branch from 092b9fc to 96d3f45 Compare October 18, 2017 23:01
@fabianofranz
Copy link
Contributor

/lgtm
/approve https://bugzilla.redhat.com/show_bug.cgi?id=1501756
/sig cli

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Oct 24, 2017
@k8s-github-robot
Copy link

/test all

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

@fabianofranz
Copy link
Contributor

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue requirement bypassed by: fabianofranz

The full list of commands accepted by this bot can be found here.

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 Oct 24, 2017
@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 8f79857 into kubernetes:master Oct 24, 2017
@juanvallejo juanvallejo deleted the jvallejo/exit-plugin-cmd-w-correct-exit-code branch October 25, 2017 14:07
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-none Denotes a PR that doesn't merit a release note. 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

6 participants