-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve the error message when pushing to a registry that does not support v2-2 #932
Conversation
} catch (IOException ex) { | ||
throw new RegistryErrorExceptionBuilder(getActionDescription(), httpResponseException) | ||
.addReason("Failed to parse registry error response body") | ||
.build(); |
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.
Maybe we should just throw httpResponseException
? The rationale is that, if we didn't have this extra parsing at all like before, it would have thrown httpResponseException
.
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.
Good catch. Will handle this in #937 as this is a lightly modified block taken from elsewhere.
// Did not get an error code back. | ||
throw httpResponseException; | ||
} | ||
ErrorCodes errorCode = ErrorCodes.valueOf(errorCodeString); |
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 noticed valueOf(String)
will throw IllegalArgumentException
if there is no matching enum, so I think we should catch 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.
Good catch. Will handle this in #937 as this is a lightly modified block taken from elsewhere.
if (errorCode.equals(ErrorCodes.MANIFEST_INVALID) | ||
|| errorCode.equals(ErrorCodes.TAG_INVALID)) { | ||
throw new RegistryErrorExceptionBuilder(getActionDescription(), httpResponseException) | ||
.addReason("Registry does not support Image Manifest Version 2, Schema 2") |
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 wonder if it's better to be a little bit less assertive with the claim, like "Registry may not be supporting ...". Just thinking TAG_INVALID
might imply something different in some cases or there were genuinely other reasons (e.g., bugs) that produced an invalid manifest. WDYT?
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.
Yeah, for example something like this would have been misleading for the artifactory errors, where the issue wasn't that they didn't support version 2 schema 2, the issue was that they were expecting fields we didn't provide.
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.
Makes sense. Note that the original Artifactory error report seemed to be due to Artifactory not being configured to support v2-2 out of the box.
It's too bad the servers don't return any actionable information to identify the registry type. Docker Registry has the following headers:
{content-length=[79], content-type=[application/json; charset=utf-8], date=[Wed, 05 Sep 2018 18:12:25 GMT], docker-distribution-api-version=[registry/2.0], connection=[close]}
And quay.io
has:
Headers: {content-length=[131], content-type=[application/json], date=[Wed, 05 Sep 2018 18:13:03 GMT], server=[nginx/1.14.0], connection=[keep-alive]}
Turns
MANIFEST_INVALID
/TAG_INVALID
errors into slightly more helpful error messages:Docker Registry 2.1, which returns a
400
/TAG_INVALID
:Docker Registry 2.2, which returns a
400
/MANIFEST_INVALID
:Pushing to quay.io, which returns a
415
/MANIFEST_INVALID
(doesn't adhere to spec):Fixes #601