Only publish exposed ports #4122
Conversation
rnorth
force-pushed
the
only-publish-exposed-ports
branch
from
0b15097 to
0e0e8c8
Compare
…tes cause issues later
…hat non-exposed port is not bound
| @@ -1,8 +1,5 @@ | |||
| package org.testcontainers.containers; | |||
There was a problem hiding this comment.
Regarding imports, there are some changes in ordering, but AFAICT these only match the IntelliJ defaults (with our .editorconfig tweaks layered on top for star imports).
TBH I'd prefer to go with the changed (hopefully stable) import order in this PR, rather than continue to constantly have the burden of maintaining the order (I don't know why the current order is as it is).
I think if we find ourselves continuing to run up against this then we should introduce a code formatter to our build (e.g. spotless gradle plugin + google formatter with AOSP style (4 space indent)).
| // Next, ExposedPorts must be set up to publish all of the above ports, randomized and fixed. | ||
| // Collect all of the exposed ports for publication | ||
| final List<ExposedPort> exposedPorts = allPortBindings.stream() | ||
| .map(PortBinding::getExposedPort) | ||
| .distinct() | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
This is the thing that is substantially different from the previous implementation; changes above are mainly to support this change and to make it clearer what we're doing.
Of course, createCommand.withPublishAllPorts(true); on line 801 is also nuked.
| @Override | ||
| protected Integer getLivenessCheckPort() { | ||
| return getMappedPort(HTTP_PORT); | ||
| public Set<Integer> getLivenessCheckPortNumbers() { |
There was a problem hiding this comment.
Not strictly required for this PR, but during experimenting with approaches I found that a handful of our modules are using the getLivenessCheckPort method, which has been deprecated for 4 years. I'd like to take this chance to clean up.
modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseContainer.java
Show resolved
Hide resolved
| protected void configure() { | ||
| super.configure(); | ||
|
|
||
| addExposedPorts( |
There was a problem hiding this comment.
Interestingly the Couchbase module seems to have always relied on an EXPOSE statement + all-ports publication.
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
Fixes #4119