Skip to content

Pulsar: add flag to enable transactions and set configuration#5479

Merged
eddumelendez merged 9 commits intotestcontainers:masterfrom
nicoloboschi:nb-pulsar-transactions
Jun 20, 2022
Merged

Pulsar: add flag to enable transactions and set configuration#5479
eddumelendez merged 9 commits intotestcontainers:masterfrom
nicoloboschi:nb-pulsar-transactions

Conversation

@nicoloboschi
Copy link
Copy Markdown
Contributor

@nicoloboschi nicoloboschi commented Jun 8, 2022 •
edited
Loading

  • 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 June 8, 2022 21:21
@nicoloboschi nicoloboschi mentioned this pull request Jun 9, 2022
Open
@sijie sijie mentioned this pull request Jun 9, 2022
Open
@nicoloboschi
Copy link
Copy Markdown
Contributor Author

nicoloboschi commented Jun 15, 2022

@eddumelendez @kiview PTAL

@kiview kiview added this to the next milestone Jun 15, 2022
kiview
kiview reviewed Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

kiview commented Jun 15, 2022

Pulsar tests failing on CI:
https://github.com/testcontainers/testcontainers-java/runs/6901742299?check_suite_focus=true#step:7:104

@nicoloboschi
Copy link
Copy Markdown
Contributor Author

nicoloboschi commented Jun 15, 2022

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

eddumelendez
eddumelendez reviewed Jun 16, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

kiview commented Jun 17, 2022 •
edited
Loading

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
kiview
kiview reviewed Jun 17, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

nicoloboschi commented Jun 17, 2022

@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
Copy Markdown
Contributor Author

nicoloboschi commented Jun 20, 2022

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

kiview
kiview approved these changes Jun 20, 2022
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@eddumelendez eddumelendez eddumelendez approved these changes

@kiview kiview kiview approved these changes

+1 more reviewer

@eolivelli eolivelli eolivelli left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

modules/pulsar type/enhancement

Projects

None yet

Milestone

1.17.3

Development

Successfully merging this pull request may close these issues.

4 participants

@nicoloboschi @kiview @eddumelendez @eolivelli