-
Notifications
You must be signed in to change notification settings - Fork 4
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(bus): implement EventBus
for RabbitMQ
#6
Conversation
Code Climate has analyzed commit 4cbb1c9 and detected 5 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 98.4% (50% is the threshold). This pull request will bring the total coverage in the repository to 99.0%. View more on Code Climate. |
EventBus
for RabbitMQ
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.
Please follow this guideline: #1 (comment)
According to BranchByAbstraction you should extract Bus
abstraction into a separate PR. The same is true for RetryStategy
, make sure that you utilize atomic PRs. while working on huge tasks. How to split huge tasks into small pieces you can find in LLD (see Proposal
).
Moreover, Bus
implementation should be to be inside a separate bus
package according to LLD.
EventBus
for RabbitMQ
EventBus
for RabbitMQ
5a5a2c3
to
3f33f03
Compare
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.
There are a lot of critical issues, oddities, and bugs.
In addition to comments, please pay attention to the following ones:
- As discussed you should use
amqp-connection-manager
to build robust and reliable transport, or implement auto-reconnect manually as it is done innexploit-cli
. See https://github.com/NeuraLegion/integration-bus - Rework tests according to comments in feat(core): provide a lightweight and simple way to configure SDK #1 (i.e. AAA pattern, proper expectations, an assertion for the whole result of execution, etc)
SdkConfig
andEventBusConfig
are separate entities. You should separate them.- Fix spelling and grammar mistakes in the test titles.
7b261c3
to
1710338
Compare
1453e5b
to
f089d60
Compare
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.
@RomanReznichenko please ensure 100% test coverage of RMQEventBus
. BTW it seems you forgot to implement HttpCommandDispatcher
(you can start working on it in a separate PR)
closes #5