Skip to content

Relax email requirement in auth config#1207

Merged
KostyaSha merged 1 commit intodocker-java:masterfrom
tobiasstadler:master
Jul 8, 2019
Merged

Relax email requirement in auth config#1207
KostyaSha merged 1 commit intodocker-java:masterfrom
tobiasstadler:master

Conversation

@tobiasstadler
Copy link
Copy Markdown
Contributor

@tobiasstadler tobiasstadler commented Jul 1, 2019

The email field is deprected in the api and was removed from the cli in v17.06


This change is Reviewable

@tobiasstadler tobiasstadler changed the title Relay email requirement in auth config Relax email requirement in auth config Jul 1, 2019
AuthConfig authConfig = null;
if (getRegistryUsername() != null && getRegistryPassword() != null && getRegistryEmail() != null
&& getRegistryUrl() != null) {
if (getRegistryUsername() != null && getRegistryPassword() != null && getRegistryUrl() != null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better fill all non-null values into authconfig without combining them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

AuthConfig authConfig = new AuthConfig;
if (getRegistryUsername() != null) {
    authConfig.setRegistryUsername(getRegistryUsername());
}
if(getRegistryPassword() != nul) {
    authConfig.setRegistryPassword(getRegistryPassword());
}
...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects.nonNull() and even in one liner

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but in effectiveAuthConfig() the return value of getAuthConfig() is checked for null. This will no longer work with your approach

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it will be empty, but not null...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, if email optional, then ok as is

@KostyaSha KostyaSha merged commit f6a2d49 into docker-java:master Jul 8, 2019
@tobiasstadler
Copy link
Copy Markdown
Contributor Author

Thank You

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants