CHE-1312: Add ability to configure credentials for several docker private registries #1544
Conversation
|
@garagatyi review please |
|
Build # 971 - FAILED Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/971/ to view the results. |
AndrienkoAleksandr
force-pushed
the
CHE-1312
branch
from
0501792 to
c49c671
Compare
|
Build # 976 - FAILED Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/976/ to view the results. |
|
LGTM |
| ### Docker registry auth config example | ||
| #docker.registry.auth.your_registry_name.url=https://index.docker.io/v1/ | ||
| #docker.registry.auth.your_registry_name.username=user-name | ||
| #docker.registry.auth.your_registry_name.password=user-password |
There was a problem hiding this comment.
shall we remove this part if it's not used
There was a problem hiding this comment.
No, it is needed for the sake of documentation I think
There was a problem hiding this comment.
It will be good to add comment which says that user can configure many registries with different names, because it is not obviously now.
|
ok |
|
is it a WIP PR or not anymore ? |
|
not anymore |
|
so PR should be renamed ? |
| conn.disconnect(); | ||
| void checkRegistryIsAvailable() { | ||
| if (TRUE.equals(snapshotUseRegistry) && !isNullOrEmpty(registryUrl)) { | ||
| String registryLink = "http://" + registryUrl; |
There was a problem hiding this comment.
it's strange to have http:// being concatenated with a variable named "URL" because URL is expected to already provide the protocol
There was a problem hiding this comment.
yes, it was the meaning of my comment, that injected string shouldn't be named with "url" inside as it's not an url
|
LGTM |
AndrienkoAleksandr
changed the title
|
Pull request updated. @garagatyi @evoevodin review please. |
|
LGTM |
Add possibility configure credentials for several docker private registries in the che.properties.