Skip to content
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

Merged

Conversation

nicoloboschi
Copy link
Contributor

@nicoloboschi nicoloboschi commented Jun 8, 2022

  • Add withTransactions() method to enable transactions on Pulsar container
  • Set default version to latest released (2.10.0)
  • Added new tests to cover new methods
  • Improved the documentation with simple usage and new methods

@nicoloboschi nicoloboschi requested a review from a team as a code owner June 8, 2022 21:21
@nicoloboschi
Copy link
Contributor Author

@eddumelendez @kiview PTAL

Copy link
Member

@kiview kiview left a 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 🙂

@kiview
Copy link
Member

kiview commented Jun 15, 2022

@nicoloboschi
Copy link
Contributor Author

@kiview thanks for reviewing. I fixed your comments, could you review it again?

Copy link
Member

@eddumelendez eddumelendez left a 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.

@kiview
Copy link
Member

kiview commented Jun 17, 2022

CI is failing because of Spotless violations, you can run ./gradlew :pulsar:spotlessApply to fix them.

nicoloboschi and others added 2 commits June 17, 2022 13:51
Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
Copy link
Member

@kiview kiview left a 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.

@nicoloboschi
Copy link
Contributor Author

@kiview I think you know better the testcontainers users than me
I removed the custom configuration method, added the constant inside the PulsarContainer with javadoc, I added a paragraph in the website that points to the Pulsar website for the Docker setup.
Also the explanation about PULSAR_PREFIX_ is already live: https://pulsar.apache.org/docs/getting-started-docker/#start-pulsar-in-docker

@nicoloboschi
Copy link
Contributor Author

@kiview @eddumelendez PTAL again, I hope it's the latest round 🤞

Copy link
Member

@kiview kiview left a 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 🙌

@eddumelendez eddumelendez merged commit 2afe714 into testcontainers:master Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants