fetch-configlet: Honor rate limits and errors.#2186
Conversation
Closes #2182. Also removed dependency on requests module.
yawpitch
left a comment
There was a problem hiding this comment.
Some questions about the backoff handling and a few minor notes. Losing the requests dependency is nice, but then you'll also want to remove it from the .travis.yml file.
|
I'll go over your notes and edit the travis file. I originally left it alone in case |
Changed tarfile call Used tarfile as context manager Add global constants
Added loop for download_and_extract Changed unused variables per style
yawpitch
left a comment
There was a problem hiding this comment.
Looks good, and as I started hitting some errors with the previous version yesterday I'm thinking this will be a much cleaner approach. Thanks for the work!
yawpitch
left a comment
There was a problem hiding this comment.
An HTTPError is currently breaking the script at line 43.
cmccandless
left a comment
There was a problem hiding this comment.
LGTM (pending comments from other reviewers not yet resolved).
The original fetch-configlet bash script had significantly less console output... I'm seeing all these print statements and thinking it's probably appropriate to use the logging module, but I can address that in a separate PR after this is merged.
|
Hopefully that will take care of it. Sorry about the urlopen hiccup, I forgot I wrapped it in the other project I had used it in to return the modified HTTPError instead of raising it. |
yawpitch
left a comment
There was a problem hiding this comment.
Ok, looks good... may benefit from some refactoring and adding a bit more logging, but lets get it deployed and see how it works with our other builds.
Closes #2182. Also removed dependency on requests module.