When using the uri config option: Respect amqps specification and use default protocol ports if unspecified #305
+36
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaellennox
force-pushed
the
restore-uri-scheme-and-default-port-parsing
branch
2 times, most recently
from
3cc2e48
to
e83bdc7
Compare
lib/hutch/broker.rb
Outdated
| @config[:mq_vhost] = u.path.sub(/^\//, "") | ||
| @config[:mq_username] = u.user | ||
| @config[:mq_password] = u.password | ||
| end | ||
|
|
||
| def default_mq_port | ||
| @config[:mq_tls] ? 5671 : 5672 |
There was a problem hiding this comment.
Please switch to AMQ::Protocol::DEFAULT_PORT and AMQ::Protocol::TLS_PORT.
There was a problem hiding this comment.
Thanks @michaelklishin , ♻️
In ruby-amqp#159 functionality was added to hutch that meant that it would respect that when using `amqps://` as the URI scheme it would use TLS and also that when not specifying a port in the URI scheme it would default to the standard protocol ports (5671 for TLS connections, 5672 for non TLS connections). This functionality appears to have been lost since and is restored by this commit it also adds a new test case to prevent another regression.
michaellennox
force-pushed
the
restore-uri-scheme-and-default-port-parsing
branch
from
e83bdc7
to
3ad4b31
Compare
|
Are failures in the JRuby build expected? Looking at the travis output all the specs passed but travis marked the build as a failure as it believed the build timed out: https://travis-ci.org/gocardless/hutch/jobs/329386746 |
|
With JRuby you have to properly close connections or the JVM won't ever terminate because RabbitMQ Java client will have an execution service with running non-daemon threads. So it's not related. |
michaelklishin
approved these changes
Jan 16, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Restores functionality implemented in #159 and since lost to:
amqpsspecification when using the URI config option (ie. utilise TLS)This means that:
amqp://guest:guest@127.0.0.1/connects to the server on port 5672 and does not utilise TLS.amqps://guest:guest@127.0.0.1/connects to the server on port 5671 and does utilise TLS.It also adds test cases for this functionality to prevent another regression.
Where should the reviewer start?
Changes are all within
Hutch::Broker#parse_uriAny background context you want to provide?
Background context can be found in the original issue ://github.com//issues/304
Any specific implementation details?
The test for covering when amqps scheme is utilised has to stub out
Hutch::Adapterinstead of acting like an integration test in a similar manner to the surrounding specs because it appears a default rabbitmq install does not activate TLS listeners and as such a user who was to run the test suite without changing their rabbitmq server config would get failing specs if the adapter was not stubbed.\cc @michaelklishin