Fix rabbit version discovery with newer RabbitMQ

[thomasgoirand]

The output of "rabbitmqctl -q status" has changed, and the version
detection is now broken in this module. As a consequence, the puppet
providers are all broken because they aren't using the much needed
--no-table-headers option for rabbitmqctl.

This patch fix this problem. It's been tested in Debian unstable
and Bullseye, which includes rabbitmq-server 3.8.9.

[wyardley]

could this be simplified into a single regex that matches both the old / new use cases?

[wyardley]

also shouldn't
RabbitMQ.*?([\d\.]+)}) already match RabbitMQ version: ([\d\.]+)?

probably need new unit test(s) for this as well?

[wyardley]

my suggestion is to start with a new test in https://github.com/voxpupuli/puppet-rabbitmq/blob/master/spec/unit/puppet/provider/rabbitmq_cli_spec.rb

[wyardley]

also, can you double-check if your git commit email address is tied to your GitHub account?

[wyardley]

Can you see if #874 (merged, but not yet released) resolves this for you?

[thomasgoirand]

Hi. This last version of my patch correctly works with both Debian Buster (rabbitmq 3.7.8) and Bullseye (rabbitmq 3.8.9). Please consider it.

[wyardley]

Hi. This last version of my patch correctly works with both Debian Buster (rabbitmq 3.7.8) and Bullseye (rabbitmq 3.8.9). Please consider it.

At a minimum, we still need a unit test for the new output format. Can you also show how the old and new output of the command looks? Hard to say without seeing it, but I kind of feel like a simpler regex may be able to work here.

Can you confirm that #874 doesn’t fix the issue on its own?

[vox-pupuli-tasks]

Dear @thomasgoirand, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[thomasgoirand]

Gosh, this is frustrating...

With my position, which is packaging all of OpenStack in Debian, and many other stuff like RabbitMQ, OpenVSwitch and many others, and writing an OpenStack cluster management software, I do see many issues in so many software. I do believe it's my duty to report and fix things as I see them. So I do it. I do expect upstream author to take over the hint I'm giving, as a good advice, so we improve things together.

But instead, very often, upstream authors asks me to spend even more time on their particular piece of program, like investing some of time to write unit tests and so on. I find it goes beyond the scope of what I can do, because I'm kind of nearly alone doing all of what I describe above, and my days only have 24 hours...

So please take over the patch please, and add unit tests yourself. if that helps you better to maintain puppet-rabbitmq. I would warmly welcome this. Though leaving this merge request bitrot, eventually to the point that it can't merge, because I haven't fulfilled your requirement, is IMO the worst thing to do, especially in a case like this one where there's really an issue with the latest version of RabbitMQ that my patch is fixing.

If you find you don't have enough time yourself to either test, or add unit tests, then why not just merge in the current state of the patch?

Please do solve the conflict and merge this ... (or close this merge request if there's a better patch).

Cheers,

Thomas Goirand (zigo)

[wyardley]

Hi --

@thomasgoirand I understand your frustration, and I'm sorry.

That said, this module was originally written by Puppet, is no longer maintained by them, and is not really actively maintained right now. I'm not the "author" of the module, and I haven't used RabbitMQ (or Puppet, for that matter) myself in a number of years now. I try to help out where I can with this module on a best-effort basis just because no one else has taken over, and without examples of the new output format, it would take quite a bit of time to get things setup to properly test / reproduce this myself. The module itself defaults to, and currently is only tested against, older versions of RabbitMQ (https://github.com/voxpupuli/puppet-rabbitmq/#module-description mentions 3.5.x and 3.6.x specifically).

Not sure why this uses rabbitmqctl status vs rabbitmq version, but not going to try to mess with that here for now.

There are a number of things that probably should happen in this module, including changing the default back to installing from the RabbitMQ repo again, and supporting newer versions; migrating to direct API calls instead of the CLI for the types / providers... but without someone with the time / effort to devote to this, I don't know when it will happen.

As far as having unit tests related to the change, that's generally Vox's policy on things, so something you can take up with the collaborators more broadly (or see if there's someone within Vox who'd like to take a more active role in working with / maintaining this module) -- I do personally find that, despite some work, especially in terms of getting things setup to run them locally initially, it not only makes things easier to test, but also prevents someone from accidentally making a change later that might cause a regression, and also kind of implicitly documents what kinds of strings the regex is looking for.

In this case, there are already examples, so, probably, adding the test should be something along the lines of adding a new example along with:

puppet-rabbitmq/spec/unit/puppet/provider/rabbitmq_cli_spec.rb

Lines 15 to 25 in 1e8c72e

	it 'gets the RabbitMQ version' do
	provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '{rabbit,"RabbitMQ","3.1.5"}'
	expect(provider_class.rabbitmq_version).to eq('3.1.5')
	end
	it 'caches the RabbitMQ version' do
	provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '{rabbit,"RabbitMQ","3.7.10"}'
	v1 = provider_class.rabbitmq_version
	# No second expects for rabbitmqctl as it shouldn't be called again
	expect(provider_class.rabbitmq_version).to eq(v1)

Creating an issue (with some debug output and details like the version you're currently using) would be one option if you don't have time to add a test that reproduces your problem. When this came up initially, I didn't have anything handy to quickly look up the format, and you didn't include that in your initial report; however, I can verify the format pretty easily using their official Docker image:

(3.7, version in the test above):
[{rabbit,"RabbitMQ","3.7.28"},

(3.8)
RabbitMQ version: 3.8.34

(3.10; latest):
RabbitMQ version: 3.10.6

Based on that, I tried adding a unit test for this locally, and, while this is a little simplified as we're not stubbing the entire output, as best I can tell, the current regex should already work with the version as reported by rmq >= 3.8. (I did try adding the previous line etc. in the stubbed value, and tests still passed). If there's something I'm missing about why the existing regex doesn't grok the value, let me know - we can try to create a test that will make it easier to catch this.

diff --git a/spec/unit/puppet/provider/rabbitmq_cli_spec.rb b/spec/unit/puppet/provider/rabbitmq_cli_spec.rb
index d710a40..c4bfa13 100644
--- a/spec/unit/puppet/provider/rabbitmq_cli_spec.rb
+++ b/spec/unit/puppet/provider/rabbitmq_cli_spec.rb
@@ -13,18 +13,23 @@ describe provider_class do
   end
 
   it 'gets the RabbitMQ version' do
-    provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '{rabbit,"RabbitMQ","3.1.5"}'
-    expect(provider_class.rabbitmq_version).to eq('3.1.5')
+    provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '     [{rabbit,"RabbitMQ","3.7.28"},'
+    expect(provider_class.rabbitmq_version).to eq('3.7.28')
   end
 
   it 'caches the RabbitMQ version' do
-    provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '{rabbit,"RabbitMQ","3.7.10"}'
+    provider_class.expects(:rabbitmqctl).with('-q', 'status').returns '     [{rabbit,"RabbitMQ","3.7.28"},'
     v1 = provider_class.rabbitmq_version
 
     # No second expects for rabbitmqctl as it shouldn't be called again
     expect(provider_class.rabbitmq_version).to eq(v1)
   end
 
+  it 'gets the RabbitMQ version with version >= 3.8' do
+    provider_class.expects(:rabbitmqctl).with('-q', 'status').returns 'RabbitMQ version: 3.10.6'
+    expect(provider_class.rabbitmq_version).to eq('3.10.6')
+  end
+
   it 'uses correct list options with RabbitMQ < 3.7.9' do
     provider_class.expects(:rabbitmq_version).returns '3.7.8'
     provider_class.expects(:rabbitmqctl).with('list_vhost', '-q').returns ''

[wyardley]

[wyardley]

Looks like #814 (from back in 2019) resolved this - is it possible you were running an older version that didn't have that fix?

[wyardley]

Please file an issue if you can reproduce this issue with a recent version, and include version / platform information, and debug output.