Skip to content

Fix exception handling.#659

Merged
twiest merged 1 commit intoopenshift:masterfrom
jarovo:exit_staus
Oct 14, 2015
Merged

Fix exception handling.#659
twiest merged 1 commit intoopenshift:masterfrom
jarovo:exit_staus

Conversation

@jarovo
Copy link
Contributor

@jarovo jarovo commented Oct 6, 2015

The subcommand of the action was called using os.system. The exit value
of os.system is a 16-bit value. This value was propagated and used as
exit value of the whole cluster {ACTION} command without any
modification, resulting in exit() being called with value > 255. In
the CPython 2.7 exit(v) with v > 255 behaves like exit(0), which hides
that we had an error during the execution.

This commit removes the error propagation by return value and introduces
using exceptions instead.

@openshift-bot
Copy link

Can one of the admins verify this patch?

The subcommand of the action was called using os.system. The exit value
of os.system is a 16-bit value. This value was propagated and used as
exit value of the whole `cluster {ACTION}` command without any
modification, resulting in `exit()` being called with value > 255. In
the CPython 2.7 exit(v) with v > 255 behaves like exit(0), which hides
that we had an error during the execution.

This commit removes the error propagation by return value and introduces
using exceptions instead.
@jarovo
Copy link
Contributor Author

jarovo commented Oct 12, 2015

Bump!

@abutcher
Copy link
Member

👍 from me - @sdodson @detiber ?

@sdodson
Copy link
Member

sdodson commented Oct 12, 2015

LGTM

@sdodson
Copy link
Member

sdodson commented Oct 12, 2015

ok to test
no idea if the bot listens to me or not

@jarovo
Copy link
Contributor Author

jarovo commented Oct 13, 2015

To reproduce the error:

Python 2.7.10 (default, Sep 24 2015, 17:50:09) 
[GCC 5.1.1 20150618 (Red Hat 5.1.1-4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> r=os.system('exit 30')
>>> print r
7680
>>> exit(r)

$ echo $?
0

@detiber
Copy link
Contributor

detiber commented Oct 13, 2015

@twiest @wshearn

@twiest
Copy link
Contributor

twiest commented Oct 14, 2015

ok to test

@twiest
Copy link
Contributor

twiest commented Oct 14, 2015

👍

@JaryN thanks for the commit! :)

twiest added a commit that referenced this pull request Oct 14, 2015
@twiest twiest merged commit f1b8264 into openshift:master Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants