-
-
Notifications
You must be signed in to change notification settings - Fork 590
feat(kafka): add apache/kafka and apache/kafka-native support #3249
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
1ef2f84
4ebe2d1
0a80e8c
32b803c
0e2dfab
57f0cd9
cb66883
246fdae
e5c5ebb
b131199
13e9773
5da08a1
d641100
5a57c72
ea05212
b55f3fd
5cbb28d
5946d39
9c948d2
9ae246b
12a1ae9
99b4833
b51a341
a2eb5e3
b21194e
4b7ebda
4d5a299
8c1a7ab
9adbde3
fcfee34
f227e38
164ff1b
31f4579
f6f75b2
1e916ee
9a00515
ad56cd2
a3a5cb7
fb876de
57b8a7c
5fe7204
0e923e0
f83d07d
b65ba06
4f02648
8706053
5f5e8fb
b8de9b1
c6a2b1d
81b1065
34242df
907ce01
4627d1b
1fc9b85
68f2f3f
3034eea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,8 +19,8 @@ const publicPort = nat.Port("9093/tcp") | |
| const ( | ||
| starterScript = "/usr/sbin/testcontainers_start.sh" | ||
|
|
||
| // starterScript { | ||
| starterScriptContent = `#!/bin/bash | ||
| // starterScriptConfluentinc { | ||
| confluentincStarterScriptContent = `#!/bin/bash | ||
| source /etc/confluent/docker/bash-config | ||
| export KAFKA_ADVERTISED_LISTENERS=%s,BROKER://%s:9092 | ||
| echo Starting Kafka KRaft mode | ||
|
|
@@ -30,6 +30,13 @@ echo '' > /etc/confluent/docker/ensure | |
| /etc/confluent/docker/configure | ||
| /etc/confluent/docker/launch` | ||
strowk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // } | ||
|
|
||
| // starterScriptApache { | ||
| apacheStarterScriptContent = `#!/bin/bash | ||
| export KAFKA_ADVERTISED_LISTENERS=%s,BROKER://%s:9092 | ||
strowk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| echo Starting Apache Kafka | ||
| exec /etc/kafka/docker/run` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid hard-coded value like it is done in #3488?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a certain benefit in keeping hardcoded value tho, it is a bit clearer what is going on when looking at docs: Docs basically take out snippets of code to include inside built static website. The way my PR does this, it looks like this:
Plus I kind of doubt that increasing complexity of this script would make it better. There is some chance that entrypoint would change in the future Apache Kafka versions, but is this probability plus losses from users facing related problems higher than efforts of testcontainers maintainers understanding what is going on in that script-building logic?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I can be too worried of (old) docker images being removed from Docker Hub (like the recent case with JDK images). It is the reason I tried to make #3488 a little bit more forward-compatible. Considering documentation code snippet, I suppose you (@strowk) is the best decision maker for this comment thread. Thank you for clarification.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll keep this open until there is maintainer to weigh-in..
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stevenh , @mdelapenya , do you guys have any opinion on this? Should we try to derive the executable from inspecting the image or just keep them hardcoded until/if they change?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mabrarov to confirm my understanding you're suggesting using the detail from inspect to identify the location of binary to exec, so that if that changed in the future it would remain compatible?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @stevenh, Yes, your understanding is correct (e.g. Thank you.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, so basically my feeling about this was that I am not sure if this technique would work reliably enough, i.e it won't break in some setups that would make resulting code less stable, not more, i.e trade off is extra complexity (and chance of bug) vs likeliness of ENTRYPOINT or CMD becoming different in one of flavors (or maybe users providing their own images without their own starter scripts.. in which case inspection might help too). |
||
| // } | ||
| ) | ||
|
|
||
| // KafkaContainer represents the Kafka container type used in the module | ||
|
|
@@ -78,7 +85,7 @@ func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustom | |
| // if the starter script fails to copy. | ||
| func(ctx context.Context, c testcontainers.Container) error { | ||
| // 1. copy the starter script into the container | ||
| if err := copyStarterScript(ctx, c); err != nil { | ||
| if err := copyStarterScript(ctx, img, c); err != nil { | ||
| return fmt.Errorf("copy starter script: %w", err) | ||
| } | ||
|
|
||
|
|
@@ -122,7 +129,7 @@ func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustom | |
| } | ||
|
|
||
| // copyStarterScript copies the starter script into the container. | ||
| func copyStarterScript(ctx context.Context, c testcontainers.Container) error { | ||
| func copyStarterScript(ctx context.Context, img string, c testcontainers.Container) error { | ||
| if err := wait.ForMappedPort(publicPort). | ||
| WaitUntilReady(ctx, c); err != nil { | ||
| return fmt.Errorf("wait for mapped port: %w", err) | ||
|
|
@@ -140,7 +147,7 @@ func copyStarterScript(ctx context.Context, c testcontainers.Container) error { | |
|
|
||
| hostname := inspect.Config.Hostname | ||
|
|
||
| scriptContent := fmt.Sprintf(starterScriptContent, endpoint, hostname) | ||
| scriptContent := fmt.Sprintf(getStarterScriptContent(img), endpoint, hostname) | ||
|
|
||
| if err := c.CopyToContainer(ctx, []byte(scriptContent), starterScript, 0o755); err != nil { | ||
| return fmt.Errorf("copy to container: %w", err) | ||
|
|
@@ -200,7 +207,7 @@ func validateKRaftVersion(fqName string) error { | |
| image := fqName[:strings.LastIndex(fqName, ":")] | ||
| version := fqName[strings.LastIndex(fqName, ":")+1:] | ||
|
|
||
| if !strings.EqualFold(image, "confluentinc/confluent-local") { | ||
| if !isConfluentinc(image) { | ||
| // do not validate if the image is not the official one. | ||
| // not raising an error here, letting the image start and | ||
| // eventually evaluate an error if it exists. | ||
|
|
||
strowk marked this conversation as resolved.
Show resolved
Hide resolved
|
strowk marked this conversation as resolved.
Show resolved
Hide resolved
strowk marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: align filename with behaviour of the methods, type, flavour or other depending on the outcome of the other comments. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package kafka | ||
|
|
||
| import "strings" | ||
|
|
||
| const ( | ||
| apacheKafkaImagePrefix = "apache/kafka" | ||
| confluentincImagePrefix = "confluentinc/" | ||
| dockerIoPrefix = "docker.io/" | ||
| ) | ||
|
|
||
| func isApache(image string) bool { | ||
| return strings.HasPrefix(image, apacheKafkaImagePrefix) || strings.HasPrefix(image, dockerIoPrefix+apacheKafkaImagePrefix) | ||
| } | ||
|
|
||
| func isConfluentinc(image string) bool { | ||
| return strings.HasPrefix(image, confluentincImagePrefix) || strings.HasPrefix(image, dockerIoPrefix+confluentincImagePrefix) | ||
| } | ||
|
|
||
| func getStarterScriptContent(image string) string { | ||
| if isApache(image) { | ||
| return apacheStarterScriptContent | ||
| } else if isConfluentinc(image) { | ||
| return confluentincStarterScriptContent | ||
| } else { | ||
| // Default to confluentinc for backward compatibility | ||
| // in situations when image was custom specified based on confluentinc | ||
| return confluentincStarterScriptContent | ||
| } | ||
| } |


Uh oh!
There was an error while loading. Please reload this page.