[Feature] Accept image as future instead of a value #106
Conversation
| @@ -63,7 +65,7 @@ | |||
| private List<String> portBindings = new ArrayList<>(); | |||
|
|
|||
| @NonNull | |||
| private String dockerImageName = "alpine:3.2"; | |||
| private Future<String> image; | |||
There was a problem hiding this comment.
The previous field was private, so it's safe to delete it. However, getters/setters are there and not deleted (to keep backward compatibility)
image name was selected because it might me both image name ( "owner/repo:tag" ), and image id ( "bde98019a9e2" )
bsideup
changed the title
|
Dockerfile support would be great, so thank you for taking this on. I'd love to see anything you can show for the developer facing API as this takes shape! Would you suggest that this PR be merged along with the Dockerfile support a little later, or do you foresee standalone use cases for this PR? Thank you - and I really like how you presented this PR by the way :) |
|
well... the reason why I decided to send this changes already was the amount of changes and probability of conflicts. In fact, it's more or less completed API of deferred image fetching, so we can call it "refactoring" :) Dockerfile will be implemented on top of it with no or really minor changes. I see some potential use cases because even without built-in support for Dockerfiles users of testcontainers might impement them already. So, if you don't mind, I would like to see it merged as a separate changeset before Dockerfile. Of couse I can continuously rebase it over the time until Dockerfile arrives, but I was hoping to avoid that :) |
|
Yep, no problem :) |
|
In fact, it adds one simple yet useful change: if you instantiate your container with RemoteDockerImage as an argument then it will be pulled only on container start, not during the constructor call. Previous code was calling "setDockerImage" in constructor when image name was passed as argument, and then, there, pull was triggered. Not a big issue since eventually it will be pulled but still something :) |
|
@rnorth sure, take your time:) I did some code changes (mostly de-duplication) to container pull logic so please review it carefuly. |
…roturnaround/testcontainers-java into zeroturnaround-feature/image_as_future Some minimal refactoring also included
…lready cached, to avoid creating a docker client
|
I've created a branch and added one commit to merge this cleanly into master, and one optimisation in the image checking code. I'm not quite sure if we can switch this PR to point to a branch, so can we continue review on #117 ? |
|
@rnorth sure :) See my comment there |
I'm going to contribute Dockerfile support, and this change is required to make it possible to pass not-yet-built images to GenericContainers. We achieve it with Future.
The previous implementation was extracted as "RemoteDockerImage".
Change must be fully binary and functionally compatible with previous implementation, but gives some opportunities in future.