Skip to content

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.

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
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.

5 participants