-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add odp rest api manager #398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! I like that you reused the mock response, that's a good idea. Just a couple changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of small changes suggested.
@jaeopt @andrewleap-optimizely I hope exception for invalid URL is handled ok. Request.Exception's class InvalidURL is not comprehensive at all. For example it doesn't catch https://api.zaius..com or XXhttps://api.zaius.com. For cases like these two I had to add other exceptions. If better idea how to guard against an invalid url lmk I'm not super clear how swift does it, it seems it checks for exactly that url string? |
We can accomplish something similar using |
@Mat001 I understand that RequestException will catch these all. |
@jaeopt @andrewleap-optimizely ok. I refactored. I got rid of the specific URL parsing exceptions and made it such that generic RequestEception catches invalid URL errors (may not include urlparse errors), but it guard against quite a few. And since we control the URL I wouldn't worry about this one, if that's ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a clarification
self.logger.error(Errors.ODP_EVENT_FAILED.format('invalid URL')) | ||
can_retry = False | ||
# retry on network errors | ||
should_retry = True | ||
except RequestException as err: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but can you confirm that this catches all other exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaeopt yes, confirm. We use the same exception handling in event_dispatcher and fetch_datafile:
https://github.com/optimizely/python-sdk/blob/master/optimizely/event_dispatcher.py#L56
https://github.com/optimizely/python-sdk/blob/master/optimizely/config_manager.py#L367
It'll catch all these: https://requests.readthedocs.io/en/latest/_modules/requests/exceptions/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
Test plan
Issues