Attach user's Docker config folder to compose containers for proper authentication. #537
Conversation
| @@ -429,6 +433,11 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) { | |||
| addEnv("DOCKER_HOST", "unix:///docker.sock"); | |||
| setStartupCheckStrategy(new IndefiniteWaitOneShotStartupCheckStrategy()); | |||
| setWorkingDirectory(containerPwd); | |||
| Path dockerConfig = Paths.get(System.getProperty("user.home"), ".docker"); | |||
There was a problem hiding this comment.
I checked my ~/.docker/ folder, it seems to be also used for things like Docker Machine and even some 3rd parties.
I would mount .docker/config.json instead of .docker/ to avoid mounting something really big :)
There was a problem hiding this comment.
Could you please mention your change in CHANGELOG.md as well? :)
| String dockerConfigProperty = System.getProperty(DOCKER_CONFIG_PROPERTY); | ||
| Path dockerConfig = Paths.get(System.getProperty("user.home"), ".docker", "config.json"); | ||
|
|
||
| if (dockerConfigEnv != null && !dockerConfigEnv.trim().isEmpty() && Files.exists(Paths.get(dockerConfigEnv))) { |
There was a problem hiding this comment.
Could you please encapsulate this boolean expressions into private methods with a speaking name? Like dockerConfigEnvExists, dockerConfigPropertyExists, etc.
There was a problem hiding this comment.
It looks like this is the last thing still to do on this PR - is that correct? I can do this quickly tomorrow if you're stuck for time @yurybubnov. It'd be no problem.
Thanks for this PR!
There was a problem hiding this comment.
I moved it to separate method determineDockerConfigPath
https://github.com/testcontainers/testcontainers-java/pull/537/files#diff-f992b47914d235d916c506932b1946f6R446
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| ### Fixed | |||
| - Fixed retrieval of Docker host IP when running inside Docker. ([\#479](https://github.com/testcontainers/testcontainers-java/issues/479)) | |||
| - Compose is now can pull images from private repositories. | |||
There was a problem hiding this comment.
Compose is now able to pull images from private repositories.
| } else if (dockerConfigProperty != null && !dockerConfigProperty.trim().isEmpty() && Files.exists(Paths.get(dockerConfigProperty))) { | ||
| addFileSystemBind(dockerConfigProperty, DOCKER_CONFIG_FILE, READ_ONLY); | ||
| } else if (Files.exists(dockerConfig)) { | ||
| addFileSystemBind(dockerConfig.toString(), DOCKER_CONFIG_FILE, READ_ONLY); |
There was a problem hiding this comment.
I think the whole block should go into a private method mountDockerConfig().
In addition I think this method could be structured a bit more concise, like maybe
String dockerConfigPath = determineDockerConfigPath();
addFileSystemBind(dockerConfigPath , DOCKER_CONFIG_FILE, READ_ONLY);
|
Made the suggested changes. Thanks for proofreading CHANGELOG. I also added documentation about the changes. |
|
Rebased and merged manually after resolving conflicts |
|
Thank you for the contribution @yurybubnov ! |
bsideup
changed the title
Attach user's Docker config folder to compose containers for proper authentication.
Fixes #536