-
-
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
Pulsar: add flag to enable transactions and set configuration #5479
Pulsar: add flag to enable transactions and set configuration #5479
Conversation
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
@eddumelendez @kiview PTAL |
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.
Thanks for the PR and special thanks for adding the docs @nicoloboschi. I added some general comments and questions 🙂
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Show resolved
Hide resolved
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
@kiview thanks for reviewing. I fixed your comments, could you review it again? |
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.
thanks @nicoloboschi ! Really glad to see PulsarContainer will make people life easier :)
I have added minor suggestions and opened the topic about the strategies order.
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/test/java/org/testcontainers/containers/PulsarContainerTest.java
Outdated
Show resolved
Hide resolved
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Show resolved
Hide resolved
CI is failing because of Spotless violations, you can run |
Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
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.
LGTM besides the open discussion about withConfiguration
.
I would prefer if we switch it to withEnv
, add an usage example to our docs, and further link the Pulsar docs once apache/pulsar#16085 is live.
@kiview I think you know better the testcontainers users than me |
modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java
Outdated
Show resolved
Hide resolved
@kiview @eddumelendez PTAL again, I hope it's the latest round 🤞 |
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.
LGTM, thanks for the great collaboration @nicoloboschi 🙌
withTransactions()
method to enable transactions on Pulsar container