-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: add RabbitMQ bridge #10534
feat: add RabbitMQ bridge #10534
Conversation
Setting the PR to draft as I have not made the change log entry yet. |
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/test/emqx_bridge_rabbitmq_connector_SUITE.erl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for very good comments. I have addressed most in a new commit and have added questions about the remaining ones.
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/test/emqx_bridge_rabbitmq_connector_SUITE.erl
Outdated
Show resolved
Hide resolved
c019ca8
to
62a9cfc
Compare
62a9cfc
to
973f937
Compare
Rebased and force pushed to remove merge conflicts with master. |
1fbea5e
to
aee62e9
Compare
apps/emqx_bridge_rabbitmq/test/emqx_bridge_rabbitmq_connector_SUITE.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
apps/emqx_bridge_rabbitmq/src/emqx_bridge_rabbitmq_connector.erl
Outdated
Show resolved
Hide resolved
false | ||
end, | ||
true = lists:any(IsRightName, Bridges), | ||
delete_bridge(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is always called, perhaps we could add this to end_per_testcase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In two test cases (t_make_delete_bridge and t_make_delete_bridge_non_existing_server) it is not called last (as these test cases checks that the delete had the desired effect) and I think there is some advantage to have it directly in the test cases as it makes it more explicit what the test case is doing. I have no strong feelings about this so I can make the change, if you insist that it should be in end_per_testcase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's fine for those two that explicitly test that behavior.
But we always want to clear all bridges during test teardown to avoid flakiness, so I think it's a good practice to do that even if there are no bridges left to delete.
1949f6d
to
cf8af6f
Compare
false | ||
end, | ||
true = lists:any(IsRightName, Bridges), | ||
delete_bridge(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's fine for those two that explicitly test that behavior.
But we always want to clear all bridges during test teardown to avoid flakiness, so I think it's a good practice to do that even if there are no bridges left to delete.
cf8af6f
to
6f9bf2d
Compare
6f9bf2d
to
065bfc9
Compare
065bfc9
to
9f0cab9
Compare
9f0cab9
to
70cf153
Compare
Fixes https://emqx.atlassian.net/browse/EMQX-9087
Summary
🤖 Generated by Copilot at 3c0677b
This pull request adds a new feature to EMQX that allows it to communicate with RabbitMQ brokers using the bridge mechanism. It introduces a new application
emqx_bridge_rabbitmq
that implements the logic and configuration for the RabbitMQ bridge type. It also updates the existing modules and files to support the new feature, such asemqx_ee_bridge
,emqx_resource
,mix.exs
, andrebar.config.erl
. It provides documentation, internationalization, and common test cases for the RabbitMQ bridge feature. It also adds a docker-compose service for RabbitMQ to facilitate testing.PR Checklist
Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:
changes/{ce,ee}/(feat|perf|fix)-<PR-id>.en.md
files