-
Notifications
You must be signed in to change notification settings - Fork 21
Remove the need for the requests library entirely #145
Conversation
9539ffd
to
2f741b0
Compare
reactive/docker.py
Outdated
r = requests.get(repository_url) | ||
r.raise_for_status() | ||
with open(cuda_repository_package, 'wb') as f: | ||
for chunk in r.iter_content(chunk_size=1024): | ||
f.write(chunk) | ||
r.close() | ||
with contextlib.closing(urlopen(repository_url)) as r: | ||
with open(cuda_repository_package, "wb") as f: | ||
f.write(r.read()) |
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.
For those looking -- this is the only change to remove the requests package
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.
urlopen(...)
will raise when there are protocol errors -- just like r.raise_for_status()
did
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.
Just a quick suggestion here: I don't know exactly what the size of the package is; however, caution should be taken with r.read()
for large files, as this can consume a significant amount of host memory.
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.
hmm... true it is pulling the whole deb package. Lemme fix that
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
the inclusion of the
requests
library drew inurllib3
whose latests versions caused issues in charms which didn't havehatching
.Update this library to not depend on
requests
at all