Skip to content

fix(exception-handling): Fix handling of network and other non-status-code errors when polling for datafile #287

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

Conversation

benweissmann
Copy link
Contributor

Summary

  • fix(exception-handling): Catch errors when requesting datafile #285 was an incomplete fix; as per the docs, timeouts and other network errors throw on the call to .get(), not on the call to .raise_for_status() (raise_for_status just checks for a non-2xx HTTP return code; it doesn't handle other kinds of errors)

  • This adds additional error handling around calls to requests.get.

  • Also made a small tweak to the README; zsh requires quoting around .[test].

Test plan

  • Added unit tests for both the polling config manager and the static config manager

@benweissmann benweissmann requested a review from a team as a code owner July 11, 2020 16:46
@benweissmann benweissmann force-pushed the benweissmann/fix-polling-error-handling branch from 076a62e to 9918eba Compare July 11, 2020 16:48
@benweissmann benweissmann force-pushed the benweissmann/fix-polling-error-handling branch from 9918eba to e5eead0 Compare July 11, 2020 16:48
@benweissmann
Copy link
Contributor Author

It looks like CI is failing because of a missing credential -- maybe because I'm an outside contributor? The tests do pass on my machine.

@aliabbasrizvi
Copy link
Contributor

@benweissmann thank you for submitting this change and signing our CLA. I will review this as soon as possible and get a release out.

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Thank you for making the change and being so comprehensive.

Will merge after figuring out CI issues.

@aliabbasrizvi
Copy link
Contributor

Tests succeeded here: https://travis-ci.org/github/optimizely/python-sdk/builds/708071108

Merging change.

@aliabbasrizvi aliabbasrizvi merged commit c07b6bb into optimizely:master Jul 14, 2020
aliabbasrizvi pushed a commit that referenced this pull request Jul 14, 2020
@aliabbasrizvi
Copy link
Contributor

@benweissmann just released updated SDK 3.5.2 with your change.

@benweissmann
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants