-
-
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
Simplify Kafka container by deferring the Kafka command #1458
Conversation
modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
Outdated
Show resolved
Hide resolved
@NonNull | ||
public synchronized Network getNetwork() { | ||
if (super.getNetwork() == null) { | ||
// Backward compatibility |
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.
Do we really want to add this for backward compatibility? IMO the network was an implementation detail before, not really part of the public API of the class.
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.
Is the timing risky here? As in:
(a) could the container come up automatically on the bridge network, but then return a completely different network to user code when this method is called?
(b) Is it possible that this code will be invoked after the container is started, meaning that withNetwork
is not going to do the right thing...
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've reverted the change, let's tackle it later (added a TODO)
modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
Outdated
Show resolved
Hide resolved
modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
Show resolved
Hide resolved
…iners#1312) Bumps [assertj-core](https://github.com/joel-costigliola/assertj-core) from 3.12.1 to 3.12.2. <details> <summary>Commits</summary> - [`5dd7fac`](assertj/assertj@5dd7fac) [maven-release-plugin] prepare release assertj-core-3.12.2 - [`d427863`](assertj/assertj@d427863) Update Guava test dependency to version 27.1-jre. - [`cb5b285`](assertj/assertj@cb5b285) anySatisfy (Maps): do not continue evaluating elements once we found a match ... - [`8d83c41`](assertj/assertj@8d83c41) Ignore static methods when finding property accessors. Fixes [testcontainers#1458](https://github-redirect.dependabot.com/joel-costigliola/assertj-core/issues/1458) - [`b600f5b`](assertj/assertj@b600f5b) Use URI#create (which does not throw a checked Exception) in tests. - [`91af5c9`](assertj/assertj@91af5c9) [maven-release-plugin] prepare for next development iteration - See full diff in [compare view](assertj/assertj@assertj-core-3.12.1...assertj-core-3.12.2) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.assertj:assertj-core&package-manager=gradle&previous-version=3.12.1&new-version=3.12.2)](https://dependabot.com/compatibility-score.html?dependency-name=org.assertj:assertj-core&package-manager=gradle&previous-version=3.12.1&new-version=3.12.2) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) If all status checks pass Dependabot will automatically merge this pull request. [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in the `.dependabot/config.yml` file in this repo: - Update frequency (including time of day and day of week) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) Finally, you can contact us by mentioning @dependabot. </details>
[//]: # (dependabot-start)⚠️ **Dependabot is rebasing this PR**⚠️ If you make any changes to it yourself then they will take precedence over the rebase. --- [//]: # (dependabot-end) Bumps [assertj-core](https://github.com/joel-costigliola/assertj-core) from 3.12.1 to 3.12.2. <details> <summary>Commits</summary> - [`5dd7fac`](assertj/assertj@5dd7fac) [maven-release-plugin] prepare release assertj-core-3.12.2 - [`d427863`](assertj/assertj@d427863) Update Guava test dependency to version 27.1-jre. - [`cb5b285`](assertj/assertj@cb5b285) anySatisfy (Maps): do not continue evaluating elements once we found a match ... - [`8d83c41`](assertj/assertj@8d83c41) Ignore static methods when finding property accessors. Fixes [testcontainers#1458](https://github-redirect.dependabot.com/joel-costigliola/assertj-core/issues/1458) - [`b600f5b`](assertj/assertj@b600f5b) Use URI#create (which does not throw a checked Exception) in tests. - [`91af5c9`](assertj/assertj@91af5c9) [maven-release-plugin] prepare for next development iteration - See full diff in [compare view](assertj/assertj@assertj-core-3.12.1...assertj-core-3.12.2) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.assertj:assertj-core&package-manager=gradle&previous-version=3.12.1&new-version=3.12.2)](https://dependabot.com/compatibility-score.html?dependency-name=org.assertj:assertj-core&package-manager=gradle&previous-version=3.12.1&new-version=3.12.2) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) If all status checks pass Dependabot will automatically merge this pull request. [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in the `.dependabot/config.yml` file in this repo: - Update frequency (including time of day and day of week) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) Finally, you can contact us by mentioning @dependabot. </details>
@@ -54,40 +56,67 @@ public KafkaContainer withExternalZookeeper(String connectString) { | |||
} | |||
|
|||
public String getBootstrapServers() { | |||
return String.format("PLAINTEXT://%s:%s", proxy.getContainerIpAddress(), proxy.getFirstMappedPort()); | |||
if (port == -1) { | |||
throw new IllegalStateException("You should start Kafka container first"); |
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.
Nice - this will fix #1450
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.
Yeah, this is cute :)
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.
Looks good on the first pass!
@NonNull | ||
public synchronized Network getNetwork() { | ||
if (super.getNetwork() == null) { | ||
// Backward compatibility |
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.
Is the timing risky here? As in:
(a) could the container come up automatically on the bridge network, but then return a completely different network to user code when this method is called?
(b) Is it possible that this code will be invoked after the container is started, meaning that withNetwork
is not going to do the right thing...
It is possible to remove the Socat & networks from KafkaContainer.
The algorithm:
sleep infinity
docker exec ...
docker exec ...