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

#323 Fix for missed TINY_IMAGE during DockerClient initialization #324

Merged
merged 2 commits into from
Apr 6, 2017

Conversation

gorelikov
Copy link
Contributor

Fix for No such image: alpine:3.5 #323
@bsideup

if (images.stream().noneMatch(it -> it.getRepoTags() != null && asList(it.getRepoTags()).contains(TINY_IMAGE))) {
client.pullImageCmd(TINY_IMAGE).exec(new PullImageResultCallback()).awaitSuccess();
}
checkAndPullImage(client, TINY_IMAGE);
Copy link
Member

Choose a reason for hiding this comment

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

checkDiskSpaceAndHandleExceptions calls checkDiskSpace which calls runInsideDocker which calls checkAndPullImage after your change.

It means that we could remove checkAndPullImage from here and inline checkAndPullImage in runInsideDocker.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed that call, but kept the method checkAndPullImage. I don't like long methods, so i would keep the separate method, if you don't mind :)

private static final String SUCCESS = "SUCCESS";

@Test
public void runCommandInsideDockerShouldPullImageIfItDoesNotExistsLocally() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this test does too much. What it should test that runInsideDocker does not fail if image wasn't pulled yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I kept here only runInsideDocker method call

import static org.junit.Assert.assertTrue;

/**
* Created mgorelikov on 05/04/2017
Copy link
Member

Choose a reason for hiding this comment

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

auto-generated doc comment. I would remove it, doesn't help here.

However, you can include yourself in contributors list :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Sorry for that

@bsideup bsideup self-assigned this Apr 5, 2017
@bsideup bsideup added this to the 1.3.0 milestone Apr 5, 2017
@bsideup bsideup merged commit 9319ada into testcontainers:master Apr 6, 2017
@bsideup
Copy link
Member

bsideup commented Apr 6, 2017

@gorelikov merged, thanks for your contribution! :)

@dddpaul
Copy link

dddpaul commented Apr 6, 2017

Hi there. It's crucial PR for our CI because we still cannot test and build up services with Jenkins. I'm afraid it's to long to wait for 1.3.0 release. So I'd be glad if it would be rescheduled to 1.2.1, for example :)

@kiview
Copy link
Member

kiview commented Apr 6, 2017

You could always fallback to using a Jitpack dependency on the merged PR:
https://www.testcontainers.org/usage.html#maven-dependencies

@bsideup bsideup mentioned this pull request Apr 6, 2017
@bsideup bsideup modified the milestones: 1.2.1, 1.3.0 Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants