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.

Sign up for GitHub

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

Jump to bottom

feat: Add ActiveMQ Artemis module #1055

Merged
merged 6 commits into from
Nov 30, 2023
Merged

feat: Add ActiveMQ Artemis module #1055

merged 6 commits into from
Nov 30, 2023

Conversation

ShirAvneri
Copy link
Contributor

@ShirAvneri ShirAvneri commented Nov 23, 2023 •
edited
Loading

What does this PR do?

Adds an ActiveMQ Artemis container to the list of supported Testcontainers.

Why is it important?

Provides an ActiveMQ Artemis test container.

How to test this PR

Run Testcontainers.ActiveMq.Tests

@netlify Netlify
Copy link

netlify bot commented Nov 23, 2023 •
edited
Loading

✅ Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 8e2d554
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/65651c7ac8c3a90008e3d274
😎 Deploy Preview https://deploy-preview-1055--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@eddumelendez eddumelendez changed the title Add ActiveMQ container Add ActiveMQ Artemis container Nov 23, 2023
eddumelendez
eddumelendez reviewed Nov 23, 2023
View reviewed changes
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.

Nowadays, we can find ActiveMQ Classic and ActiveMQ Artemis. Although, the module refers to activemq, in general, would be great to be specific about Artemis instead of ActiveMQ. In Testcontainers for Java, the plan is to have one module to host both of them.

src/Testcontainers.ActiveMq/ActiveMqBuilder.cs Outdated Show resolved Hide resolved
tests/Testcontainers.ActiveMq.Tests/ActiveMqContainerTest.cs Outdated Show resolved Hide resolved
@ShirAvneri
Copy link
Contributor Author

Nowadays, we can find ActiveMQ Classic and ActiveMQ Artemis. Although, the module refers to activemq, in general, would be great to be specific about Artemis instead of ActiveMQ. In Testcontainers for Java, the plan is to have one module to host both of them.

@eddumelendez I can also include the classic container in this project, if that is okay? Or should they be hosted in different projects?

@eddumelendez
Copy link
Member

ActiveMQ Classic doesn't support credentials in env vars right now, the image have not been published yet, that's why the PR is still open in Testcontainers for Java.

My comment is related to the naming rather than adding support for ActiveMQ Classic.

HofmeisterAn
HofmeisterAn reviewed Nov 25, 2023
View reviewed changes
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! Great first contribution 💪 In my opinion, we can merge it. I just have a quick question (see the comment below).

src/Testcontainers.ActiveMq/ArtemisContainer.cs Show resolved Hide resolved
@HofmeisterAn HofmeisterAn changed the title Add ActiveMQ Artemis container feat: Add ActiveMQ Artemis module Nov 25, 2023
@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Nov 25, 2023
@HofmeisterAn
Copy link
Collaborator

@eddumelendez Is there anything left we should address before merging the PR?

HofmeisterAn
HofmeisterAn approved these changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@HofmeisterAn HofmeisterAn 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 updating the PR. I just noticed the changes today. LGTM — thanks again. I will merge it later in the day.

@HofmeisterAn HofmeisterAn merged commit 62975bc into testcontainers:develop Nov 30, 2023
8 checks passed
@eddumelendez
Copy link
Member

Great! Once released I'll merge the changes to make it available in the Modules Catalog. You can see the preview here.

@ShirAvneri ShirAvneri deleted the feature/activemq_container branch December 1, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddumelendez eddumelendez eddumelendez left review comments

@HofmeisterAn HofmeisterAn HofmeisterAn approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShirAvneri @eddumelendez @HofmeisterAn