Support multiple routing keys for bindings using separate parameters #504

wyardley · 2016-09-01T19:01:20Z

Updated: proposing this as an option instead of #375
This PR lets you specify legacy style resources (source@destination@vhost) and / or lets you specify source, destination (and, optionally, routing_key and vhost) as separate parameters, with a descriptive or arbitrary name.

There's still probably some room for improvement, but I've finally got this in shape where I think it could be merged.

hunner · 2016-12-01T18:00:37Z

lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb

            :destination_type => destination_type,
            :routing_key      => routing_key,
            :arguments        => JSON.parse(arguments),
            :ensure           => :present,
-            :name             => "%s@%s@%s" % [source_name, destination_name, vhost],


This should be "%s@%s@%s@%s" % [source_name, destination_name, vhost, routing_key],

In retrospect, perhaps this should just be a hash of these values to remain unique but not duplicate the information that is available in the properties when printed by puppet resource rabbitmq_binding

@hunner so, like this?

hashed_name = Digest::SHA256.hexdigest "%s@%s@%s@%s" % [source_name, destination_name, vhost, routing_key unless(source_name.empty?) binding = { :source => source_name, [...] :name => hashed_name, }

hunner · 2016-12-01T18:01:05Z

lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb

            :destination_type => destination_type,
            :routing_key      => routing_key,
            :arguments        => JSON.parse(arguments),
            :ensure           => :present,
-            :name             => "%s@%s@%s" % [source_name, destination_name, vhost],
          }
          resources << new(binding) if binding[:name]


This needs fixing

hunner · 2016-12-01T18:01:58Z

lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb

            :destination_type => destination_type,
            :routing_key      => routing_key,
            :arguments        => JSON.parse(arguments),
            :ensure           => :present,
-            :name             => "%s@%s@%s" % [source_name, destination_name, vhost],
          }
          resources << new(binding) if binding[:name]
        end


Prefetch should match on all 4 properties, not on .name like

if provider = bindings.find { |route| route.source == source && route.dest == dest && ... }

hunner · 2016-12-01T18:03:33Z

lib/puppet/type/rabbitmq_binding.rb

+  newparam(:vhost, :namevar => true) do
+    desc 'vhost'
+    newvalues(/^\S+$/)
+    defaultto('/')


Why does a required (because namevar) property have a defaultto? Maybe not needed?

Comment: keep the default because when explicitly specifying source/dest/routing_key and not using title_pattern, then it's good to have a default.

fatmcgav · 2017-07-14T16:01:00Z

Any progress on this one?

wyardley · 2017-08-15T23:05:45Z

@fatmcgav Sorry for the delayed response, and also to not having updated this PR in forever. I got some good pointers from @hunner, @bmjen, and @DavidS, but honestly haven't had the energy to jump back into this so far, hopefully soon. I still do have the use case (though would be just as happy if someone else wanted to take a stab at it).

I've rebased against current master, made some of the suggested changes, and am taking a quick look to see if it works (some of the existing tests for 'name' patterns would need to be dropped, but perhaps there should be some additional tests added).

I've made most of the suggestions @hunner had in the review, but I'm still not getting it to work, whether or not I try to do it in a backwards-compatible way or not, so I think there are still some bits of the ever-fun title_patterns / composite namevars that are eluding me. I know some people have said composite namevars are not necessary, but I think maybe they are?

I briefly got it to work for one use case in Vagrant, but right now I'm getting stuck on:

Error: Failed to apply catalog: undefined method `source' for (provider=rabbitmqadmin):Puppet::Type::Rabbitmq_binding::ProviderRabbitmqadmin

which seems to be similar to what's described in
https://projects.puppetlabs.com/issues/15046
(so maybe title_patterns is somehow not matching)?

These are my test cases:

      rabbitmq_binding { 'exchange1@queue1@host1':
        user             => 'dan',
        password         => 'bar',
        destination_type => 'queue',
        routing_key      => '#',
        ensure           => present,
      }

      rabbitmq_binding { 'test1':
        source           => 'exchange1',
        dest             => 'queue1',
        vhost            => 'host1',
        user             => 'dan',
        password         => 'bar',
        destination_type => 'queue',
        routing_key      => 'test3',
        ensure           => present,
      }

      rabbitmq_binding { 'test2':
        source           => 'exchange1',
        dest             => 'queue1',
        vhost            => 'host1',
        user             => 'dan',
        password         => 'bar',
        destination_type => 'queue',
        routing_key      => 'test4',
        ensure           => present,
      }

DavidS · 2017-08-16T13:54:59Z

The error is coming (presumably) from https://github.com/voxpupuli/puppet-rabbitmq/pull/504/files#diff-6848b732a3c79af87de0ad6905d8387eR73 and I don't think that can work like this. Print out what the name is there, maybe that gives you a clue on how to build that matching there.

It's been a long time since I played around with that part.

fatmcgav · 2017-08-16T16:42:06Z

OK, so I've been able to re-produce the failure in a unit test.
See fatmcgav@1dfa47b which results in this failure:

Failures:

  1) Puppet::Type::Rabbitmq_binding::ProviderRabbitmqadmin#prefetch matches
     Failure/Error: if provider = bindings.find{ |route| route.source == source && route.dest == dest && route.vhost == vhost && route.routing_key == routing_key }

     NoMethodError:
       undefined method `source' for (provider=rabbitmqadmin):Puppet::Type::Rabbitmq_binding::ProviderRabbitmqadmin
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:73:in `block (2 levels) in prefetch'
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:73:in `each'
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:73:in `find'
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:73:in `block in prefetch'
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:72:in `each'
     # ./lib/puppet/provider/rabbitmq_binding/rabbitmqadmin.rb:72:in `prefetch'
     # ./spec/unit/puppet/provider/rabbitmq_binding/rabbitmqadmin_spec.rb:113:in `block (3 levels) in <top (required)>'

Deprecation Warnings:

Using `should` from rspec-expectations' old `:should` syntax without explicitly enabling the syntax is deprecated. Use the new `:expect` syntax or explicitly enable `:should` with `config.expect_with(:rspec) { |c| c.syntax = :should }` instead. Called from /Users/gavinw/Puppet/modules/puppetlabs/puppetlabs-rabbitmq/spec/unit/puppet/provider/rabbitmq_binding/rabbitmqadmin_spec.rb:29:in `block (3 levels) in <top (required)>'.


If you need more of the backtrace for any of these deprecations to
identify where to make the necessary changes, you can configure
`config.raise_errors_for_deprecations!`, and it will turn the
deprecation warnings into errors, giving you the full backtrace.

1 deprecation warning total

Finished in 0.02138 seconds (files took 0.95047 seconds to load)
8 examples, 1 failure

Failed examples:

rspec ./spec/unit/puppet/provider/rabbitmq_binding/rabbitmqadmin_spec.rb:93 # Puppet::Type::Rabbitmq_binding::ProviderRabbitmqadmin#prefetch matches

That makes it a bit easier to debug... I'll see if can spot the cause later...

wyardley · 2017-08-16T16:56:42Z

@fatmcgav @DavidS thanks for the help, was reading up on debugging types / providers yesterday, but these will both help a lot. Thanks also for creating a unit test @fatmcgav -- also, I'm going to merge your tests to my branch (with attribution), hope that's Ok?

wyardley · 2017-08-16T20:57:49Z

@hunner @DavidS if there's time, can you discuss this one in the meeting?
I think my updated version (thanks to some big help from @fatmcgav) is functional.

Probably should add some additional acceptance tests, but this is getting a lot closer.

wyardley · 2017-08-16T21:42:40Z

I am curious if there's a way to validate whether we have a valid source / destination / vhost / routing_key combination in total, between defaults and splitting up the name in the legacy case, but not sure what the cleanest way to do that validation would be.

hunner · 2017-08-17T17:32:47Z

lib/puppet/type/rabbitmq_binding.rb

+    isnamevar
+  end
+
+  newparam(:source) do


source/destination/vhost/routing_key should be properties because they can be read from & written to the system's configuration. This will also allow them to be printed when running puppet resource rabbitmq_binding

Also the global validate do block can check raise ArgumentError, "source is required" if ! self[:source] and ! self.provider.source etc. to ensure that the values are actually passed.

I updated those, as well as a couple other things that are read from / written to the config to be properties, let me know how that looks. Will work on validation.

I did now have to update title_patterns to match :name where I didn't before, not sure why that is. With the a@b@c style naming, I got a name error

[root@centos-7-x64 tmp]# puppet apply apply_manifest.pp.1fJLTF [...] Notice: /Stage[main]/Main/Rabbitmq_exchange[exchange1@host1]/ensure: created Notice: /Stage[main]/Main/Rabbitmq_binding[binding 1]/ensure: created Notice: /Stage[main]/Main/Rabbitmq_binding[binding 2]/ensure: created Notice: Applied catalog in 4.25 seconds [root@centos-7-x64 tmp]# puppet resource rabbitmq_binding rabbitmq_binding { '3716cc442c568f6be3772365f29af0ca786ea852b527e3c564dcb3cd83f928a9': ensure => 'present', destination => 'queue1', destination_type => 'queue', routing_key => 'test2', source => 'exchange1', vhost => 'host1', } rabbitmq_binding { 'b0f96dde732ed9be309bb466b680faa49117a151716a524eceaeb91080a34d64': ensure => 'present', destination => 'queue1', destination_type => 'queue', routing_key => 'test1', source => 'exchange1', vhost => 'host1', }

wyardley · 2017-08-17T17:39:12Z

Other questions: should routing_key default to #, and does it need to be defined? Can it / should it be an empty string if it's not used, and would this mess up all our logic?

…ferent routing key (MODULES-3679)

wyardley · 2017-08-17T18:36:26Z

ps - I took out the default for routing_key, which I think is closer to the existing behavior.

fatmcgav · 2017-08-17T19:45:49Z

lib/puppet/type/rabbitmq_binding.rb

+    desc 'destination of binding'
+
+    newvalues(/^\S+$/)
+    isnamevar


Is this needed aswell as the :namevar => true?

Good point, they're redundant. Removed.

wyardley · 2017-08-17T20:57:19Z

~~TODO: global validate do block~~
Validate block added.

  validate do
    unless self[:source] and self[:destination]
      raise ArgumentError, "Source and destination must both be defined."
    end
  end

Interestingly, routing_key matches \S* rather than \S+, and it seems like an empty string is tolerated happily enough (and works). I have not tested some corner cases here, but I'm guessing it will just compare that empty string happily enough.

wyardley · 2017-08-17T23:12:17Z

@hunner @fatmcgav I think this is good to go maybe!?

Technically, the user / password are managed by Puppet but not this resource, so am I right in thinking they should be params not properties? A lot of the things that should have been properties were params before, here's how I have it now:

param name
property source
property destination
property vhost
property routing_key
property destination_type
property arguments
param user
param password

Multiple routing keys for bindings using namevars

…ake2 Multiple routing keys for bindings using namevars

wyardley mentioned this pull request Sep 1, 2016

BREAKING: manage_repos is now repos_ensure (default false), version is now unused, switch to RabbitMQ's "packagecloud" repos #493

Merged

wyardley changed the title ~~Multiple routing keys for bindings with composite namevars~~ [WIP] Multiple routing keys for bindings with composite namevars Oct 7, 2016

hunner reviewed Dec 1, 2016

View reviewed changes

wyardley changed the title ~~[WIP] Multiple routing keys for bindings with composite namevars~~ [WIP] Multiple routing keys for bindings using namevars Jan 18, 2017

wyardley mentioned this pull request Jan 18, 2017

Rabbitmq bindings #375

Closed

wyardley force-pushed the feature-modules-3679-take2 branch 6 times, most recently from 1d88136 to eb81ba8 Compare August 15, 2017 23:00

wyardley force-pushed the feature-modules-3679-take2 branch from eb81ba8 to b62e944 Compare August 15, 2017 23:44

wyardley force-pushed the feature-modules-3679-take2 branch 6 times, most recently from f647763 to 0272df3 Compare August 16, 2017 20:55

wyardley force-pushed the feature-modules-3679-take2 branch from 0272df3 to b799846 Compare August 16, 2017 21:09

wyardley changed the title ~~[WIP] Multiple routing keys for bindings using namevars~~ Multiple routing keys for bindings using namevars Aug 16, 2017

wyardley force-pushed the feature-modules-3679-take2 branch from b799846 to 4f55649 Compare August 16, 2017 21:26

wyardley force-pushed the feature-modules-3679-take2 branch 2 times, most recently from 498e3eb to 4f55649 Compare August 17, 2017 06:46

wyardley force-pushed the feature-modules-3679-take2 branch from 4f55649 to 5cd02e1 Compare August 17, 2017 06:50

hunner reviewed Aug 17, 2017

View reviewed changes

Add support for multiple bindings with same source/dest/vhost but dif…

c674065

…ferent routing key (MODULES-3679)

wyardley force-pushed the feature-modules-3679-take2 branch 2 times, most recently from aa186f2 to b405be7 Compare August 17, 2017 18:34

fatmcgav reviewed Aug 17, 2017

View reviewed changes

wyardley force-pushed the feature-modules-3679-take2 branch 2 times, most recently from f4b0cd0 to 2e00a19 Compare August 17, 2017 20:56

wyardley force-pushed the feature-modules-3679-take2 branch from 2e00a19 to 2b7e726 Compare August 17, 2017 23:01

MatMoore mentioned this pull request Aug 21, 2017

Update requeue task to requeue by document type alphagov/publishing-api#1009

Merged

Increase test coverage

9897c01

wyardley force-pushed the feature-modules-3679-take2 branch from 2b7e726 to 9897c01 Compare August 24, 2017 17:16

hunner merged commit 9897c01 into voxpupuli:master Aug 24, 2017

hunner added a commit that referenced this pull request Aug 24, 2017

Merge pull request #504 from wyardley/feature-modules-3679-take2

eb5a614

Multiple routing keys for bindings using namevars

wyardley added the enhancement New feature or request label Sep 5, 2017

wyardley changed the title ~~Multiple routing keys for bindings using namevars~~ Support multiple routing keys for bindings using separate parameters Sep 5, 2017

wyardley deleted the feature-modules-3679-take2 branch September 6, 2017 04:22

wyardley mentioned this pull request Oct 13, 2017

Error on using puppet resource rabbitmq_binding #650

Closed

wyardley mentioned this pull request Dec 4, 2017

Still can't put multiple routing keys on the same exchange #670

Closed

Slm0n87 pushed a commit to Slm0n87/puppet-rabbitmq that referenced this pull request Mar 7, 2019

Merge pull request voxpupuli#504 from wyardley/feature-modules-3679-t…

3a0eef8

…ake2 Multiple routing keys for bindings using namevars

cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021

Merge pull request voxpupuli#504 from wyardley/feature-modules-3679-t…

4415648

…ake2 Multiple routing keys for bindings using namevars

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple routing keys for bindings using separate parameters #504

Support multiple routing keys for bindings using separate parameters #504

wyardley commented Sep 1, 2016 •

edited

hunner Dec 1, 2016

hunner Aug 17, 2017

hunner Aug 17, 2017

wyardley Aug 17, 2017 •

edited

hunner Dec 1, 2016

hunner Dec 1, 2016

hunner Dec 1, 2016

hunner Dec 1, 2016

fatmcgav commented Jul 14, 2017

wyardley commented Aug 15, 2017 •

edited

DavidS commented Aug 16, 2017

fatmcgav commented Aug 16, 2017 •

edited

wyardley commented Aug 16, 2017

wyardley commented Aug 16, 2017

wyardley commented Aug 16, 2017

hunner Aug 17, 2017

hunner Aug 17, 2017

wyardley Aug 17, 2017

wyardley Aug 17, 2017

wyardley Aug 17, 2017

wyardley commented Aug 17, 2017

wyardley commented Aug 17, 2017

fatmcgav Aug 17, 2017

wyardley Aug 17, 2017 •

edited

wyardley commented Aug 17, 2017 •

edited

wyardley commented Aug 17, 2017 •

edited

Support multiple routing keys for bindings using separate parameters #504

Support multiple routing keys for bindings using separate parameters #504

Conversation

wyardley commented Sep 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley Aug 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatmcgav commented Jul 14, 2017

wyardley commented Aug 15, 2017 • edited

DavidS commented Aug 16, 2017

fatmcgav commented Aug 16, 2017 • edited

wyardley commented Aug 16, 2017

wyardley commented Aug 16, 2017

wyardley commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley commented Aug 17, 2017

wyardley commented Aug 17, 2017

Choose a reason for hiding this comment

wyardley Aug 17, 2017 • edited

Choose a reason for hiding this comment

wyardley commented Aug 17, 2017 • edited

wyardley commented Aug 17, 2017 • edited

wyardley commented Sep 1, 2016 •

edited

wyardley Aug 17, 2017 •

edited

wyardley commented Aug 15, 2017 •

edited

fatmcgav commented Aug 16, 2017 •

edited

wyardley Aug 17, 2017 •

edited

wyardley commented Aug 17, 2017 •

edited

wyardley commented Aug 17, 2017 •

edited