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

Add global parameter mqtt-vhost mapping functions #429

Conversation

awills96
Copy link

For this contribution, I followed the current setup of making the templated get function for global parameters that mirrors the vhost parameter setup. On top of that, also implemented the use of global parameter for port to virtual host mapping per specified by the rabbitmq mqtt plugin: https://www.rabbitmq.com/mqtt.html

Not sure if there are any other global parameters that are somewhat common that should be included.

- Add generic global parameter call to allow for follow on global
parameter getting functions to be implemented.
- Demonstate global parameter, setting, and deleting with mqtt vhost
  mapping
- Testing in Reactor Netty and Client specs
Correct tests in Reactor and Client spec to say testing with a blank vhost value
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. This looks reasonable to me and even has integration tests.

I'd like to rename a few methods because if we don't do it now, it will be much harder to rename them once this version of the client API ships in a release.

src/main/java/com/rabbitmq/http/client/Client.java Outdated Show resolved Hide resolved
src/main/java/com/rabbitmq/http/client/Client.java Outdated Show resolved Hide resolved
src/main/java/com/rabbitmq/http/client/Client.java Outdated Show resolved Hide resolved
- Also rename mirror functions in Reactor Netty Client
@michaelklishin
Copy link
Member

Let's wait for @acogoluegnes' feedback, this LGTM now.

Copy link
Contributor

@acogoluegnes acogoluegnes left a comment

Choose a reason for hiding this comment

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

LGTM.

@acogoluegnes acogoluegnes added this to the 5.3.0 milestone Jan 17, 2024
@acogoluegnes acogoluegnes merged commit 47bb6b9 into rabbitmq:main Jan 17, 2024
2 checks passed
@acogoluegnes
Copy link
Contributor

Thanks!

@awills96 Do we still need #426 or can we close it?

@awills96
Copy link
Author

@acogoluegnes Good to close!

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.

4 participants