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

Refactor: extracted RabbitMQ node management functions from IntegrationFixture #884

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

pergh
Copy link

@pergh pergh commented Jun 30, 2020

Proposed Changes

Reuse of code for controlling RabbitMQ server is made simpler for external projects.

IntegrationFixture is now simpler and does no longer contain low level RabbitMQ server controlling methods.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Refactoring

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Simplifying IntegrationFixture.
Make reuse of code for controlling RabbitMQ server simpler.
@michaelklishin
Copy link
Member

michaelklishin commented Jun 30, 2020

Thank you for considering a contribution.

I'm not sure how this makes much difference or make anything simpler (easier, possibly, but not sure what). It's already possible to run tests with a RabbitMQ node running in a container and override the expected rabbitmqctl location. A little more context, please.

Also, we never spell RabbitMQ as "RabbitMq" and "server controller" at first made me think this was a contribution to the Kubernetes operator repository. IIRC Java client uses RabbitMQCtl as the name for a similar collection of static methods.

@pergh
Copy link
Author

pergh commented Jun 30, 2020

Thanks for prompt reply!

In context of the RabbitMQClient itself the change makes the code only marginally cleaner.

Our reason to refactor is that we are making heavy use of RabbitMQ in our solution, and need to test the robustness of our services in regards to RabbitMQ availability. The proposed extraction of RabbitMQCtl from IntegrationFixture makes it easy for us to reuse tested code from the rabbitmq-dotnet-client repository.

If we can give this back to the community, so much the better.

RabbitMQCtl is a better name, yes. Renamed.

Copy link
Member

@michaelklishin michaelklishin 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 clarification. I see no reason not to extract this class then. @lukebakken?

@lukebakken
Copy link
Contributor

Thanks for explaining @pergh. I figured that was the reason. I'll review this today.

@michaelklishin michaelklishin changed the title Refactor: Extracted RabbitMq server controller component from IntegrationFixture Refactor: extracted RabbitMQ node management functions from IntegrationFixture Jul 1, 2020
@lukebakken lukebakken added this to the 6.2.0 milestone Jul 1, 2020
@lukebakken lukebakken merged commit df61498 into rabbitmq:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants