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

Fix to RMQ socketoptions being incorrectly passed to ampq-connection-manager #5790

Merged
merged 10 commits into from
Dec 9, 2020

Conversation

christianallred
Copy link
Contributor

@christianallred christianallred commented Nov 25, 2020

PR Type

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

RMQ microservices were not properly passing over the the heartbeatIntervalInSeconds to the amqp-connection-manager and the only way to get it work as expected was to pass in a url object with the heartbeat value in the object.

Issue Number: 5788

What is the new behavior?

I have updated the object being passed to amqp-connection-manager to match the footprint they expect so that socket options are properly accepted.

Does this PR introduce a breaking change?

[ ] Yes
[ x] No

Other information

Expected footprint for amqp-connection-manager options
https://github.com/jwalton/node-amqp-connection-manager

{
     connectionOptions: any
     hartbeatIntervalInSeconds: number
     reconnectTimeInSeconds: number
     findServers: callback
}

@coveralls
Copy link

coveralls commented Nov 25, 2020

Pull Request Test Coverage Report for Build d308d8fa-8567-479a-9c90-93bcad83dfd8

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 94.616%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-rmq.ts 0 3 0.0%
Totals Coverage Status
Change from base Build 24d3285b-41a3-4820-9784-4ec138ed78e2: -0.04%
Covered Lines: 5008
Relevant Lines: 5293

💛 - Coveralls

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

Can you add a regression test to ensure this is doing what is expected, and to help it from failing in the future?

Comment on lines 92 to 101
/**
* Expected footprint for amqp-connection-manager options
* https://github.com/jwalton/node-amqp-connection-manager
* {
* connectionOptions: any,
* heartbeatIntervalInSeconds: number,
* reconnectTimeInSeconds: number
* findServers: callback
* }
*/
Copy link
Member

Choose a reason for hiding this comment

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

While the comment is nice, it should be in the pull request or the commit message so that it's referencable without being in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR to contain this. removed comment from code. I'll work on a regression test later tonight

@christianallred
Copy link
Contributor Author

christianallred commented Nov 25, 2020

I've added the typings for this as well to help with programming a bit more. if i have time later this week i will add the typings for queueOptions as well. shouldn't be too bad. I didn't have time to write the regression tests yet. Im admittedly not the worlds best test writer :P, i'll work on getting those up as soon as i can.

@christianallred
Copy link
Contributor Author

I added the rest of the typings and it caused Integration tests to fail so must be another bug somewhere :) I'll be trying to get tests to run locally tonight or tomorrow i was having trouble getting them running yesterday

@kamilmysliwiec
Copy link
Member

Can you please rebase your PR (or cherry-pick your changes)? This PR contains a few unnecessary commits (and changes) unrelated to the actual fix

@christianallred
Copy link
Contributor Author

christianallred commented Dec 8, 2020

Can you please rebase your PR (or cherry-pick your changes)? This PR contains a few unnecessary commits (and changes) unrelated to the actual fix

Done. Sorry i haven't had time to add the tests. been SUPER busy lately with some real life stuffs. I may have lost some integration updates when i rebased. i'll double check it.

@kamilmysliwiec
Copy link
Member

Thanks!

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.

4 participants