-
Notifications
You must be signed in to change notification settings - Fork 37
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
blobs: switch from hyper to reqwest #65
Conversation
c92cdef
to
41695ee
Compare
TL;DR: It does now, thanks for pointing this out 😄 I thought it already did because I was able to trigger an error when I cross tested the changes from cincinnati, but I must've made a mistake while testing it |
41695ee
to
836a934
Compare
faa3def
to
ba70f72
Compare
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 just have a nit/question on embedding whole body into error message, otherwise seems fine.
This change is motivated by improved statuscode handling. Instead of reimplementing this here we can make use of the reqwest library which already does it.
d58f92e
to
f432cf9
Compare
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
…ient_error The introduced code returns an error if the status code in the success range. Before this change even error codes would be treated as success and the HTTP body, likely an error description from the server, would be misused as an image blob. In accordance with [RFC2068][1] the body should be displayed to the user on client errors. For usability we include a UTF8 decoded version since it's likely to yield something human readable, e.g.: ```rust ErrorMessage { msg: "could not download blob: GET request failed with status \'404 Not Found\' and body of size 233: \"<!DOCTYPE HTML PUBLIC \\\"-//W3C//DTD HTML 3.2 Final//EN\\\">\\n<title>404 Not Found</title>\\n<h1>Not Found</h1>\\n<p>The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.</p>\\n\"" }' ``` [1]: https://www.freesoft.org/CIE/RFC/2068/89.htm
f432cf9
to
dc3734f
Compare
This change is motivated by improved status error handling.
Before this change even error codes would be treated as success and the
body would contain the HTML formatted error message passed through to
the caller.
Instead of reimplementing proper error handling here we can make use of
the reqwest library which already does it.
Closes #63
Progress on #44