-
-
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
Only publish exposed ports #4122
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -54,8 +56,8 @@ public ClickHouseContainer(final DockerImageName dockerImageName) { | |||
} | |||
|
|||
@Override | |||
protected Integer getLivenessCheckPort() { | |||
return getMappedPort(HTTP_PORT); | |||
public Set<Integer> getLivenessCheckPortNumbers() { |
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.
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
@@ -166,6 +166,19 @@ public String getConnectionString() { | |||
protected void configure() { | |||
super.configure(); | |||
|
|||
addExposedPorts( |
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.
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
core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
Outdated
Show resolved
Hide resolved
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.
💯
Fixes #4119