search

Foo-Foo-MQ / foo-foo-mq

Abstractions around RabbitMQ
MIT License
48 stars 24 forks source link

Fix port resolving on amqps:// protocol #27

Closed stefanosala closed 3 years ago

stefanosala commented 3 years ago

As suggested by Cloudamqp support person, foo-foo-mq should correctly auto-resolve the ssl port to 5671 instead of the standard 5672. This replicate the same behavior as amqplib: https://github.com/squaremo/amqp.node/blob/e3e10167d3f498f632a5a50dc7fac62b314400c8/lib/connect.js#L109

zlintz commented 3 years ago

@stefanosala Thanks for the PR and for updating the tests

The only thing I would ask is that you update the commit message to follow our commit style guide https://github.com/Foo-Foo-MQ/foo-foo-mq/blob/main/HOW_TO_CONTRIBUTE.md#commit-style with this as a BREAKING CHANGE: given the default port for useSSL is changing

zlintz commented 3 years ago

@auroq any additional questions or concerns?

stefanosala commented 3 years ago

@zlintz is this good? :)

zlintz commented 3 years ago

I believe so

auroq commented 3 years ago

Looks good to me. Calling it a breaking change is a good idea since it modifies a default.

auroq commented 3 years ago

Merged. Thank you for the contribution @stefanosala!

nomanyaqubny-zz commented 3 years ago

Looks like this commit has introduced a regression by removing the 'protocol' property from connection parameters. If you don't have 'useSSL' set to true it defaults to 'ampq' even if you have 'port' set to '5671'

old this.protocol = getOption(parameters, 'RABBIT_PROTOCOL') || getOption(parameters, 'protocol', 'amqp://');

new this.protocol = getOption(parameters, 'RABBIT_PROTOCOL') || (useSSL ? 'amqps://' : 'amqp://');