Allow configuration of exchange options #170

Merged
merged 2 commits into from Oct 17, 2015

Conversation

Projects
None yet
4 participants
@dkastner
Contributor

dkastner commented Sep 2, 2015

No description provided.

@@ -13,6 +13,7 @@ def self.initialize(params={})
mq_host: 'localhost',
mq_port: 5672,
mq_exchange: 'hutch', # TODO: should this be required?
+ mq_exchange_options: {},

This comment has been minimized.

@rpond-pa

rpond-pa Sep 2, 2015

Some unsolicited advice, I would make this default to { durable: true} instead of blank.

That way, anyone who upgrades won't get any big surprises, as in, their queues aren't durable anymore.

@rpond-pa

rpond-pa Sep 2, 2015

Some unsolicited advice, I would make this default to { durable: true} instead of blank.

That way, anyone who upgrades won't get any big surprises, as in, their queues aren't durable anymore.

This comment has been minimized.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

Agreed.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

Agreed.

This comment has been minimized.

@teodor-pripoae

teodor-pripoae Sep 2, 2015

Contributor

No need for this since you are already doing the hash merge thing in broker.rb above.

@teodor-pripoae

teodor-pripoae Sep 2, 2015

Contributor

No need for this since you are already doing the hash merge thing in broker.rb above.

This comment has been minimized.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

It makes more sense to me to move new default into the config.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

It makes more sense to me to move new default into the config.

This comment has been minimized.

@dkastner

dkastner Sep 3, 2015

Contributor

I thought about that but then there might be people who directly set Hutch::Config.set :mq_exchange_options, custom: :thing and then mistakenly lose the default durable option

@dkastner

dkastner Sep 3, 2015

Contributor

I thought about that but then there might be people who directly set Hutch::Config.set :mq_exchange_options, custom: :thing and then mistakenly lose the default durable option

This comment has been minimized.

@dkastner

dkastner Sep 3, 2015

Contributor

Also it's a pain to have to remember to re-set what is a default option

@dkastner

dkastner Sep 3, 2015

Contributor

Also it's a pain to have to remember to re-set what is a default option

This comment has been minimized.

@teodor-pripoae

teodor-pripoae Sep 3, 2015

Contributor

Since this is a new functionality I think we can put a warning comment near this option. If somebody looks to find which is the required configuration key for this, will see that default is durable and also will see the comment.

@teodor-pripoae

teodor-pripoae Sep 3, 2015

Contributor

Since this is a new functionality I think we can put a warning comment near this option. If somebody looks to find which is the required configuration key for this, will see that default is durable and also will see the comment.

lib/hutch/broker.rb
@@ -52,10 +52,12 @@ def set_up_amqp_connection
open_channel!
exchange_name = @config[:mq_exchange]
+ default_options = { durable: true }

This comment has been minimized.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

This can be inlined.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

This can be inlined.

lib/hutch/broker.rb
@@ -52,10 +52,12 @@ def set_up_amqp_connection
open_channel!
exchange_name = @config[:mq_exchange]
+ default_options = { durable: true }
+ exchange_options = default_options.merge @config[:mq_exchange_options]

This comment has been minimized.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

Please use parentheses around method arguments.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

Please use parentheses around method arguments.

This comment has been minimized.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

I think this entire line can be inlined, too.

@michaelklishin

michaelklishin Sep 2, 2015

Collaborator

I think this entire line can be inlined, too.

@michaelklishin michaelklishin self-assigned this Sep 2, 2015

michaelklishin added a commit that referenced this pull request Oct 17, 2015

Merge pull request #170 from dkastner/exchange-options
Allow configuration of exchange options

@michaelklishin michaelklishin merged commit d4dba95 into gocardless:master Oct 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Oct 17, 2015

Collaborator

Thank you. Sorry that has gone under my radar.

Collaborator

michaelklishin commented Oct 17, 2015

Thank you. Sorry that has gone under my radar.

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