Skip to content
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

[Feature] Accept image as future instead of a value #106

Closed

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Mar 29, 2016

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.

@@ -63,7 +65,7 @@
private List<String> portBindings = new ArrayList<>();

@NonNull
private String dockerImageName = "alpine:3.2";
private Future<String> image;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bsideup changed the title Accept image as future instead of a value [Feature] Accept image as future instead of a value Mar 29, 2016
@rnorth
Copy link
Member

rnorth commented Mar 29, 2016

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 :)

@bsideup
Copy link
Member Author

bsideup commented Mar 29, 2016

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 :)

@rnorth
Copy link
Member

rnorth commented Mar 29, 2016

Yep, no problem :)
I think I'll have to check this out and poke around tomorrow, now, but I'll mark this for inclusion in the next release.
Thanks!

@rnorth rnorth added this to the 1.0.3 milestone Mar 29, 2016
@bsideup
Copy link
Member Author

bsideup commented Mar 29, 2016

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 :)

@bsideup
Copy link
Member Author

bsideup commented Mar 29, 2016

@rnorth sure, take your time:)

I did some code changes (mostly de-duplication) to container pull logic so please review it carefuly.

@rnorth rnorth modified the milestones: 1.0.3, 1.0.4 Mar 30, 2016
rnorth added a commit that referenced this pull request Apr 3, 2016
…roturnaround/testcontainers-java into zeroturnaround-feature/image_as_future

Some minimal refactoring also included
rnorth added a commit that referenced this pull request Apr 3, 2016
…lready cached, to avoid creating a docker client
@rnorth rnorth mentioned this pull request Apr 3, 2016
@rnorth
Copy link
Member

rnorth commented Apr 3, 2016

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 ?

@bsideup
Copy link
Member Author

bsideup commented Apr 3, 2016

@rnorth sure :) See my comment there

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.

2 participants