Relax email requirement in auth config#1207
Relax email requirement in auth config#1207KostyaSha merged 1 commit intodocker-java:masterfrom tobiasstadler:master
Conversation
| AuthConfig authConfig = null; | ||
| if (getRegistryUsername() != null && getRegistryPassword() != null && getRegistryEmail() != null | ||
| && getRegistryUrl() != null) { | ||
| if (getRegistryUsername() != null && getRegistryPassword() != null && getRegistryUrl() != null) { |
There was a problem hiding this comment.
maybe better fill all non-null values into authconfig without combining them?
There was a problem hiding this comment.
something like
AuthConfig authConfig = new AuthConfig;
if (getRegistryUsername() != null) {
authConfig.setRegistryUsername(getRegistryUsername());
}
if(getRegistryPassword() != nul) {
authConfig.setRegistryPassword(getRegistryPassword());
}
...
There was a problem hiding this comment.
Objects.nonNull() and even in one liner
There was a problem hiding this comment.
Ok, but in effectiveAuthConfig() the return value of getAuthConfig() is checked for null. This will no longer work with your approach
There was a problem hiding this comment.
Then it will be empty, but not null...
There was a problem hiding this comment.
yes and because it is not null the empty config will be used for authetification instead of getting the auth config from the docker config file. So in effectiveAuthConfig() we need to check if the auth config returned by getAuthConfig() is empty. Which I think is ugly.
Maybe we should change the && in ||:
if (getRegistryUsername() != null || getRegistryPassword() != null || getRegistryEmail() != null || getRegistryUrl() != null)
@KostyaSha What di you think?
There was a problem hiding this comment.
well, if email optional, then ok as is
|
Thank You |
The email field is deprected in the api and was removed from the cli in v17.06
This change is