- Use OSError with EPERM as our error condition when being run as root. SystemExit is really designed for stuff that calls sys.exit() and other low-level stuff. OSError is a far better description of what's happening here.
- Remove weird _handle_system_exit() abstraction. I'm not even sure why we need this since none of our code outside of here calls sys.exit() or raises this exception, but at least it's slighly less confusing now
mmodell • thcipriani
- Group Reviewers
- rMSCAadbeba3d447f: Tidy up some low-level exception handling
- Patch without arc
- git checkout -b D795 && curl -L https://phabricator.wikimedia.org/D795?download=true | git apply
To be perfectly honest, I'm not sure why we want to re-raise in this condition at all... there's no reason we can't handle it gracefully below and tbh nobody should be calling SystemExit anyway.
Build has FAILED
Link to build: https://integration.wikimedia.org/ci/job/differential-docker-test/251/
See console output for more information: https://integration.wikimedia.org/ci/job/differential-docker-test/251/console
Rebase should fix this now. Something weird happening with flake8 dependencies. It's fixed in master though.
Change looks fine to me, I think that function was built out in anticipation of future use that never materialized.
longer rambling thoughts since you asked :)
seems more correct :)
Couple of thoughts on this:
These are mostly speculation. Have to ask @bd808 for the straight dope.
Now I'm not entirely sure it's desirable to raise here, really since it skips logging.shutdown (or maybe we should move logging.shutdown into _before_exit). It might be better to restore _handle_system_exit in case we do ever decide to use sys.exit anywhere:
def _handle_system_exit(ex): return ex.message
But really I have no strong opinion on this. This seems fine.
From a commit made Mar 2, 2014... yeah. I think the original was a very generic fatal error cascade handler probably copied from some bot wrapper I had written at some other point in my life. If you look at rMSCAbd5f470beaad: Add base class for creating CLI wrappers you will see that things were pretty basic.
Build has FAILED
Link to build: https://integration.wikimedia.org/ci/job/differential-docker-test/256/
See console output for more information: https://integration.wikimedia.org/ci/job/differential-docker-test/256/console