Change errors structure to handle them gracefully#390
Change errors structure to handle them gracefully#390runcom merged 1 commit intocontainers:masterfrom boaz0:errors_cause_for_download
Conversation
|
/cc @rhatdan @TomSweeneyRedHat WDYT? |
docker/docker_client.go
Outdated
| logrus.Debugf("Ping %s status %d", url, resp.StatusCode) | ||
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized { | ||
| return errors.Errorf("error pinging repository, response code %d", resp.StatusCode) | ||
| return errors.Wrapf(errors.New(http.StatusText(resp.StatusCode)), "Error pingin repository") |
There was a problem hiding this comment.
nit "pingin" to "pinging"
|
LGTM |
|
The change LGTM with the exception of the nit, but I think I'd rather not remove anything from the original message. I personally like the status codes, but see the value in the text. I think I'd prefer from your example: Error initiating layer upload to /path/to/upload, status 401: Unauthorized. Also you didn't hit up all the occurrences in this project, should we? @nalind WDYT? |
|
With the errors wrap won't we end up with the 401 anyways if you print out the error? |
|
LGTM with the pingin fix. |
The Here are the texts for each status-code from Maybe I should change it to @TomSweeneyRedHat thank you for noticing! Working on it. @runcom thanks for the review. |
|
I like this. |
|
@mtrmac PTAL and get this merged. |
This still relies on a string comparison, which is fragile (it is non-obvious that the string is an API commitment), and invisible to tools. I’d much prefer using a specific In fact, focusing on the 401 status which seems to be the core concern of containers/buildah#254 , there already are two plausible options:
In the worst case, a new |
|
@ripcurld0 this needs to be rebased. |
|
@mtrmac Thanks a lot for the review. By the way, we're not vendoring this package, and we're relying on other people to do that. Should I leave it as it is or we need to vendor this package? @TomSweeneyRedHat I have rebased the patch. Thank you so much for noticing. |
|
ping @mtrmac 🖌 ♻️ |
|
👍 Thanks! Please rebase once more. @runcom you might want to take another look. |
If commiting or reading Docker image fails the error message is verbose
and thus it's hard to handle the error gracefully.
This patch attempts to solve in the following way:
The `docker/distribution/registry/client.HandleErrorResponse` takes a
`http.Response` and returns a new error containing a human-readable
error message alongside with the status code.
In order to handle such errors, verify the error type and handle it:
if respErr, ok := errors.Cause(err).(errcode.Error); ok {
// handle the error here.
}
This change is going to apply on docker images.
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
|
LGTM |
|
Thank you so much. |
FWIW this needs to stay as it is because c/image is a library package; types in vendored packages are not visible to users. See e.g. opencontainers/image-spec#528 (comment) , and this would equally apply to the error types returned by |
|
@ripcurld0 Could you vendor this into buildah? |
|
@rhatdan consider this done 😉 |
6 participants
Using
errors.Causeone can understand what was the cause of the failure. Unfortunately, when the error is returned due to status code different than 203, 200 or 201errors.Causeis a verbose string.This patch suggests to set the cause of the error to be the staus text.
For example, if failed to upload a layer, instead of having the following:
Error initiating layer upload to /path/to/upload, status 401This error message will be returned:
Error initiating layer upload to /path/to/upload": UnauthorizedIt's then easier to handle this error by reading
errors.Cause:This can help me to fix containers/buildah#254.