-
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
Create helper for parsing an error code from error response #938
Conversation
@@ -53,6 +55,35 @@ public BlobHttpContent getContent() { | |||
return Collections.emptyList(); | |||
} | |||
|
|||
@Override | |||
public Void handleHttpResponseException(HttpResponseException 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.
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.
It includes — it should get tossed once #932 is merged.
/** | ||
* Extract an {@link ErrorCodes} response from an error object. | ||
* | ||
* @param ex the response exception |
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.
This Javadoc error is causing the build failure. There is no ex
.
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.
Argh!
Assert.assertThat( | ||
ex.getMessage(), | ||
CoreMatchers.containsString( | ||
"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.
"Registry may not".
import java.io.IOException; | ||
import java.util.List; | ||
|
||
/** Helper class for parsing {@link ErrorResponseTemplate} from JSON. */ |
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.
This javadoc looks out-of-place?
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.
Originally I was going to have a few different methods, including to parse an ErrorResponseTemplate from a JSON body, etc. But it turned out this single method was enough.
// rethrow the original exception | ||
throw 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.
nit: add empty private constructor to prevent instantiation
public class ErrorResponseUtil { | ||
|
||
/** | ||
* Extract an {@link ErrorCodes} response from an error object. |
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.
from an {@link HttpResponseException}
?
.build(); | ||
} | ||
ErrorCodes errorCode = ErrorResponseUtil.getErrorCode(httpResponseException); | ||
if (errorCode.equals(ErrorCodes.BLOB_UNKNOWN)) { |
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.
SInce it's enum, errorCode == ErrorCodes.BLOB_UNKNOWN
will work, right?
|
||
} catch (IOException ex) { | ||
ErrorCodes errorCode = ErrorResponseUtil.getErrorCode(httpResponseException); | ||
if (errorCode.equals(ErrorCodes.MANIFEST_INVALID) || errorCode.equals(ErrorCodes.TAG_INVALID)) { |
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.
ditto.
import java.io.IOException; | ||
import java.util.List; | ||
|
||
/** Utility methods for parsing {@link ErrorResponseTemplate JSON-encoded error responses}. */ |
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.
Is the JSON-encoded error responses
intended to be inside the {@link
?
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. Callers aren't necessarily aware of the class, but its javadocs have a nice description of the object layout.
Fixed #937, builds on fix in #932.