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

Replace and remove `raise Exception` calls #91

Merged
merged 5 commits into from Apr 5, 2019

Conversation

Projects
None yet
2 participants
@lukehinds
Copy link
Contributor

lukehinds commented Mar 21, 2019

Please see GH issue #87 for details

Fixes #87

Replace and remote `raise Exception` calls
Please see GH issue #87 for details

Fixes #87

@lukehinds lukehinds requested a review from nabilschear Mar 21, 2019

@lukehinds lukehinds changed the title Replace and remote `raise Exception` calls Replace and remove `raise Exception` calls Mar 21, 2019

@nabilschear

This comment has been minimized.

Copy link
Contributor

nabilschear commented Mar 21, 2019

so the exceptions were nice because they would stop the flow of execution and head out to an error handler. If we just let it keep going, then more errors (and less easy to understand) are going to happen. What if instead, we drop the duplicate logger.error lines put raise Exception back in and instead just remove this line of code or change it to logger.debug? https://github.com/keylime/python-keylime/blob/ba0a323ceaffab9c82d5a94ccbfb698d49e136e6/keylime/tenant.py#L768 i think that will clean it up a bit. in that spirit, we should also change this https://github.com/keylime/python-keylime/blob/ba0a323ceaffab9c82d5a94ccbfb698d49e136e6/keylime/tenant.py#L765-L766 and this https://github.com/keylime/python-keylime/blob/ba0a323ceaffab9c82d5a94ccbfb698d49e136e6/keylime/tenant.py#L702-L703 to use exceptions rather than directly calling sys.exit. Not sure if this should also be converted to a raise Exception? https://github.com/keylime/python-keylime/blob/ba0a323ceaffab9c82d5a94ccbfb698d49e136e6/keylime/tenant.py#L721

also the tenant webapp deals with things this way: https://github.com/keylime/python-keylime/blob/ba0a323ceaffab9c82d5a94ccbfb698d49e136e6/keylime/tenant_webapp.py#L527-L531 i'm not sure if that will print a stack trace or not.

@nabilschear

This comment has been minimized.

Copy link
Contributor

nabilschear commented Mar 21, 2019

ha it already is logger.debug. maybe we just need to change the default loglevel in keylime.conf to not be debug =)

@lukehinds

This comment has been minimized.

Copy link
Contributor Author

lukehinds commented Mar 21, 2019

ha it already is logger.debug. maybe we just need to change the default loglevel in keylime.conf to not be debug =)

That's not a bad idea. We could also set a more human friendly format.

[formatter_formatter]
format = %(asctime)s.%(msecs)03d - %(name)s - %(levelname)s - %(message)s
datefmt = %Y-%m-%d %H:%M:%S

My main motivation was to not make correctly captured events look like a bug (when it spews out a trackback to the various code objects). Report everything gracefully.

@nabilschear

This comment has been minimized.

Copy link
Contributor

nabilschear commented Apr 3, 2019

@lukehinds i went ahead and tried out the special exception solution, updated the log formatting/level, and cleaned up other instances of stack traces being printed. what do you think?

@lukehinds

This comment has been minimized.

Copy link
Contributor Author

lukehinds commented Apr 3, 2019

@lukehinds i went ahead and tried out the special exception solution, updated the log formatting/level, and cleaned up other instances of stack traces being printed. what do you think?

Thanks, will take a look (not out machine just now).

@lukehinds lukehinds merged commit b01b16c into keylime:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lukehinds lukehinds deleted the lukehinds:raise_except branch Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.