-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Image pull policy (#1345) #2024
Conversation
* get exitcode from inspectExecCmd * Update core/src/main/java/org/testcontainers/containers/Container.java Co-Authored-By: dmarkhas <minimizer@gmail.com> * Update core/src/main/java/org/testcontainers/containers/Container.java Co-Authored-By: dmarkhas <minimizer@gmail.com> * Update core/src/main/java/org/testcontainers/containers/Container.java Co-Authored-By: dmarkhas <minimizer@gmail.com> * Use Lombok's `@Value` in `ExecResult` * Update Container.java * Add image pull policy * dont expose docker-java Image directly * fixed pullingNonExistentImageFailsGracefully test * fixed failing tests * use OffsetDateTime instead of System.currentMillis * use java.time.Instant * make Wait and PullPolicy non-instantiable * code review changes * fixed failing tests * use hamcreset anyOf * fix ImagePullPolicyTest * make ImageData an interface * revert ContainerCreationTest.java * use java.time.Instant * revert an unrelated change in ContainerLogsTest * use java.time.Instant * use redis instead of alpine in example * use OptionalLong instead of Long in ImageData * resolve conflict with master * resolve conflict with master * resolve conflict with master * resolve conflict with master
Hey @dmarkhas, Could you please take a look and tell us if this implementation will cover your use cases? Thanks! |
Logger logger = DockerLoggerFactory.getLogger(imageName.toString()); | ||
|
||
// Does our cache already know the image? | ||
ImageData imageData = LOCAL_IMAGES_CACHE.get(imageName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm half tempted to suggest having an additional boolean that can be used in line 27, e.g. something like:
if (isCached && !shouldPullCached(...
Alternatively, perhaps renaming this to cachedImageData
would be equivalently clear.
return false; | ||
} | ||
|
||
log.trace("Should pull image: {}", imageName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling that we should always log, maybe at debug level, about the pull decision (including if we've decided not to pull!)
Also covers case described in #2048 |
See #1345 by @dmarkhas
Fixes #1324