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.

Sign up for GitHub

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

Jump to bottom

Image name in remote docker image to string #2450

Closed
wants to merge 9 commits into from
Closed

Image name in remote docker image to string #2450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
410e96e
clean up imports in GenericContainer
Mar 17, 2020
12c3b1c
include image name in RemoteDockerImage.toString() to fix #2443
Mar 17, 2020
47329ba
test RemoteDockerImage directly
Mar 18, 2020
9b31632
handle failures getting the image name
Mar 19, 2020
1af3325
use completeExceptionally instead of mocking
Mar 19, 2020
c78defa
tweak assertions
Mar 19, 2020
8a7318f
more assertion tweaks
Mar 23, 2020
d6908d0
add isDone check to RemoteDockerImage.toString -- still figuring out …
Mar 23, 2020
1a31c5c
go back to using lombok @ToString, exclude dockerClient while we're a…
Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 1 addition & 10 deletions core/src/main/java/org/testcontainers/containers/GenericContainer.java
View file
Open in desktop
Expand Up @@ -26,9 +26,6 @@
import lombok.NonNull;
import lombok.Setter;
import lombok.SneakyThrows;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream;
import org.apache.commons.compress.utils.IOUtils;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.jetbrains.annotations.NotNull;
Expand All @@ -42,7 +39,6 @@
import org.slf4j.Logger;
import org.testcontainers.DockerClientFactory;
import org.testcontainers.UnstableAPI;
import org.testcontainers.images.ImagePullPolicy;
import org.testcontainers.containers.output.OutputFrame;
import org.testcontainers.containers.startupcheck.IsRunningStartupCheckStrategy;
import org.testcontainers.containers.startupcheck.MinimumDurationRunningStartupCheckStrategy;
Expand All @@ -51,8 +47,8 @@
import org.testcontainers.containers.wait.Wait;
import org.testcontainers.containers.wait.WaitStrategy;
import org.testcontainers.containers.wait.strategy.WaitStrategyTarget;
import org.testcontainers.images.ImagePullPolicy;
import org.testcontainers.images.RemoteDockerImage;
import org.testcontainers.images.builder.Transferable;
import org.testcontainers.lifecycle.Startable;
import org.testcontainers.lifecycle.Startables;
import org.testcontainers.lifecycle.TestDescription;
Expand All @@ -64,14 +60,9 @@
import org.testcontainers.utility.PathUtils;
import org.testcontainers.utility.ResourceReaper;
import org.testcontainers.utility.TestcontainersConfiguration;
import org.testcontainers.utility.ThrowingFunction;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.UndeclaredThrowableException;
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/org/testcontainers/images/RemoteDockerImage.java
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@ public class RemoteDockerImage extends LazyFuture<String> {

private static final Duration PULL_RETRY_TIME_LIMIT = Duration.ofMinutes(2);

@ToString.Exclude
private Future<DockerImageName> imageNameFuture;

@Wither
Expand All @@ -54,9 +55,8 @@ protected DockerImageName resolve() {
}

@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 +95,10 @@ protected final String resolve() {
throw new ContainerFetchException("Failed to get Docker client for " + imageName, e);
}
}

@ToString.Include(name = "imageName", rank = 1)
Copy link
Member

Choose a reason for hiding this comment

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

please add more tests for the scenario where imageNameFuture fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to do this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took a crack at it with 9b31632

Copy link
Member

Choose a reason for hiding this comment

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

btw why did you remove Lombok's toString? I really liked the @ToString.Include approach :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seemed like I needed a place to add logic about using imageName if available, and falling back to imageNameFuture.

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'm staring at it again now and I see a potential struggle. If I use @ToString.Include the "key" in the resulting string is always the same. I no longer get to choose whether to have imageName=foo when I have an image name, and fall back to imageNameFuture=....

But maybe it's OK, or even better, to have imageName= in both cases?

Copy link
Member

Choose a reason for hiding this comment

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

@dbyron0 could you please share examples of the toString call of both options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With @ToString.Include:

  • when imageName is available: RemoteDockerImage{imageName=ibemazgg:latest, imagePullPolicy=DefaultPullPolicy(), dockerClient=LazyDockerClient.INSTANCE}
  • when imageName isn't available: RemoteDockerImage{imageName=org.testcontainers.images.RemoteDockerImage$1@5d01486, imagePullPolicy=DefaultPullPolicy(), dockerClient=LazyDockerClient.INSTANCE}

Without lombok:

  • when imageName is available: RemoteDockerImage{imageName=ibemazgg:latest, imagePullPolicy=DefaultPullPolicy(), dockerClient=LazyDockerClient.INSTANCE}
  • when imageName isn't available: RemoteDockerImage{imageNameFuture=org.testcontainers.images.RemoteDockerImage$1@5d01486, imagePullPolicy=DefaultPullPolicy(), dockerClient=LazyDockerClient.INSTANCE}

Copy link
Member

Choose a reason for hiding this comment

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

I kinda like the Lombok version :)

side note: I would also exclude dockerClient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough: 1a31c5c

Would love a hand figure out the test failure / calling isDone.

@SneakyThrows({InterruptedException.class, ExecutionException.class})
DockerImageName getImageName() {
return imageNameFuture.get();
}
}
14 changes: 13 additions & 1 deletion core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
View file
Open in desktop
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 Expand Up @@ -54,6 +53,19 @@ public void shouldReportErrorAfterWait() {
}
}

@Test
public void shouldLogImageNameWhenGetDockerImageNameFails() {
// A docker image that doesn't exist is enough. The NotFoundException
// that the ContainerFetchException wraps may contain the image name.
// This test verifies that the ContainerFetchException itself does.
String imageName = "doesnotexist";
bsideup marked this conversation as resolved.
Show resolved Hide resolved
try(GenericContainer container = new GenericContainer<>(imageName)) {
bsideup marked this conversation as resolved.
Show resolved Hide resolved
assertThatThrownBy(container::getDockerImageName)
.isInstanceOf(ContainerFetchException.class)
.hasMessageContaining(imageName);
}
}

static class NoopStartupCheckStrategy extends StartupCheckStrategy {

@Override
Expand Down