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

Be able to provide full connection url instead of separate :amqp and :vhost #17

Closed
graf opened this issue Jan 29, 2014 · 5 comments
Closed
Milestone

Comments

@graf
Copy link

graf commented Jan 29, 2014

I've faced quite odd behaviour of Sneakers when I've deployed application on Heroku.

Heroku's RabbitMQ Addon provides full amqp url like amqp://login:pass@host:port/vhost, and pure Bunny do understand this string easily. However, due to https://github.com/jondot/sneakers/blob/master/lib/sneakers/queue.rb#L19 I have to parse this line, extract vhost and provide it for Sneakers manually.

Here my suggestion.
Bunny.new method supports either connection_string or hash with options, so it would be nice to leverage this feature and pass Sneakers::Config as it is. So, everyone who likes to pass :ampq and :vhost separately would be able to do that, on the other hand it would be easy to deploy on Heroku platform.

@jondot
Copy link
Owner

jondot commented Jan 30, 2014

Thanks. I think it stemmed from the fact that initially we used an older api of bunny. Will look into it.

@jondot
Copy link
Owner

jondot commented Jan 30, 2014

So what i'm thinking is to drop :vhost entirely, and then people would configure via amqp url. However it will break running deployments of Sneakers. Although we can break API from time to time (externalized with proper versioning), I always want to minimize.

I was thinking in the direction of a new :amqp_url parameter, once given it will ignore all other parameters.

Then perhaps bigger steps in API will converge into one :amqp parameter in the future.

What do you think?

@SebastianEdwards
Copy link
Contributor

Just a quick note, a naked call to Bunny.new will automatically pull from $RABBITMQ_URL. Perhaps just not passing options to Bunny which haven't been explicitly set by the user will be enough?

@jondot
Copy link
Owner

jondot commented Jan 31, 2014

Yup @SebastianEdwards is also right. let's see how the discussion converges, and then understand how to put an elegant non-breaking solution in place.

@graf
Copy link
Author

graf commented Feb 1, 2014

So, @jondot, @SebastianEdwards, I think both variants are feasible. Nevertheless, my vote for "one :amqp parameter in the future", but without any suggestions about default value, because Bunny already does that under the hood and it respects $RABBITMQ_URL if the last one is presented.

@jondot jondot added this to the 0.1.0 milestone Feb 8, 2014
@jondot jondot closed this as completed Jun 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants