Conversation
connelldave
left a comment
connelldave
left a comment
There was a problem hiding this comment.
Looks like a much cleaner change and implementation than previous discussions in most part, but I'm unsure what value it really adds: does it fail fast enough? Is it clear that this (if I understand right) will throw and all other exceptions will be supressed on raise_exceptions=False but also be an implied default with raise_exceptions=True?
My concern is that this is overly niche at the cost of less clear API behaviour of the library
| logger.exception(cove_session.format_cove_error(e)) | ||
| raise InvalidRegion(account_session_info["Region"]) from e | ||
|
|
||
| if self.raise_exception is True: |
There was a problem hiding this comment.
Needs to be refactored to support this flag: I assume we want to just propogate the error to the Cove output unless this flag is also set?
There was a problem hiding this comment.
suggest just flipping to
| if self.raise_exception is True: | |
| if self.raise_exception is False: | |
| continue |
or something other kind of short circuit
There was a problem hiding this comment.
or perhaps this is incorrect and contradicts the point of the change?
There was a problem hiding this comment.
with this change the raise_exception flag is a bit of a misnomer. I think of it as "raise exception caused by some error in the client's function". I don't want to change the behavior for any of these exceptions.
The new condition being checked is to "raise exception caused by a misconfigured boto3 session".
Because of how boto3 works, the region isn't actually validated until the first API call is made. If cove gets bad input for the region, then the client's function is doomed before it even executes because it has been given a misconfigured session.
We couldn't find a sane way to validate the region inputs before this point, so bailing out here is the next best place.
does it fail fast enough?
The reason for this PR is to fail faster than botocove currently does with an invalid region name. Currently what happens is that botocove will try every account once for the bad region, and pause for several seconds for each one. When I first saw that happening, I had assumed there was some network error that was causing botocove to slow to a crawl. In any case, it was taking so long to process the accounts that I just had to cancel it anyway and then try to figure out what was wrong (and I didn't always realise that it was because I had mispelled a region name).
| FailedAssumeRole: List[Dict[str, Any]] | ||
|
|
||
|
|
||
| class BotocoveError(Exception): |
There was a problem hiding this comment.
Unused - no-op unless it defines some behaviour different to it's parent class
There was a problem hiding this comment.
Fair enough. I was following the advice in the Python Exceptions documentation:
When creating a module that can raise several distinct errors, a common practice is to create a base class for exceptions defined by that module, and subclass that to create specific exception classes for different error conditions.
The only real custom error is InvalidRegion, so I suppose the base class is just cruft.
| logger.exception(cove_session.format_cove_error(e)) | ||
| raise InvalidRegion(account_session_info["Region"]) from e |
There was a problem hiding this comment.
Premature optimisation/gold plating here? We can just logger.exception what's happened and re-raise botocore's EndpointConnectionError
There was a problem hiding this comment.
It may not be immediately obvious that an EndpointConnectionError is caused by an invalid region. You can read the region name from the error message, but it's a bit harder to parse. If you think we don't need the custom exception I can remove it.
|
|
||
| except Exception as e: | ||
|
|
||
| if isinstance(e, EndpointConnectionError): |
There was a problem hiding this comment.
this is a strange enough thing to look for that it seems worth commenting exactly why we're checking
There was a problem hiding this comment.
I thought I had commented it somewhere, but I seem to have lost that text before pushing. I agree. I'll add something.
Yes, I think that is enough to reject this implementation. The fact that it raises so many doubts would be bad for long-term maintenance. What exactly would I will close this for now unless you think there is any reason to keep it open. I have a simpler suggestion in #76 . |
2 participants
Resolves #55 by assuming any region is valid until evidence to the contrary. If a request raises an EndpointConnectionError and the region isn't in Boto3's model, then in all liklihood, the region doesn't exist.