Add error message for credentials not being sent over HTTP #613
Conversation
| @@ -108,6 +112,11 @@ public String forDockerNotInstalled() { | |||
| return suggest("make sure Docker is installed and you have correct privileges to run it"); | |||
| } | |||
|
|
|||
| public String forInsecureRegistry() { | |||
There was a problem hiding this comment.
This seems like the more useful message, but I don't see a reference to this?
There was a problem hiding this comment.
This one was already here, just moved it because it was all the way at the bottom of the file. Is this actually more useful? I thought the problem happened even if you set allowInsecureRegistries.
There was a problem hiding this comment.
It's called slightly further down in BuildStepRunner
There was a problem hiding this comment.
Yeah, I think that's the one that happens if allowInsecureRegistries isn't set and it tries to do an HTTP fallback. But the error we're throwing here is when HTTP fallback proceeds, but credentials are required (and not sent).
There was a problem hiding this comment.
Ah right, I should read more before commenting haha.
| @@ -96,6 +96,10 @@ public String forCredentialsNotCorrect(String registry) { | |||
| return suggest("make sure your credentials for '" + registry + "' are set up correctly"); | |||
| } | |||
|
|
|||
| public String forCredentialsNotSent() { | |||
| return suggest("use a registry that supports HTTPS so credentials can be sent safely"); | |||
There was a problem hiding this comment.
I think prefixing with an explanation before the suggestion would be good. Something along the lines of:
The registry requires credentials but credentials were not sent because the connection was over HTTP.
There was a problem hiding this comment.
Hmm should that prefix go in the new exception instead of HelpfulSuggestions, or is it too specific for that?
There was a problem hiding this comment.
Yep, good catch, it should go in the new exception instead.
Part of #599