add EXTRA_ADVERTISED_LISTENERS#3649
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe Kafka container initialization script in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/kafka/kafka.go (1)
24-24: Add explicit tests for extra listener append behaviorThe current advertised-listeners test (modules/kafka/kafka_test.go:77-91) validates
BROKER://<hostname>:9092, but not the new${...:+,...}append logic. Please add cases for (1) unset extra var, (2) set extra var, to lock formatting and comma behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/kafka/kafka.go` at line 24, Update the advertised-listeners unit test in modules/kafka/kafka_test.go to explicitly cover the new append behavior for the environment expansion in the KAFKA_ADVERTISED_LISTENERS export line: add two subcases—one where EXTRA_ADVERTISED_LISTENERS is unset (expect exactly "BROKER://<hostname>:9092" with no trailing comma/extra), and one where EXTRA_ADVERTISED_LISTENERS is set (expect "BROKER://<hostname>:9092,<extra-value>" i.e., comma plus extra); ensure the test asserts both the full exported string and that comma placement is correct so the "${EXTRA_ADVERTISED_LISTENERS:+,${EXTRA_ADVERTISED_LISTENERS}}" behavior is locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/kafka/kafka.go`:
- Line 24: Mismatch between the exported env var name and the documented
feature: replace the incorrect env var name used in the export string inside
modules/kafka/kafka.go (the export KAFKA_ADVERTISED_LISTENERS line that
currently references EXTRA_ADVERTISED_LISTENERS) with the documented environment
variable name used by the feature (ensure the token exactly matches the name
used in docs/other code), and then extend kafka_test.go to set that env var and
assert that its value is appended to the computed KAFKA_ADVERTISED_LISTENERS
(add a test case that sets the env var and verifies the EXTRA listeners are
included after BROKER://%s:9092).
---
Nitpick comments:
In `@modules/kafka/kafka.go`:
- Line 24: Update the advertised-listeners unit test in
modules/kafka/kafka_test.go to explicitly cover the new append behavior for the
environment expansion in the KAFKA_ADVERTISED_LISTENERS export line: add two
subcases—one where EXTRA_ADVERTISED_LISTENERS is unset (expect exactly
"BROKER://<hostname>:9092" with no trailing comma/extra), and one where
EXTRA_ADVERTISED_LISTENERS is set (expect
"BROKER://<hostname>:9092,<extra-value>" i.e., comma plus extra); ensure the
test asserts both the full exported string and that comma placement is correct
so the "${EXTRA_ADVERTISED_LISTENERS:+,${EXTRA_ADVERTISED_LISTENERS}}" behavior
is locked down.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03fccecb-3438-4967-862b-8e40593d09c9
📒 Files selected for processing (1)
modules/kafka/kafka.go
| starterScriptContent = `#!/bin/bash | ||
| source /etc/confluent/docker/bash-config | ||
| export KAFKA_ADVERTISED_LISTENERS=%s,BROKER://%s:9092 | ||
| export KAFKA_ADVERTISED_LISTENERS="%s,BROKER://%s:9092${EXTRA_ADVERTISED_LISTENERS:+,${EXTRA_ADVERTISED_LISTENERS}}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which variable name is used by docs/tests/code.
rg -n -C2 'EXTRA_KAFKA_ADVERTISED_LISTENERS|EXTRA_ADVERTISED_LISTENERS'Repository: testcontainers/testcontainers-go
Length of output: 499
🏁 Script executed:
# Search across entire codebase for any mention of EXTRA_KAFKA_ADVERTISED_LISTENERS
rg -i 'EXTRA_KAFKA' --type-list | head -20
rg 'EXTRA_KAFKA_ADVERTISED_LISTENERS' -t go -t md -t txt --type-list
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*README*" \) | head -20Repository: testcontainers/testcontainers-go
Length of output: 1447
🏁 Script executed:
# Check kafka_test.go for any test related to extra listeners
rg -A5 -B5 'EXTRA.*ADVERTISED|extra.*listener' modules/kafka/kafka_test.goRepository: testcontainers/testcontainers-go
Length of output: 58
🏁 Script executed:
# Look for any documentation or comments mentioning the feature name
rg -C3 'EXTRA_ADVERTISED_LISTENERS' modules/kafka/Repository: testcontainers/testcontainers-go
Length of output: 697
🏁 Script executed:
# Explicit search for EXTRA_KAFKA_ADVERTISED_LISTENERS
rg 'EXTRA_KAFKA_ADVERTISED_LISTENERS' --max-count 10Repository: testcontainers/testcontainers-go
Length of output: 58
🏁 Script executed:
# Read the kafka_test.go file to see test coverage
cat modules/kafka/kafka_test.goRepository: testcontainers/testcontainers-go
Length of output: 2915
🏁 Script executed:
# Search for any docs mentioning kafka advertised listeners
rg -l 'advertised.*listener|KAFKA_ADVERTISED' --type md --type txtRepository: testcontainers/testcontainers-go
Length of output: 58
Fix environment variable name mismatch in starter script
Line 24 uses EXTRA_ADVERTISED_LISTENERS, but this inconsistency with the documented feature name means users following the intended API contract will not have their settings applied. The code currently references an environment variable that does not appear elsewhere in the codebase or documentation.
🔧 Proposed fix
-export KAFKA_ADVERTISED_LISTENERS="%s,BROKER://%s:9092${EXTRA_ADVERTISED_LISTENERS:+,${EXTRA_ADVERTISED_LISTENERS}}"
+export KAFKA_ADVERTISED_LISTENERS="%s,BROKER://%s:9092${EXTRA_KAFKA_ADVERTISED_LISTENERS:+,${EXTRA_KAFKA_ADVERTISED_LISTENERS}}"Additionally, the existing test in kafka_test.go only validates the BROKER:// URL format and does not cover the extra advertised listeners append behavior. Consider adding a test to verify that extra listeners are properly applied when the environment variable is set.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export KAFKA_ADVERTISED_LISTENERS="%s,BROKER://%s:9092${EXTRA_ADVERTISED_LISTENERS:+,${EXTRA_ADVERTISED_LISTENERS}}" | |
| export KAFKA_ADVERTISED_LISTENERS="%s,BROKER://%s:9092${EXTRA_KAFKA_ADVERTISED_LISTENERS:+,${EXTRA_KAFKA_ADVERTISED_LISTENERS}}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/kafka/kafka.go` at line 24, Mismatch between the exported env var
name and the documented feature: replace the incorrect env var name used in the
export string inside modules/kafka/kafka.go (the export
KAFKA_ADVERTISED_LISTENERS line that currently references
EXTRA_ADVERTISED_LISTENERS) with the documented environment variable name used
by the feature (ensure the token exactly matches the name used in docs/other
code), and then extend kafka_test.go to set that env var and assert that its
value is appended to the computed KAFKA_ADVERTISED_LISTENERS (add a test case
that sets the env var and verifies the EXTRA listeners are included after
BROKER://%s:9092).
1 participant
What does this PR do?
Adds ability to use EXTRA_KAFKA_ADVERTISED_LISTENERS variable declared for kafka module to patch KAFKA_ADVERTISED_LISTENERS in a modules/kafka starterScriptContent.
Why is it important?
modules/kafka got const
starterScriptContentwith exportedKAFKA_ADVERTISED_LISTENERSthat is currently hidden for modifications from outside of the module. This add few problems with testing.Because, as you know kafka wants to know what hosts will be used to connect to it before it was started. For my usecase I want to connect from kafka ui to debug messages in topics in a local test environment. Current script have no patch points to KAFKA_ADVERTISED_LISTENERS.
Related issues
How to test this PR
Add
echo $KAFKA_ADVERTISED_LISTENERSand see how it works after bash use it. I can see how to test it automatically, i just want to know what do you think about that way of fixing it.