-
Notifications
You must be signed in to change notification settings - Fork 18
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
Kafka Kraft Mode support via confluent-local image #623
Conversation
@@ -56,7 +63,11 @@ protected String getDefaultImageName() { | |||
|
|||
@Override | |||
protected KafkaContainer createContainer(DockerImageName imageName, Map<String, Object> requestedProperties, Map<String, Object> testResourcesConfig) { | |||
return new KafkaContainer(imageName); | |||
if (isKraftMode(testResourcesConfig)) { |
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 attempted to do a separate class that leveraged the statics (e.g. display, simple name) in this class since they were the same.
That approach did not work. I'm guessing because they were both of AbstractTestContainersProvider<KafkaContainer>
and caused the KafkaKraftTestResourceProvider
to always be ignored?
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 think we should take a slightly different approach to fix a discrepancy. The base image is different whether you use kraft mode or not. However, we also have the generic property to override the base image, so the imageName
that you get may be different from what getDefaultImageName()
returns. Therefore, I would add a check if the image name has been changed, and if so, keep it (but still enable kraft mode on that changed image if needed).
Otherwise, you lose the ability to override the version of the kraft image.
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.
@melix how do I check if the image name has been changed? I see how it's done in AbstractTestContainersProvider, but I cannot use TestContainerMetadataSupport
inside KafkaTestResourceProvider
because
'io. micronaut. testresources. testcontainers. TestContainerMetadataSupport' is not public in 'io. micronaut. testresources. testcontainers'. Cannot be accessed from outside package
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.
You have access to the imageName
as a method parameter. I would compare it with getDefaultImageName()
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.
Whoops 😅
@melix sure you are busy, just checking in especially on the alternative approach issue I hit. |
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.
Hi @scprek , sorry I forgot about this PR.
@@ -56,7 +63,11 @@ protected String getDefaultImageName() { | |||
|
|||
@Override | |||
protected KafkaContainer createContainer(DockerImageName imageName, Map<String, Object> requestedProperties, Map<String, Object> testResourcesConfig) { | |||
return new KafkaContainer(imageName); | |||
if (isKraftMode(testResourcesConfig)) { |
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 think we should take a slightly different approach to fix a discrepancy. The base image is different whether you use kraft mode or not. However, we also have the generic property to override the base image, so the imageName
that you get may be different from what getDefaultImageName()
returns. Therefore, I would add a check if the image name has been changed, and if so, keep it (but still enable kraft mode on that changed image if needed).
Otherwise, you lose the ability to override the version of the kraft image.
Thanks for the PR update! |
Sorry it took so long! Was in micronaut-openapi space for a while ha. |
Fixes: #622
I attempted to do a separate class, similar to Redis vs RedisCluster mode. That approach did not work. I'm guessing because they were both of
AbstractTestContainersProvider<KafkaContainer>
and caused theKafkaKraftTestResourceProvider
to always be ignored?In KafkaTestResourceProvider I made sure to provide an empty list of properties if in Kraft Mode.
New class