feat: enable access to kafka using container hostname#1786
Closed
sermio-te wants to merge 1 commit intotestcontainers:mainfrom
sermio-te:kafka-advertised-listener-fix
Closed
feat: enable access to kafka using container hostname#1786sermio-te wants to merge 1 commit intotestcontainers:mainfrom sermio-te:kafka-advertised-listener-fix
sermio-te wants to merge 1 commit intotestcontainers:mainfrom
sermio-te:kafka-advertised-listener-fix
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mdelapenya
reviewed
Oct 20, 2023
Member
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
This LGTM, although there is something needed to be changed. See comments inline
| } | ||
| containerHost := strings.TrimPrefix(name, "/") | ||
|
|
||
| scriptContent := fmt.Sprintf(starterScriptContent, host, port.Int(), containerHost) |
Member
There was a problem hiding this comment.
With this change, I think we do not need to get the host in L70 anymore, right?
Could you update it?
Member
There was a problem hiding this comment.
There are one thing to care about. The advertised listeners should be two:
- one will be listening from outside the containers and used by the clients through Brokers fn. This one used the host from where docker executes and not the container host
- other will be listening in the container itself
You can look at the Java implementation. We can align both implementations and get the config.Hostname which looks like is not available in testcontainers.Container interface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR applies a fix to the entrypoint script template variables used in the kafka module container in order to make kafka accessible to other containers in the same network.
Why is it important?
Kafka container created by the kafka module needs to be exposed to the other containers in the same network.
Related issues
How to test this PR
After starting a Kafka container using the Kafka module, the
KAFKA_ADVERTISED_LISTENERSshould contain the endpoint of the container host.E.g.
For Kafka container name set to
broker, the/usr/sbin/testcontainers_start.shscript should contain the following value: