Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 247: Add support for $HOME/.docker/config.json #270

Merged
merged 1 commit into from
Jan 21, 2017

Conversation

tourea
Copy link
Contributor

@tourea tourea commented Jan 18, 2017

As described in issue #247, docker-java, up to version 3.0.6, hardcoded the private registry authentication file to use ($HOME/.docker/.dockercfg).
I updated docker-java to support both config.json and .dockercfg (docker-java/docker-java#756), which has been released in version 3.0.7.

This commit upgrades docker-java's version to 3.0.7

As described in issue 247 (
testcontainers#247),
docker-java, up to version 3.0.6, hardcoded the private registry
authentication file to use ($HOME/.docker/.dockercfg).
I updated docker-java to support both config.json and .dockercfg
(docker-java/docker-java#756), which has
been released in version 3.0.7.

This commit upgrades docker-java's version to 3.0.7
@tourea
Copy link
Contributor Author

tourea commented Jan 18, 2017

@rnorth Please review

@bsideup
Copy link
Member

bsideup commented Jan 19, 2017

Hey @tourea,
thanks for your PR. I'll take it :)

Looks like the Travis build is red, I restarted it, maybe some flaky test.

Meanwhile, did you run the tests locally?

@rnorth
Copy link
Member

rnorth commented Jan 19, 2017

😄 it's nice to get this feature with a one-line change! As @bsideup says, it'll be good to get confirmation of no regressions once the tests have run.

Would it be worth adding an automated test for this, in case we somehow break it in the future?

BTW I think I also want to get my head around how this change might affect:

  • the docker daemon discovery code in DockerClientFactory - are there any cases where this can now do the wrong thing?
  • other features that run docker tools inside a container, e.g. docker-compose and upcoming dind. I suspect these won't work as expected out-of-the-box in conjunction with this, so do we need to update these to volume mount the config file or something similar?

@bsideup
Copy link
Member

bsideup commented Jan 19, 2017

I see testExecInContainer test is still failing.

Did they change anything in docker-java related to execCmd?

@rnorth
Copy link
Member

rnorth commented Jan 21, 2017

I've restarted the Travis build, as this seems to be passing locally for me (Docker for Mac 1.13.0)...

@bsideup bsideup added this to the 1.1.8 milestone Jan 21, 2017
@bsideup
Copy link
Member

bsideup commented Jan 21, 2017

LGTM

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.

None yet

3 participants