Adds exception handling to ManifestPusher. #63
Conversation
| */ | ||
| throw new RegistryErrorExceptionBuilder(getActionDescription(), httpResponseException) | ||
| .addReason( | ||
| "repository name not known to registry (make sure that you are using a valid tag - tags cannot contain backslashes)") |
There was a problem hiding this comment.
I wonder if this message can be confusing. Since we're talking about a bad repository name, but then asking them to correct their tag?
There was a problem hiding this comment.
Changed the wording to "...perhaps you are using an invalid tag..." - does that sound better?
| @@ -90,6 +92,9 @@ RegistryErrorExceptionBuilder addReason(String reason) { | |||
| } | |||
|
|
|||
| RegistryErrorException build() { | |||
| // Provides a feedback channel. | |||
| errorMessageBuilder.append( | |||
| " | If this is a bug, please file an issue at https://github.com/google/jib/issues/new"); | |||
There was a problem hiding this comment.
Can you put github.com/google/jib.... in a ProjectInfo file or something static that we reference everywhere?
| */ | ||
| throw new RegistryErrorExceptionBuilder(getActionDescription(), httpResponseException) | ||
| .addReason( | ||
| "repository name not known to registry (perhaps you are using an invalid tag - tags cannot contain backslashes)") |
There was a problem hiding this comment.
Is the repository name available here? If so it could be in the error message and you could tell for sure if it contains slashes.
There was a problem hiding this comment.
I've tried this on GCR and it produces a correct error message already (about using an invalid tag - this error message will show up as part of the wrapped HttpResponseException), but on ECR, it simply gives a 404 and not in the expected Docker registry 404 format.
There was a problem hiding this comment.
If we know their tag here already on our own, from imageTag, can we just tell them that their tag contains backslashes if it does?
There was a problem hiding this comment.
and if it doesn't, then we print the registry from registryEndpointProperties.getImageName() ?
There was a problem hiding this comment.
If their imageTag contains backslashes, we might want to just tell them that before entering the build process. Maybe we should just add a warning in the Maven plugin if the tag configuration contains backslashes?
There was a problem hiding this comment.
If tags with \ are illegal, perhaps manifest pusher might want to throw an illegal arugment exception in the constructor?
There was a problem hiding this comment.
I think it might be good to keep ManifestPusher decoupled from any logic about the formatting of the image tag, since that is specific to the implementation of different Docker registries. Also, an IllegalArgumentException would mean an additional logic in BuildImageMojo to catch that exception and give the user a more meaningful error message. I added some validation in the BuildImageMojo that validates its configuration - currently it just logs some errors but doesn't fail, but perhaps it should fail? I can also remove this custom error message in ManifestPusher since the one in the Mojo is more helpful.
| // 'tag' must not contain backslashes. | ||
| if (tag.indexOf('/') >= 0) { | ||
| getLog().error("'tag' cannot contain backslashes"); | ||
| shouldFail = true; |
There was a problem hiding this comment.
this tag check can probably just go at the end, and we can remove the shouldFail flag.
Fixes #40 .
Specifically, it lets the user know what might have caused a 404 (a bad tag is most likely). Note that there is no validation for the configuration here.