Image pull policy #1345

dmarkhas · 2019-03-27T10:23:24Z

This is an attempt to address #1324 and provide a means of specifying the way the library should handle pulling images with static tags.
I expect there will be some comments 😄

Co-Authored-By: dmarkhas <minimizer@gmail.com>

…ners-java

bsideup · 2019-03-27T10:41:23Z

core/src/main/java/org/testcontainers/containers/image/pull/policy/ImagePullPolicy.java

+@FunctionalInterface
+public interface ImagePullPolicy {
+
+    boolean shouldPull(Image image);


I feel uneasy about using docker-java's classes in our public APIs where otherwise possible.
Could you please add a simple interface for the Image? ☺️

Also, I'm not sure we should use Image here.
As a pull policy developer, I do not expect an instance of image available before I decide how to pull it. Maybe the name isn't correct and it should be something like ImageRefreshPolicy?

Sure, I can avoid exposing Image.
As for the name - I "borrowed" the Kubernetes term but I have no strong feelings either way. I think it makes sense to assume the policy dictates the behavior when an image exists locally, otherwise the library will always pull it from a remote registry, but I'm OK with changing it.

One thing I changed and would like your feedback on is the fact that the ctor of GenericContainer was triggering the docker pull flow immediately (by resolving the Future) - this seemed strange to me, because it means this flow would run even if the container is never used at all. Also this would've prevented me from adding the imagePullPolicy because by that time the image would have already been pulled (or not).
So I moved this step to configure() instead - is that OK?

@rnorth WDYT?

I'm totally OK with moving the image pull behaviour out of the constructor. This is really quite a longstanding itch; the intention was to enable 'fail-fast' if a bad image was specified, but it's generally caused a fair amount of confusion.

👍 for changing this.

core/src/main/java/org/testcontainers/containers/image/pull/policy/AgeBasedPullPolicy.java

core/src/main/java/org/testcontainers/containers/image/pull/policy/PullPolicy.java

rnorth · 2019-03-28T17:32:22Z

core/src/main/java/org/testcontainers/containers/image/pull/policy/DefaultPullPolicy.java

+import org.testcontainers.containers.image.ImageData;
+
+@Slf4j
+public class DefaultPullPolicy implements ImagePullPolicy {


I can imagine that this will be the place people go to look for javadocs on the default policy, so please could there be class javadocs describing it here? Maybe lift what's on PullPolicy to the classes, and reduce the method-level javadocs there to links.

I have unresolved this because I don't think it is actually resolved.

@dmarkhas please keep the conversations open, the reporters will resolve them when they feel so :)

@bsideup this class now contains javadocs that describe its function, why do you think it is not resolved?

dmarkhas · 2019-03-28T19:52:39Z

The test failure seems unrelated to any of my changes, is it possible to re-trigger the CI pipeline?

dmarkhas · 2019-03-29T08:04:53Z

Thanks @bsideup 🐱
On 2nd thought I prefer the current terminology of imagePullPolicy over imageRefreshPolicy, since there is no concept of "image refresh" in the Docker nomencalture, whereas it is clear what "pull policy" implies and what behavior it would affect.

dmarkhas · 2019-04-03T11:11:47Z

So, um, is anyone going to merge this? 😄

bsideup · 2019-04-03T12:45:41Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

 import com.google.common.base.Strings;
-import lombok.*;
+import java.io.ByteArrayInputStream;


please avoid changing the imports order and star-imports

this is not resolved yet. Also, please do not resolve other's conversations to keep them visible until they are actually resolved.

core/src/main/java/org/testcontainers/containers/GenericContainer.java

…olicy

…ners-java

# Conflicts: # core/src/main/java/org/testcontainers/images/RemoteDockerImage.java

tyagiakhilesh · 2019-08-23T08:05:08Z

Looks like all the checks have passed. Can we take it forward ?

mangan77 · 2019-09-12T06:49:58Z

Any plans to get this merged?

tschmidt01 · 2019-09-16T09:10:49Z

Waiting for the merge too.

bsideup · 2019-11-01T21:33:17Z

@dmarkhas I just merged your PR into a branch in the main repo and will make some adjustments, so that we can merge it to master soon 👍
Thank you!

@dmarkhas

See #1345 by @dmarkhas Fixes #1324

Dan Markhasin and others added 9 commits March 18, 2019 10:58

get exitcode from inspectExecCmd

880788e

Update core/src/main/java/org/testcontainers/containers/Container.java

0080d29

Co-Authored-By: dmarkhas <minimizer@gmail.com>

Update core/src/main/java/org/testcontainers/containers/Container.java

2239277

Co-Authored-By: dmarkhas <minimizer@gmail.com>

Update core/src/main/java/org/testcontainers/containers/Container.java

f78d328

Co-Authored-By: dmarkhas <minimizer@gmail.com>

Use Lombok's @Value in ExecResult

f177274

Update Container.java

de7ae4f

Merge branch 'master' of https://github.com/testcontainers/testcontai…

b024581

…ners-java

Merge remote-tracking branch 'upstream/master'

4509ac1

Add image pull policy

ec8bb5d

dmarkhas requested review from bsideup, kiview and rnorth as code owners March 27, 2019 10:23

bsideup reviewed Mar 27, 2019

View reviewed changes

Dan Markhasin added 4 commits March 27, 2019 16:39

dont expose docker-java Image directly

fe4b907

fixed pullingNonExistentImageFailsGracefully test

ca4dc61

fixed failing tests

b6c22b0

use OffsetDateTime instead of System.currentMillis

ec8442a

dbyron0 reviewed Mar 27, 2019

View reviewed changes

core/src/main/java/org/testcontainers/containers/image/pull/policy/AgeBasedPullPolicy.java Outdated Show resolved Hide resolved

use java.time.Instant

b671134

rnorth reviewed Mar 28, 2019

View reviewed changes

core/src/main/java/org/testcontainers/containers/image/pull/policy/PullPolicy.java Outdated Show resolved Hide resolved

rnorth reviewed Mar 28, 2019

View reviewed changes

core/src/main/java/org/testcontainers/containers/image/pull/policy/PullPolicy.java Show resolved Hide resolved

rnorth reviewed Mar 28, 2019

View reviewed changes

Dan Markhasin and others added 2 commits March 28, 2019 20:43

make Wait and PullPolicy non-instantiable

494ac19

Merge branch 'master' into imagePullPolicy

b2dcf05

bsideup reviewed Apr 3, 2019

View reviewed changes

core/src/main/java/org/testcontainers/containers/GenericContainer.java Outdated Show resolved Hide resolved

bsideup reviewed Apr 3, 2019

View reviewed changes

core/src/main/java/org/testcontainers/containers/GenericContainer.java Outdated Show resolved Hide resolved

Dan Markhasin added 8 commits August 21, 2019 12:52

use OptionalLong instead of Long in ImageData

b4174ad

Merge remote-tracking branch 'origin/imagePullPolicy' into imagePullP…

5ce1eb3

…olicy

Merge branch 'master' of https://github.com/testcontainers/testcontai…

24d162e

…ners-java

Merge branch 'master' into imagePullPolicy

c7b397a

# Conflicts: # core/src/main/java/org/testcontainers/images/RemoteDockerImage.java

resolve conflict with master

0a2964b

resolve conflict with master

4efe3e1

resolve conflict with master

3768c1c

resolve conflict with master

4f0620b

bsideup mentioned this pull request Sep 14, 2019

Some errors cause a NoClassDefFoundError and the real error is hidden #1872

Open

Merge branch 'master' into imagePullPolicy

463fab5

rnorth mentioned this pull request Sep 24, 2019

Expose OracleContainer image name property as public constant #1904

Closed

rnorth self-assigned this Oct 3, 2019

rnorth added this to the next milestone Oct 3, 2019

Merge branch 'master' into imagePullPolicy

f0e9be1

rnorth mentioned this pull request Oct 5, 2019

Images used in modules should be be pulled by digest and not by tag #1951

Open

Merge branch 'master' into imagePullPolicy

192e476

rnorth mentioned this pull request Oct 12, 2019

Always pulling images even when they're locally available #1941

Closed

bsideup modified the milestones: 1.12.3, next Oct 26, 2019

bsideup assigned bsideup and unassigned rnorth Nov 1, 2019

bsideup changed the base branch from master to image_pull_policy November 1, 2019 21:29

Merge branch 'image_pull_policy' into imagePullPolicy

a5fea32

bsideup merged commit 6b19f04 into testcontainers:image_pull_policy Nov 1, 2019

bsideup mentioned this pull request Nov 1, 2019

Image pull policy (#1345) #2024

Merged

bsideup added a commit that referenced this pull request Nov 27, 2019

Image pull policy (#2024)

17d86c9

See #1345 by @dmarkhas Fixes #1324

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image pull policy #1345

Image pull policy #1345

dmarkhas commented Mar 27, 2019

bsideup Mar 27, 2019

dmarkhas Mar 27, 2019

bsideup Mar 27, 2019

rnorth Mar 28, 2019

rnorth Mar 28, 2019

bsideup Apr 3, 2019

dmarkhas Apr 11, 2019

dmarkhas commented Mar 28, 2019

dmarkhas commented Mar 29, 2019

dmarkhas commented Apr 3, 2019

bsideup Apr 3, 2019

bsideup Apr 10, 2019

tyagiakhilesh commented Aug 23, 2019

mangan77 commented Sep 12, 2019

tschmidt01 commented Sep 16, 2019

bsideup commented Nov 1, 2019

Image pull policy #1345

Image pull policy #1345

Conversation

dmarkhas commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarkhas commented Mar 28, 2019

dmarkhas commented Mar 29, 2019

dmarkhas commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyagiakhilesh commented Aug 23, 2019

mangan77 commented Sep 12, 2019

tschmidt01 commented Sep 16, 2019

bsideup commented Nov 1, 2019