ruby-amqp / bunny

Bunny is a popular, easy to use, mature Ruby client for RabbitMQ
Other
1.39k stars 303 forks source link

Support connection name #600

Closed brerx closed 4 years ago

brerx commented 4 years ago

related to https://github.com/ruby-amqp/bunny/issues/599.

brerx commented 4 years ago

@michaelklishin This is my first approach to solve this.

Without the addition in docker-entrypoint, the CLI tool proceeded before the actual users were created, which resulted in countless Auth errors during testing. So this should enable more people to contribute here.

I am eager to hear your feedback, especially whether the unit test is in the right place. I was not sure whether to put it rather into the integration/connection_spec...

michaelklishin commented 4 years ago

This does not actually populate the client property by merging it into @client_properties. So this makes it easy to access connection name in the app but not set it, which I thought was the goal?

brerx commented 4 years ago

Ah now I think I know how you want to understand it: Instead of

Bunny.new(properties: { connection_name: 'name' } }

which will merge automatically to the @client_properties, you thought of something like this:

      @client_properties   = DEFAULT_CLIENT_PROPERTIES.merge(client_props).merge(connection_name: opts[:connection_name])

=> Bunny.new(connection_name: 'name')

Am I right? I like this even better.

michaelklishin commented 4 years ago

Correct, it should be easy to set a connection name, reading it is primarily useful for tests and, perhaps, logging.

Bunny.new(properties: { connection_name: 'name' } } would still work. I'm not sure whether connection_name the option or client_properties should take precedence, your approach seems right.

brerx commented 4 years ago

Ok, I changed it to the easier usecase. What do you think?

michaelklishin commented 4 years ago

I have updated the code to support connection names specified directly in client_properties. Will add a spec example next. The rest looks good to me.

brerx commented 4 years ago

Thx for merging this so fastly. Will you make a new release soon?

michaelklishin commented 4 years ago

2.17.0 is up on rubygems.org.