-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Image name in remote docker image to string #2450
Image name in remote docker image to string #2450
Conversation
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.
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
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.
Still need to do this...
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.
Took a crack at it with 9b31632
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.
btw why did you remove Lombok's toString? I really liked the @ToString.Include
approach :)
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.
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.
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?
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.
@dbyron0 could you please share examples of the toString
call of both options?
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.
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.
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
.
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.
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: