Add service_restart option to prevent automatic service reload

[spuder]

Pull Request (PR) description

Make the restart optional since it is dangerous to do an uncontrolled rolling restart. We lost many messages this way.
It has been many years since I have written any puppet code so I don't have high confidence that I did this entirely correct. It is based off of how the kafka module prevents rolling restarts https://github.com/voxpupuli/puppet-kafka/search?q=service_restart&unscoped_q=service_restart

This Pull Request (PR) fixes the following issues

Fixes #727

[wyardley]

See one comment. I think the basic idea is right from a quick once-over, but we’ll need spec tests to be added for the case where it’s disabled, and any failing tests should also be updated

[wyardley]

Should be restart here

[spuder]

I've fixed the failing tests. I'm not able to get a valid test to check this. Any help appreciated

not working

      describe 'service with service_restart equal to false' do
        let :params do
          { service_restart: false }
        end

        it { is_expected.not_to contain_file('rabbitmq.config').that_notifies("Class[rabbitmq::service]") }
      end

[wyardley]

@spuder You could try on IRC or in the #testing channel on Puppet Slack. I haven't been working with Puppet or rspec that much recently. I'm not sure that's the best way to test this... but if I invert your test, it passes:

$ bundle install
$ bundle exec rake spec_prep
$ bundle exec rspec spec/classes/rabbitmq_spec.rb:1459

--- a/spec/classes/rabbitmq_spec.rb
+++ b/spec/classes/rabbitmq_spec.rb
@@ -1455,6 +1455,15 @@ describe 'rabbitmq' do
 
         it { is_expected.not_to contain_service('rabbitmq-server') }
       end
+
+      describe 'service with service_restart equal to false' do
+        let :params do
+          { service_restart: false }
+        end
+
+        it { is_expected.to contain_file('rabbitmq.config').that_notifies('Class[rabbitmq::service]') }
+      end
+
     end
   end
 end

  on redhat-7-x86_64
    service with service_restart equal to false
      should contain File[rabbitmq.config] that notifies Class[rabbitmq::service]

So I think that because there's a class level notify (via the squiggly arrow), you need to make this logic happen at that level, and the notify statements in the config class should all be redundant.

  -> Class['rabbitmq::config']
  ~> Class['rabbitmq::service']

I think you can handle the logic here, and then it will also be easier to test the class relationships directly. As a plus, we'll remove some cruft from the codebase too.

[wyardley]

@spuder I PRed your PR - let me know if that works. I changed what the test is as well. There might be a more elegant way, but I think it works.

  if ($service_restart) {
    Class['rabbitmq::config'] ~> Class['rabbitmq::service']
  }

  Class['rabbitmq::install']
  -> Class['rabbitmq::config']
  -> Class['rabbitmq::service']
  -> Class['rabbitmq::management']

If that looks good, can you merge my PR, then squash those commits down your branch, and I'll get one other person to review? It's fine to either squash my commit in and leave it separate; either way, squash down the other commits if you know how. (should be something like git rebase -i HEAD~5, change 'pick' to 'fix' for the commits you want to squash, then git push --force-with-lease).

[spuder]

Thanks for the help. Tests are passing and everything looks correct. I squashed the commits (inadvertently including your commit). If the travis tests pass I believe this would be good to merge.

[wyardley]

@bastelfreak @ekohl: I know you're both busy, but if either of you has a chance to review, would appreciate it.

[wyardley]

I'm good with this, but I also suggested some of the changes, let's get one more +1 on here too.

[ekohl]

I know https://github.com/puppetlabs/puppetlabs-postgresql has a mechanism to reload. Would that that work here?

[ekohl]

I think this could be phrased a bit clearer. How about:
Whether to restart the service on config change.

[spuder]

Fixed

[wyardley]

I know https://github.com/puppetlabs/puppetlabs-postgresql has a mechanism to reload. Would that
that work here?

It's been a little, but I think not. From a quick look, at least, it would seem that reload and restart both will restart the service.

[wyardley]

Thanks for the contribution (and patience)