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

Include image name in RemoteDockerImage#toString #2558

Merged
merged 14 commits into from Apr 12, 2020
Merged
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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)) {
Expand Down Expand Up @@ -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();
Copy link
Member

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?

Copy link
Member Author

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 😅

} catch (InterruptedException | ExecutionException e) {
return e.getMessage();
Copy link
Member

Choose a reason for hiding this comment

The 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...

Copy link
Member Author

Choose a reason for hiding this comment

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

this is for toString() - it would be weird to rethrow from toString IMO. Instead, we return the message, so that RemoveDockerImage#toString shows that there was an exception

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, might create some nasty bugs if rethrowing.

}
}
}
Expand Up @@ -2,7 +2,6 @@

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.InspectContainerResponse.ContainerState;
import com.github.dockerjava.api.model.HostConfig;
import lombok.RequiredArgsConstructor;
import lombok.SneakyThrows;
import lombok.experimental.FieldDefaults;
Expand Down
@@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 toStringWithCompletedImageName ?

String imageName = Base58.randomString(8).toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

Why random string?

Copy link
Member Author

Choose a reason for hiding this comment

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

why not? :)

Copy link
Member

Choose a reason for hiding this comment

The 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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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));
}
}