Image name in remote docker image to string #2450
Conversation
dbyron0
force-pushed
the
image_name_in_RemoteDockerImage_toString
branch
from
dc4a5c8
to
12c3b1c
Compare
core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
Outdated
Show resolved
Hide resolved
| @@ -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) | |||
There was a problem hiding this comment.
please add more tests for the scenario where imageNameFuture fails
There was a problem hiding this comment.
Still need to do this...
There was a problem hiding this comment.
Took a crack at it with 9b31632
There was a problem hiding this comment.
btw why did you remove Lombok's toString? I really liked the @ToString.Include approach :)
There was a problem hiding this comment.
Seemed like I needed a place to add logic about using imageName if available, and falling back to imageNameFuture.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@dbyron0 could you please share examples of the toString call of both options?
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
I kinda like the Lombok version :)
side note: I would also exclude dockerClient.
There was a problem hiding this comment.
Fair enough: 1a31c5c
Would love a hand figure out the test failure / calling isDone.
core/src/main/java/org/testcontainers/images/RemoteDockerImage.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/images/RemoteDockerImageTest.java
Outdated
Show resolved
Hide resolved
|
@dbyron0 I am looking at the test failure now. Have you got a chance to debug it further? |
|
Sorry, I haven't. |
|
@dbyron0 could you please give me permissions to push to your PR? I have a fix :) |
|
Superseeded by #2450 |
|
Guess I wasn't fast enough...Looks like #2558 took over from here. Thanks much! |
|
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
With this change, the ContainerFetchException looks like:
where before it looked like: