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

Respect RABBITMQ_URL if it's set. #52

Merged
merged 1 commit into from Jul 6, 2014

Conversation

Projects
None yet
2 participants
@norman
Collaborator

norman commented Jun 27, 2014

This change makes Sneakers respect the RABBITMQ_URL environment variable. This is the same variable that Bunny will look for.

Since the RabbitMQ virtual host can be set in the URL, this change makes makes it look for the virtualhost there rather than defaulting to '/'.

At present in Sneakers, it's possible to set the amqp_url to a value that includes a virtual host, but unless you explicitly set the virtualhost value as well, Sneakers will still use "/". This caused me a bit of confusion at first, as I was wondering why my URL with a virtual host wasn't sufficient to set that value in Sneakers.

Note that there are no tests in place for this, since at present the DEFAULTS hash is initialized at compile-time and frozen, which means it's too late to set the ENV var in the tests. I think it would be a good idea to implement a Sneakers::Configuration class with accessor methods for the config variables Sneakers uses; I've done something similar in friendly_id, but didn't want to send you too large a pull request unless you agreed with the idea.

@norman norman changed the title from Respect RABBITMQ_URL if its set. to Respect RABBITMQ_URL if it's set. Jun 27, 2014

@jondot

This comment has been minimized.

Owner

jondot commented Jul 6, 2014

Hi Norman,
Thanks for the time working on this and sorry for the confusion :)
Even though there aren't tests - I will merge this to avoid confusion for others - thanks again.
Would solving the just-in-time initialization be possible with a simple bit of mocking?

Note - i'll merge but feel free to keep discussing here

jondot added a commit that referenced this pull request Jul 6, 2014

@jondot jondot merged commit d7a2226 into jondot:master Jul 6, 2014

@norman

This comment has been minimized.

Collaborator

norman commented Jul 7, 2014

Thanks @jondot for this and the other merges, I'm glad you find them useful. 😄 We're about to put our app using Sneakers in production - we're really grateful for all the time your library has saved us from writing boring boilerplate code over and over again. 👍

I don't think in this case mocking would be enough to make this code testable; by the time we can begin to implement our mocks, the Sneakers library will already have been loaded, and the RABBITMQ_URL would have been parsed.

The approach I'd take to solving this issue would be to create a configuration class and instantiate an instance of it at compile time. Then to test it you can create another instance of it in your tests. The code that parses the RABBITMQ_URL could go in the constructor, and set an instance variable for :vhost and all the other values. If this class implements [], []= and fetch methods, then you can continue to treat it like a Hash in the rest of the code, so it wouldn't require modification of the code elsewhere in the library.

I'll work on that today and send over a PR to see what you think.

With my coworker @deepakinseattle we also already tweaked the code in this PR to use AMQ::Settings.parse_amqp_url rather than URI to parse RABBITMQ_URL, since that handles some of the quirks in the formats RabbitMQ allows. I'll send that over too.

@norman norman deleted the flexminder:use_rabbitmq_url_as_default branch Jul 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment