Repository navigation
Conversation
|
|
||
| import org.testcontainers.images.RemoteDockerImage; | ||
|
|
||
| class ContainerDef extends BaseContainerDef { |
There was a problem hiding this comment.
can you please explain the need of this intermediate package-private type? I didn't get it
There was a problem hiding this comment.
See #4465. Eventually, BaseContainerDef's signature will include S extends Started type parameter, plus it is abstract.
| this(new RemoteDockerImage(image)); | ||
| } | ||
|
|
||
| BaseContainerDef createContainerDef(RemoteDockerImage image) { |
There was a problem hiding this comment.
What's the purpose of createContainerDef here?
There was a problem hiding this comment.
See KafkaContainer#createContainerDef
|
|
||
| Map<String, String> env = new HashMap<>(); | ||
|
|
||
| protected void putEnv(@NonNull String key, String value) { |
There was a problem hiding this comment.
I feel that mutable Container classes, and lack of builders, was a mistake I made in the initial Testcontainers APIs.
Are we unable to make ContainerDef immutable?
There was a problem hiding this comment.
We have two options:
- reuse
ContainerDefs inGenericContainerto avoid a lot of code duplicated - not reuse, but then we will need to add things to both places unless we promote this new API to stable
if we reuse, then, due to getEnv() and similar methods being used in an immutable way, we can't make underlying structures immutable (one of the first things I tried!).
BUT note that these methods are protected and only supposed to be used at the time of object's construction:
https://gist.github.com/bsideup/8247229e8a8dd86ac8bcf490e2ce6acb#file-newapitest-java-L7-L14
So, while it is not truly immutable, it dramatically improves the situation as compared to GenericContainer.
There was a problem hiding this comment.
Also note that, in 2.0, we can finally make it immutable
|
Superseded by #7714 |
Extracted this from #4465 to demonstrate the idea without the overhead of
Startedstate + only one state field (for now)