Fix indentation for cluster_nodes

[jplindquist]

Pull Request (PR) description

Fix indentation of cluster_nodes in rabbitmq.config.

This is just realigning things after upgrading recently and noticed the change from erb to epp templates, and subsequent conditional added here changed the alignment

This Pull Request (PR) fixes the following issues

Correctly aligns cluster_nodes when empty...

--- /etc/rabbitmq/rabbitmq.config	2024-05-29 20:04:23.109445139 +0000
+++ /tmp/puppet-file20240529-12718-1ikruem	2024-05-29 20:16:08.230884643 +0000
@@ -5,8 +5,7 @@
   {rabbit, [
     {loopback_users, []},
     {auth_backends, [rabbit_auth_backend_internal, rabbit_auth_backend_oauth2]},
-        {cluster_nodes, {[], disc}},
-
+    {cluster_nodes, {[], disc}},
     {cluster_partition_handling, pause_minority},
     {tcp_listen_options, [
          {backlog,       128},

Correctly aligns cluster_nodes when not empty...

--- /etc/rabbitmq/rabbitmq.config	2024-05-29 20:04:23.109445139 +0000
+++ /tmp/puppet-file20240529-14820-1u3363v	2024-05-29 20:17:16.176505986 +0000
@@ -5,8 +5,7 @@
   {rabbit, [
     {loopback_users, []},
     {auth_backends, [rabbit_auth_backend_internal, rabbit_auth_backend_oauth2]},
-        {cluster_nodes, {['[email protected]', '[email protected]', '[email protected]'], disc}},
-
+    {cluster_nodes, {['[email protected]', '[email protected]', '[email protected]'], disc}},
     {cluster_partition_handling, pause_minority},
     {tcp_listen_options, [
          {backlog,       128},

[wyardley]

Thanks for the contribution @jplindquist - appreciate it.

Wonder if it would make the tests too fragile to adjust the existing tests something like this, the second of which should fail on main and pass in your branch?

diff --git a/spec/classes/rabbitmq_spec.rb b/spec/classes/rabbitmq_spec.rb
index 916c82b..7b13a25 100644
--- a/spec/classes/rabbitmq_spec.rb
+++ b/spec/classes/rabbitmq_spec.rb
@@ -408,7 +408,7 @@ describe 'rabbitmq' do
           end
 
           it 'for cluster_nodes' do
-            is_expected.to contain_file('rabbitmq.config').with('content' => %r{cluster_nodes.*\['rabbit@hare-1', 'rabbit@hare-2'\], ram})
+            is_expected.to contain_file('rabbitmq.config').with('content' => %r{^ {8}\{cluster_nodes, \{\['rabbit@hare-1', 'rabbit@hare-2'\], ram})
           end
         end
 
@@ -423,7 +423,7 @@ describe 'rabbitmq' do
           end
 
           it 'for cluster_nodes' do
-            is_expected.to contain_file('rabbitmq.config').with('content' => %r{cluster_nodes.*\[\], ram})
+            is_expected.to contain_file('rabbitmq.config').with('content' => %r{^ {8}\{cluster_nodes, \{\[\], ram})
           end
         end
       end

[wyardley]

☝️ The above should catch indentation changes (at least for this specific line), at the cost of requiring the test to be updated if the formatting of the template / config changed. IMO, that's probably a good thing to avoid unexpected formatting changes of the type you see, but I could see the argument in the other direction as well (that the exact formatting of the file shouldn't matter if the behavior is the same).

It makes rubocop happy, though there might be a more idiomatic way to write / quote the regex. I specifically didn't use \s{8} since that would also match exactly 8 tabs.

[jplindquist]

I agree it would be nice to catch changes like that. I'm happy to update the test with your suggestion above if you'd like?

[wyardley]

I agree it would be nice to catch changes like that. I'm happy to update the test with your suggestion above if you'd like?

Sounds great!
If at some later point, someone wanted to tighten up some of the other tests, I wouldn't mind that either, but probably good to keep this narrow. Lmk if you have problems applying the above as a patch.

[jplindquist]

Thanks again for the quick review and reply. I thought I applied it just like you suggested, but the related tests are failing now. Looking to see if I missed something...

[wyardley]

Should be good, just let me know if that looks right

I didn't check out your branch locally yet, but seems like it's failing, and if I'm pasting out of the CI tests, looks like that one (with nodes, vs. without) is indented 4 characters vs. 8?

[wyardley]

Let me know if you're not able to get setup to run the tests locally, but that will make it easier to verify the diffs and make sure the right behavior is in place

For the one on line 410, I think you have the same value twice, vs. a different one - did you apply my diff above as a patch, or copy it in manually?

[jplindquist]

know if you're not able to get setup to run the tests locally, but that will make it easier to verify the diffs and make sure the right behavior is in place

For the one on line 410, I think you have the same value twice, vs. a different one - did you apply my diff above as a patch, or copy it in manually?

I just copied it manually, I've got things running locally and will recheck, I just think it's 4 spaces vs. 8

[jplindquist]

They're all passing locally at least, let's see how this goes. 🤞 Let me know if you'd like me to squash and clean up any of this too

[wyardley]

Great.
Since you've got signed commits, would you mind squashing those 4 commits down to one and force-pushing if everything passes?

[wyardley]

I sorted out the situation with #1001 so can probably cut a release for those two shortly.

[jplindquist]

Awesome, thanks again for your help with this!