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
Include image name in RemoteDockerImage#toString #2558
Changes from all commits
410e96e
12c3b1c
47329ba
9b31632
1af3325
c78defa
8a7318f
d6908d0
1a31c5c
d70c67a
4f627bc
6756f78
31630ce
d5c42d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import com.github.dockerjava.api.DockerClient; | ||
import com.github.dockerjava.api.exception.DockerClientException; | ||
import com.github.dockerjava.api.exception.InternalServerErrorException; | ||
import com.google.common.util.concurrent.Futures; | ||
import lombok.AccessLevel; | ||
import lombok.AllArgsConstructor; | ||
import lombok.NonNull; | ||
|
@@ -28,11 +29,13 @@ public class RemoteDockerImage extends LazyFuture<String> { | |
|
||
private static final Duration PULL_RETRY_TIME_LIMIT = Duration.ofMinutes(2); | ||
|
||
@ToString.Exclude | ||
private Future<DockerImageName> imageNameFuture; | ||
|
||
@Wither | ||
private ImagePullPolicy imagePullPolicy = PullPolicy.defaultPolicy(); | ||
|
||
@ToString.Exclude | ||
private DockerClient dockerClient = DockerClientFactory.lazyClient(); | ||
|
||
public RemoteDockerImage(String dockerImageName) { | ||
|
@@ -44,19 +47,13 @@ public RemoteDockerImage(@NonNull String repository, @NonNull String tag) { | |
} | ||
|
||
public RemoteDockerImage(@NonNull Future<String> imageFuture) { | ||
this.imageNameFuture = new LazyFuture<DockerImageName>() { | ||
@Override | ||
@SneakyThrows({InterruptedException.class, ExecutionException.class}) | ||
protected DockerImageName resolve() { | ||
return new DockerImageName(imageFuture.get()); | ||
} | ||
}; | ||
this.imageNameFuture = Futures.lazyTransform(imageFuture, DockerImageName::new); | ||
} | ||
|
||
@Override | ||
@SneakyThrows({InterruptedException.class, ExecutionException.class}) | ||
protected final String resolve() { | ||
final DockerImageName imageName = imageNameFuture.get(); | ||
final DockerImageName imageName = getImageName(); | ||
Logger logger = DockerLoggerFactory.getLogger(imageName.toString()); | ||
try { | ||
if (!imagePullPolicy.shouldPull(imageName)) { | ||
|
@@ -95,4 +92,21 @@ protected final String resolve() { | |
throw new ContainerFetchException("Failed to get Docker client for " + imageName, e); | ||
} | ||
} | ||
|
||
private DockerImageName getImageName() throws InterruptedException, ExecutionException { | ||
return imageNameFuture.get(); | ||
} | ||
|
||
@ToString.Include(name = "imageName", rank = 1) | ||
private String imageNameToString() { | ||
if (!imageNameFuture.isDone()) { | ||
return "<resolving>"; | ||
} | ||
|
||
try { | ||
return getImageName().toString(); | ||
} catch (InterruptedException | ExecutionException e) { | ||
return e.getMessage(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This caught me by surprise - in case of a failure, why would we not rethrow? At first sight it seems odd that an exception message could become the 'name' of the image... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough - I guess we can't do too much more than this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, might create some nasty bugs if rethrowing. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package org.testcontainers.images; | ||
|
||
import static org.hamcrest.CoreMatchers.containsString; | ||
import static org.junit.Assert.assertFalse; | ||
import static org.junit.Assert.assertThat; | ||
|
||
import org.junit.Test; | ||
import org.testcontainers.utility.Base58; | ||
|
||
import java.util.concurrent.CompletableFuture; | ||
|
||
public class RemoteDockerImageTest { | ||
|
||
@Test | ||
public void toStringContainsOnlyImageName() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method name came to be back when there was a choice between imageName and imageNameFuture. Maybe a better name now is |
||
String imageName = Base58.randomString(8).toLowerCase(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why random string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because constant example string seems more controlled to me. |
||
RemoteDockerImage remoteDockerImage = new RemoteDockerImage(imageName); | ||
assertThat(remoteDockerImage.toString(), containsString("imageName=" + imageName)); | ||
} | ||
|
||
@Test | ||
public void toStringWithExceptionContainsOnlyImageNameFuture() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this method name doesn't really match the contents anymore |
||
CompletableFuture<String> imageNameFuture = new CompletableFuture<>(); | ||
imageNameFuture.completeExceptionally(new RuntimeException("arbitrary")); | ||
|
||
RemoteDockerImage remoteDockerImage = new RemoteDockerImage(imageNameFuture); | ||
assertThat(remoteDockerImage.toString(), containsString("imageName=java.lang.RuntimeException: arbitrary")); | ||
} | ||
|
||
@Test(timeout=5000L) | ||
public void toStringDoesntResolveImageNameFuture() { | ||
CompletableFuture<String> imageNameFuture = new CompletableFuture<>(); | ||
|
||
// verify that we've set up the test properly | ||
assertFalse(imageNameFuture.isDone()); | ||
|
||
RemoteDockerImage remoteDockerImage = new RemoteDockerImage(imageNameFuture); | ||
assertThat(remoteDockerImage.toString(), containsString("imageName=<resolving>")); | ||
|
||
// Make sure the act of calling toString doesn't resolve the imageNameFuture | ||
assertFalse(imageNameFuture.isDone()); | ||
|
||
String imageName = Base58.randomString(8).toLowerCase(); | ||
imageNameFuture.complete(imageName); | ||
assertThat(remoteDockerImage.toString(), containsString("imageName=" + 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.
Why is try necessary after the future is done?
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.
you need to get the value from the future. Unless I miss something 😅