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

Deprecate CLI utility methods in RabbitMQ module #7588

Merged
merged 1 commit into from Sep 28, 2023

Conversation

eddumelendez
Copy link
Member

@eddumelendez eddumelendez commented Sep 26, 2023

Those utility methods couple the implementation to the cli, which is only provided when using management image. Also, although very convinient methods, it will be hard to maintain for every command in the cli.

@eddumelendez eddumelendez requested a review from a team as a code owner September 26, 2023 22:10
@eddumelendez eddumelendez added the type/deprecation Public APIs are being deprecated but not changed yet label Sep 26, 2023
@eddumelendez eddumelendez added this to the next milestone Sep 26, 2023
@eddumelendez eddumelendez changed the title Deprecate utility methods in RabbitMQ module Deprecate CLI utility methods in RabbitMQ module Sep 26, 2023
@eddumelendez eddumelendez merged commit 4c83a54 into main Sep 28, 2023
87 checks passed
@eddumelendez eddumelendez deleted the rabbitmq-utility-methods-deprecation branch September 28, 2023 20:07
@Philzen
Copy link

Philzen commented Oct 23, 2023

I see your point, but this change seriously broke the DX for us. In 1.19.0, we could initialize and configure the complete container in one command on a static field. B/c of the required exception handling (and also the different return type), we now would need to generate a dedicated ApplicationContextInitializer method to execute the raw commands via execInContainer.

What about moving these convenience methods into a dedicated subclass, i.e. RabbitMQMgmtContainer?

@Philzen
Copy link

Philzen commented Oct 23, 2023

Those utility methods couple the implementation to the cli, which is only provided when using management image.

Interestingly enough that is the default image:

https://github.com/testcontainers/testcontainers-java/blob/dd1427ebd30bbaba7f32184f1376b7c21e725ab5/modules/rabbitmq/src/main/java/org/testcontainers/containers/RabbitMQContainer.java#L39C40-L39C40

Also, although very convinient methods, it will be hard to maintain for every command in the cli.

@eddumelendez does RabbitMQ have a history of often changing or breaking the CLI options?

@eddumelendez
Copy link
Member Author

Hi, few notes:

  1. Default images should not be used. That's the reason why those are deprecated
  2. Testcontainers responsibility is about container initialization

@Philzen
Copy link

Philzen commented Oct 23, 2023

@eddumelendez i understand you points and that this was the proper action to reduce scope creep of testcontainers.

Do you think it would be OK to keep the current containerIsStarted hook and simply expose a single fluid utility method on RabbitMQContainer where one could add commands to the values array?

At the end of the day that's the biggest loss here and it would be superawesome to still have a way of configuring "onReady" commands while keeping object initialization down to a single command that doesn't require additional exception handling (as it's completely fine the way containerIsStarted currently logs them).

@eddumelendez
Copy link
Member Author

Do you think it would be OK to keep the current containerIsStarted hook and simply expose a single fluid utility method on RabbitMQContainer where one could add commands to the values array?

I think vault and consul are doing it like that. Indeed, this is something we have been discussing. But, aligning all the modules would be until version 2.0.

@oneiros-de
Copy link

May I kindly ask you to eat your own dog food and adapt your own tests, e.g. RabbitMQContainerTest.shouldCreateRabbitMQContainerWithExchangeInVhost ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules/rabbitmq type/deprecation Public APIs are being deprecated but not changed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants