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

Support ImageFromDockerfile authenticated image pulls #2573

Merged
merged 7 commits into from Apr 19, 2020

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Apr 14, 2020

#2201 was intended to support authenticated pulls for images required by ImageFromDockerfile builds and Docker Compose. Unfortunately there was a bug in the ImageFromDockerfile implementation, which needs patching.

Sadly authenticated pulls are not easy to automatically test, so this was missed in manual testing.

This PR:

  • Ensures that the required images are pulled once the list of images is actually populated
  • Automatically disable's docker's built-in pull if this has been done, as it is not required and will fail if an authenticated image is involved
  • Adds integration tests that pull images from a local private docker registry. This covers both building a Docker Image from dockerfile and also launching a compose project. The tests are intended to ensure that the authenticated pull code is actually used; unit tests cover the various permutations of looking up which images need to be pulled.

@bsideup
Copy link
Member

bsideup commented Apr 14, 2020

@rnorth WDYT about writing a test for it? IIRC we already have an infrastructure for authenticated pulls

@rnorth
Copy link
Member Author

rnorth commented Apr 14, 2020

@bsideup yeah, I really wanted to do that, and have done so now building upon our existing local private registry test.

…Dockerfile.java

Co-Authored-By: David Byron <dbyron@dbyron.com>
Comment on lines -112 to +172
.exec(new PushImageResultCallback())
.exec(new ResultCallback.Adapter<>())
Copy link
Member Author

Choose a reason for hiding this comment

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

Deprecation duty.

Comment on lines +135 to +140
@Language("yaml") String composeFileContent =
"version: '2.0'\n" +
"services:\n" +
" privateservice:\n" +
" command: /bin/sh -c 'sleep 60'\n" +
" image: " + testImageNameWithTag;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish we had text blocks!

I would have preferred to just point to a static file on disk, but unfortunately our dockerized private registry has a port number that can vary, and thus the image name changes.

Hence, constructing a tiny YAML file in a string seemed like a tolerable evil.

Copy link
Member

Choose a reason for hiding this comment

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

@rnorth rnorth marked this pull request as ready for review April 14, 2020 21:01
@rnorth rnorth requested a review from kiview as a code owner April 14, 2020 21:01
@rnorth rnorth merged commit f631023 into master Apr 19, 2020
@rnorth rnorth deleted the fix-imagefromdockerfile-authenticated-pull branch April 19, 2020 18:57
rnorth added a commit that referenced this pull request Apr 22, 2020
Co-Authored-By: David Byron <dbyron@dbyron.com>
quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020
@erohana
Copy link

erohana commented May 3, 2021

hi @rnorth
the current solution doesn't support pulling base images with build args

for example
FROM my-company-ecr/openjdk:${jdk_version}
where jdk_version is a build arg, is there a solution for that ?

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

4 participants