Fix error handler. #631
Fix error handler. #631
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #631 +/- ##
=======================================
Coverage 83.33% 83.33%
=======================================
Files 164 164
Lines 8109 8109
=======================================
Hits 6758 6758
Misses 1351 1351
|
| except api_errors.ApiExecutionError as e: | ||
| LOGGER.error('Failed to execute API %s, v=%s\n%s', | ||
| except api_errors.ApiInitializationError as e: | ||
| LOGGER.error('Failed to initialize API %s, v=%s\n%s', | ||
| api_class_name, api_version, e) | ||
| raise api_errors.ApiInitializationError(e) |
There was a problem hiding this comment.
This can just be the following, since you are re-raising the same exception now:
raise
| @@ -90,8 +90,8 @@ def _get_api(self, api_name): | |||
| else: | |||
| api = api_class(self.global_configs, | |||
| version=api_version) | |||
| except api_errors.ApiExecutionError as e: | |||
| LOGGER.error('Failed to execute API %s, v=%s\n%s', | |||
| except api_errors.ApiInitializationError as e: | |||
There was a problem hiding this comment.
I think you should have an except for both ApiExecutionError and ApiInitializationError. That way you can recover or propagate either type of error.
There was a problem hiding this comment.
I would do it, but at this point, we really are just initializing the api clients and not executing them. Can you please confirm if this is still what would be nice to do?
Changed the raise statement to just a single raise.
There was a problem hiding this comment.
So long as that's the only exception that we care about at this stage and we know we've consistently made sure that's the exception that is raised, then that makes sense. I think longer term there is a lot more work to do around exception handling and ensuring all APIs are consistent. I tried to make things more consistent in my mixins PR, but I know more work needs to be done.
| @@ -90,8 +90,8 @@ def _get_api(self, api_name): | |||
| else: | |||
| api = api_class(self.global_configs, | |||
| version=api_version) | |||
| except api_errors.ApiExecutionError as e: | |||
| LOGGER.error('Failed to execute API %s, v=%s\n%s', | |||
| except api_errors.ApiInitializationError as e: | |||
There was a problem hiding this comment.
I would do it, but at this point, we really are just initializing the api clients and not executing them. Can you please confirm if this is still what would be nice to do?
Changed the raise statement to just a single raise.
…eti-security into fixerrorhandler
This properly handles the error when the admin directory api client can not be created due to missing gsuite key, so that we can get the proper logging message to understand what happened.
@ahoying, AFAICT, this will not affect the rest of the api clients. Can you please confirm?