Support policy format change in v3.7.0 - #671 (Replaces #674)

[fatmcgav]

RabbitMQ 3.7.0 seems to have changed the ordering of fields in the policies output.

Credit to @alexizmailov for the original work in #674, I just tweaked it a bit and added some tests :)

Fixes #671

[wyardley]

This mostly makes sense to me, just curious why there's a literal .* in the heredoc in these various examples (including in the existing code)... shouldn't the result be the literal output of running rabbitmqctl list_policies?

[fatmcgav]

I believe .* can be the output from rabbitmqctl list_polices, as it's a regex pattern that can be used within the rabbitmq policy to control what resources the policy affects.
Unless I'm missing your point :)

More details here: https://www.rabbitmq.com/parameters.html#policies

[wyardley]

Got it, that makes sense then. I didn't have a rabbitmq instance handy so couldn't check the actual output, and a quick Google search failed me.

[wyardley]

Thanks for this @fatmcgav, and thanks for preserving attribution.

I'm +1 on this, with just the open question above (maybe I'm misunderstanding how the mock works here?)

My only other question is on how reliable rabbitmq_version is, and whether it might not make sense to just try both match patterns? But since the existing code seems to already have been using it, I guess it should work.
The rubocop / Puppet 5 failures obviously would need to be resolved as well.

[fatmcgav]

Yeh, I'm confused by the TravisCI failures, as those tests all pass for me locally :s

[wyardley]

@fatmcgav did you try with PUPPET_VERSION="~> 5.0"?

[wyardley]

I'm also not seeing the error locally, even with PUPPET_VERSION="~> 5.0".

I think the following should resolve the rubocop failures?

--- a/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb
+++ b/lib/puppet/provider/rabbitmq_policy/rabbitmqctl.rb
@@ -20,10 +20,12 @@ Puppet::Type.type(:rabbitmq_policy).provide(:rabbitmqctl, parent: Puppet::Provid
       # / ha-all .* all {"ha-mode":"all","ha-sync-mode":"automatic"} 0 << This is for RabbitMQ v >= 3.7.0
       if Puppet::Util::Package.versioncmp(rabbitmq_version, '3.7') >= 0
         regex = %r{^(\S+)\s+(\S+)\s+(\S+)\s+(all|exchanges|queues)?\s+(\S+)\s+(\d+)$}
-        applyto_index, pattern_index = 4, 3
+        applyto_index = 4
+        pattern_index = 3
       else
         regex = %r{^(\S+)\s+(\S+)\s+(all|exchanges|queues)?\s*(\S+)\s+(\S+)\s+(\d+)$}
-        applyto_index, pattern_index = 3, 4
+        applyto_index = 3
+        pattern_index = 4
       end
 
       policy_list.split(%r{\n}).each do |line|

[wyardley]

Proposed a possible fix in
fatmcgav#1
Alternately, you could try dropping the context statements around the version-specific bits.
I could only replicate the error locally by switching to this pattern (and then only when running in parallel):

provider_class = Puppet::Type.type(:rabbitmq_policy).provider(:rabbitmqctl)
describe provider_class do
[...]
  let(:provider) { provider_class.new(resource) }

It's basically an issue with that same instance being reused in combination with the parallel tests, I think? I think voxpupuli/puppet-nodejs@6b7acb5 was a similar problem (similar error and similar cause)

[fatmcgav]

@wyardley it looks like ure fix has got all the tests passing, so thanks for that... 😀

[wyardley]

wasn't 100% sure about this, but seems to work without it.

[fatmcgav]

Yeh, it does :)

[wyardley]

Looks good to me, but since I've got some changes in here, hoping someone else can merge. After merge, we should cut a release.

[fatmcgav]

@wyardley A new release sounds good to me :)

[bastelfreak]

Thanks @fatmcgav!