Image pull policy - #1345
Conversation
Co-Authored-By: dmarkhas <minimizer@gmail.com>
Co-Authored-By: dmarkhas <minimizer@gmail.com>
Co-Authored-By: dmarkhas <minimizer@gmail.com>
| @FunctionalInterface | ||
| public interface ImagePullPolicy { | ||
|
|
||
| boolean shouldPull(Image image); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| import org.testcontainers.containers.image.ImageData; | ||
|
|
||
| @Slf4j | ||
| public class DefaultPullPolicy implements ImagePullPolicy { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
@bsideup this class now contains javadocs that describe its function, why do you think it is not resolved?
|
The test failure seems unrelated to any of my changes, is it possible to re-trigger the CI pipeline? |
|
Thanks @bsideup 🐱 |
|
So, um, is anyone going to merge this? 😄 |
| import com.github.dockerjava.api.model.VolumesFrom; | ||
| import com.google.common.base.Strings; | ||
| import lombok.*; | ||
| import java.io.ByteArrayInputStream; |
There was a problem hiding this comment.
please avoid changing the imports order and star-imports
There was a problem hiding this comment.
this is not resolved yet. Also, please do not resolve other's conversations to keep them visible until they are actually resolved.
# Conflicts: # core/src/main/java/org/testcontainers/images/RemoteDockerImage.java
|
Looks like all the checks have passed. Can we take it forward ? |
|
Any plans to get this merged? |
|
Waiting for the merge too. |
|
@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 👍 |
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 😄