systemctl path fixes, rspec modernization, etc

- systemd provider uses `which` mixin to determine systemctl path
- let-ification of spec tests for system provider
- fix some stubbing and mocking of current_resource that was confusing
  and useless

2 files changed, 225 insertions, 188 deletions

diff --git a/lib/chef/provider/service/systemd.rb b/lib/chef/provider/service/systemd.rb
index 311751ab9a..717ff7dc5c 100644
--- a/lib/chef/provider/service/systemd.rb
+++ b/lib/chef/provider/service/systemd.rb
@@ -18,87 +18,91 @@
 
 require 'chef/resource/service'
 require 'chef/provider/service/simple'
+require 'chef/mixin/which'
 
 class Chef::Provider::Service::Systemd < Chef::Provider::Service::Simple
 
+  include Chef::Mixin::Which
+
   provides :service, os: "linux"
 
+  attr_accessor :status_check_success
+
   def self.supports?(resource, action)
     Chef::Platform::ServiceHelpers.service_resource_providers.include?(:systemd)
   end
 
   def load_current_resource
-    @current_resource = Chef::Resource::Service.new(@new_resource.name)
-    @current_resource.service_name(@new_resource.service_name)
+    @current_resource = Chef::Resource::Service.new(new_resource.name)
+    current_resource.service_name(new_resource.service_name)
     @status_check_success = true
 
-    if @new_resource.status_command
-      Chef::Log.debug("#{@new_resource} you have specified a status command, running..")
+    if new_resource.status_command
+      Chef::Log.debug("#{new_resource} you have specified a status command, running..")
 
-      unless shell_out(@new_resource.status_command).error?
-        @current_resource.running(true)
+      unless shell_out(new_resource.status_command).error?
+        current_resource.running(true)
       else
         @status_check_success = false
-        @current_resource.running(false)
-        @current_resource.enabled(false)
-        nil
+        current_resource.running(false)
+        current_resource.enabled(false)
       end
     else
-      @current_resource.running(is_active?)
+      current_resource.running(is_active?)
     end
 
-    @current_resource.enabled(is_enabled?)
-    @current_resource
+    current_resource.enabled(is_enabled?)
+    current_resource
   end
 
   def define_resource_requirements
     shared_resource_requirements
     requirements.assert(:all_actions) do |a|
-      a.assertion { @status_check_success }
+      a.assertion { status_check_success }
       # We won't stop in any case, but in whyrun warn and tell what we're doing.
-      a.whyrun ["Failed to determine status of #{@new_resource}, using command #{@new_resource.status_command}.",
+      a.whyrun ["Failed to determine status of #{new_resource}, using command #{new_resource.status_command}.",
         "Assuming service would have been installed and is disabled"]
     end
   end
 
   def start_service
-    if @current_resource.running
-      Chef::Log.debug("#{@new_resource} already running, not starting")
+    if current_resource.running
+      Chef::Log.debug("#{new_resource} already running, not starting")
     else
-      if @new_resource.start_command
+      if new_resource.start_command
         super
       else
-        shell_out_with_systems_locale!("/bin/systemctl start #{@new_resource.service_name}")
+        shell_out_with_systems_locale!("#{systemctl_path} start #{new_resource.service_name}")
       end
     end
   end
 
   def stop_service
-    unless @current_resource.running
-      Chef::Log.debug("#{@new_resource} not running, not stopping")
+    unless current_resource.running
+      Chef::Log.debug("#{new_resource} not running, not stopping")
     else
-      if @new_resource.stop_command
+      if new_resource.stop_command
         super
       else
-        shell_out_with_systems_locale!("/bin/systemctl stop #{@new_resource.service_name}")
+        shell_out_with_systems_locale!("#{systemctl_path} stop #{new_resource.service_name}")
       end
     end
   end
 
   def restart_service
-    if @new_resource.restart_command
+    if new_resource.restart_command
       super
     else
-      shell_out_with_systems_locale!("/bin/systemctl restart #{@new_resource.service_name}")
+      shell_out_with_systems_locale!("#{systemctl_path} restart #{new_resource.service_name}")
     end
   end
 
   def reload_service
-    if @new_resource.reload_command
+    if new_resource.reload_command
       super
     else
-      if @current_resource.running
-        shell_out_with_systems_locale!("/bin/systemctl reload #{@new_resource.service_name}")
+      if current_resource.running
+        shell_out_with_systems_locale!("#{systemctl_path} reload #{new_resource.service_name}")
       else
         start_service
       end
@@ -106,18 +110,28 @@ class Chef::Provider::Service::Systemd < Chef::Provider::Service::Simple
   end
 
   def enable_service
-    shell_out!("/bin/systemctl enable #{@new_resource.service_name}")
+    shell_out!("#{systemctl_path} enable #{new_resource.service_name}")
   end
 
   def disable_service
-    shell_out!("/bin/systemctl disable #{@new_resource.service_name}")
+    shell_out!("#{systemctl_path} disable #{new_resource.service_name}")
   end
 
   def is_active?
-    shell_out("/bin/systemctl is-active #{@new_resource.service_name} --quiet").exitstatus == 0
+    shell_out("#{systemctl_path} is-active #{new_resource.service_name} --quiet").exitstatus == 0
   end
 
   def is_enabled?
-    shell_out("/bin/systemctl is-enabled #{@new_resource.service_name} --quiet").exitstatus == 0
+    shell_out("#{systemctl_path} is-enabled #{new_resource.service_name} --quiet").exitstatus == 0
   end
+
+  private
+
+  def systemctl_path
+    if @systemctl_path.nil?
+      @systemctl_path = which("systemctl")
+    end
+    @systemctl_path
+  end
+
 end
diff --git a/spec/unit/provider/service/systemd_service_spec.rb b/spec/unit/provider/service/systemd_service_spec.rb
index 2bdbdaad05..7fa1d11336 100644
--- a/spec/unit/provider/service/systemd_service_spec.rb
+++ b/spec/unit/provider/service/systemd_service_spec.rb
@@ -19,238 +19,261 @@
 require 'spec_helper'
 
 describe Chef::Provider::Service::Systemd do
+
+  let(:node) { Chef::Node.new }
+
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+
+  let(:service_name) { "rsyslog.service" }
+
+  let(:new_resource) { Chef::Resource::Service.new(service_name) }
+
+  let(:provider) { Chef::Provider::Service::Systemd.new(new_resource, run_context) }
+
+  let(:shell_out_success) do
+    double('shell_out_with_systems_locale', :exitstatus => 0, :error? => false)
+  end
+
+  let(:shell_out_failure) do
+    double('shell_out_with_systems_locale', :exitstatus => 1, :error? => true)
+  end
+
+  let(:current_resource) { Chef::Resource::Service.new(service_name) }
+
   before(:each) do
-    @node = Chef::Node.new
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, {}, @events)
-    @new_resource = Chef::Resource::Service.new('rsyslog.service')
-    @provider = Chef::Provider::Service::Systemd.new(@new_resource, @run_context)
-
-    @shell_out_success = double('shell_out_with_systems_locale',
-                                :exitstatus => 0, :error? => false)
-    @shell_out_failure = double('shell_out_with_systems_locale',
-                                :exitstatus => 1, :error? => true)
+    allow(Chef::Resource::Service).to receive(:new).with(service_name).and_return(current_resource)
   end
 
   describe "load_current_resource" do
-    before(:each) do
-      @current_resource = Chef::Resource::Service.new('rsyslog.service')
-      allow(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
 
-      allow(@provider).to receive(:is_active?).and_return(false)
-      allow(@provider).to receive(:is_enabled?).and_return(false)
+    before(:each) do
+      allow(provider).to receive(:is_active?).and_return(false)
+      allow(provider).to receive(:is_enabled?).and_return(false)
     end
 
     it "should create a current resource with the name of the new resource" do
-      expect(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
-      @provider.load_current_resource
+      expect(Chef::Resource::Service).to receive(:new).with(new_resource.name).and_return(current_resource)
+      provider.load_current_resource
     end
 
     it "should set the current resources service name to the new resources service name" do
-      expect(@current_resource).to receive(:service_name).with(@new_resource.service_name)
-      @provider.load_current_resource
+      provider.load_current_resource
+      expect(current_resource.service_name).to eql(service_name)
     end
 
     it "should check if the service is running" do
-      expect(@provider).to receive(:is_active?)
-      @provider.load_current_resource
+      expect(provider).to receive(:is_active?)
+      provider.load_current_resource
     end
 
     it "should set running to true if the service is running" do
-      allow(@provider).to receive(:is_active?).and_return(true)
-      expect(@current_resource).to receive(:running).with(true)
-      @provider.load_current_resource
+      allow(provider).to receive(:is_active?).and_return(true)
+      provider.load_current_resource
+      expect(current_resource.running).to be true
     end
 
     it "should set running to false if the service is not running" do
-      allow(@provider).to receive(:is_active?).and_return(false)
-      expect(@current_resource).to receive(:running).with(false)
-      @provider.load_current_resource
+      allow(provider).to receive(:is_active?).and_return(false)
+      provider.load_current_resource
+      expect(current_resource.running).to be false
     end
 
     describe "when a status command has been specified" do
       before do
-        allow(@new_resource).to receive(:status_command).and_return("/bin/chefhasmonkeypants status")
+        allow(new_resource).to receive(:status_command).and_return("/bin/chefhasmonkeypants status")
       end
 
       it "should run the services status command if one has been specified" do
-        allow(@provider).to receive(:shell_out).and_return(@shell_out_success)
-        expect(@current_resource).to receive(:running).with(true)
-        @provider.load_current_resource
+        allow(provider).to receive(:shell_out).and_return(shell_out_success)
+        provider.load_current_resource
+        expect(current_resource.running).to be true
       end
 
       it "should run the services status command if one has been specified and properly set status check state" do
-        allow(@provider).to receive(:shell_out).with("/bin/chefhasmonkeypants status").and_return(@shell_out_success)
-        @provider.load_current_resource
-        expect(@provider.instance_variable_get("@status_check_success")).to be_truthy
+        allow(provider).to receive(:shell_out).with("/bin/chefhasmonkeypants status").and_return(shell_out_success)
+        provider.load_current_resource
+        expect(provider.status_check_success).to be true
       end
 
       it "should set running to false if a status command fails" do
-        allow(@provider).to receive(:shell_out).and_return(@shell_out_failure)
-        expect(@current_resource).to receive(:running).with(false)
-        @provider.load_current_resource
+        allow(provider).to receive(:shell_out).and_return(shell_out_failure)
+        provider.load_current_resource
+        expect(current_resource.running).to be false
       end
 
       it "should update state to indicate status check failed when a status command fails" do
-        allow(@provider).to receive(:shell_out).and_return(@shell_out_failure)
-        @provider.load_current_resource
-        expect(@provider.instance_variable_get("@status_check_success")).to be_falsey
+        allow(provider).to receive(:shell_out).and_return(shell_out_failure)
+        provider.load_current_resource
+        expect(provider.status_check_success).to be false
       end
     end
 
     it "should check if the service is enabled" do
-      expect(@provider).to receive(:is_enabled?)
-      @provider.load_current_resource
+      expect(provider).to receive(:is_enabled?)
+      provider.load_current_resource
     end
 
     it "should set enabled to true if the service is enabled" do
-      allow(@provider).to receive(:is_enabled?).and_return(true)
-      expect(@current_resource).to receive(:enabled).with(true)
-      @provider.load_current_resource
+      allow(provider).to receive(:is_enabled?).and_return(true)
+      provider.load_current_resource
+      expect(current_resource.enabled).to be true
     end
 
     it "should set enabled to false if the service is not enabled" do
-      allow(@provider).to receive(:is_enabled?).and_return(false)
-      expect(@current_resource).to receive(:enabled).with(false)
-      @provider.load_current_resource
+      allow(provider).to receive(:is_enabled?).and_return(false)
+      provider.load_current_resource
+      expect(current_resource.enabled).to be false
     end
 
     it "should return the current resource" do
-      expect(@provider.load_current_resource).to eql(@current_resource)
+      expect(provider.load_current_resource).to eql(current_resource)
     end
   end
 
-  describe "start and stop service" do
-    before(:each) do
-      @current_resource = Chef::Resource::Service.new('rsyslog.service')
-      allow(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
-      @provider.current_resource = @current_resource
-    end
+  def setup_current_resource
+    provider.current_resource = current_resource
+    current_resource.service_name(service_name)
+  end
 
-    it "should call the start command if one is specified" do
-      allow(@new_resource).to receive(:start_command).and_return("/sbin/rsyslog startyousillysally")
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog startyousillysally")
-      @provider.start_service
-    end
+  %w{/usr/bin/systemctl /bin/systemctl}.each do |systemctl_path|
+    describe "when systemctl path is #{systemctl_path}" do
+      before(:each) do
+        setup_current_resource
+        allow(provider).to receive(:which).with("systemctl").and_return(systemctl_path)
+      end
 
-    it "should call '/bin/systemctl start service_name' if no start command is specified" do
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/bin/systemctl start #{@new_resource.service_name}").and_return(@shell_out_success)
-      @provider.start_service
-    end
+      describe "start and stop service" do
 
-    it "should not call '/bin/systemctl start service_name' if it is already running" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      expect(@provider).not_to receive(:shell_out_with_systems_locale!).with("/bin/systemctl start #{@new_resource.service_name}")
-      @provider.start_service
-    end
+        it "should call the start command if one is specified" do
+          allow(new_resource).to receive(:start_command).and_return("/sbin/rsyslog startyousillysally")
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog startyousillysally")
+          provider.start_service
+        end
 
-    it "should call the restart command if one is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      allow(@new_resource).to receive(:restart_command).and_return("/sbin/rsyslog restartyousillysally")
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog restartyousillysally")
-      @provider.restart_service
-    end
+        it "should call '#{systemctl_path} start service_name' if no start command is specified" do
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} start #{service_name}").and_return(shell_out_success)
+          provider.start_service
+        end
 
-    it "should call '/bin/systemctl restart service_name' if no restart command is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/bin/systemctl restart #{@new_resource.service_name}").and_return(@shell_out_success)
-      @provider.restart_service
-    end
+        it "should not call '#{systemctl_path} start service_name' if it is already running" do
+          current_resource.running(true)
+          expect(provider).not_to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} start #{service_name}")
+          provider.start_service
+        end
 
-    describe "reload service" do
-      context "when a reload command is specified" do
-        it "should call the reload command" do
-          allow(@current_resource).to receive(:running).and_return(true)
-          allow(@new_resource).to receive(:reload_command).and_return("/sbin/rsyslog reloadyousillysally")
-          expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog reloadyousillysally")
-          @provider.reload_service
+        it "should call the restart command if one is specified" do
+          current_resource.running(true)
+          allow(new_resource).to receive(:restart_command).and_return("/sbin/rsyslog restartyousillysally")
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog restartyousillysally")
+          provider.restart_service
         end
-      end
 
-      context "when a reload command is not specified" do
-        it "should call '/bin/systemctl reload service_name' if the service is running" do
-          allow(@current_resource).to receive(:running).and_return(true)
-          expect(@provider).to receive(:shell_out_with_systems_locale!).with("/bin/systemctl reload #{@new_resource.service_name}").and_return(@shell_out_success)
-          @provider.reload_service
+        it "should call '#{systemctl_path} restart service_name' if no restart command is specified" do
+          current_resource.running(true)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} restart #{service_name}").and_return(shell_out_success)
+          provider.restart_service
         end
 
-        it "should start the service if the service is not running" do
-          allow(@current_resource).to receive(:running).and_return(false)
-          expect(@provider).to receive(:start_service).and_return(true)
-          @provider.reload_service
+        describe "reload service" do
+          context "when a reload command is specified" do
+            it "should call the reload command" do
+              current_resource.running(true)
+              allow(new_resource).to receive(:reload_command).and_return("/sbin/rsyslog reloadyousillysally")
+              expect(provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog reloadyousillysally")
+              provider.reload_service
+            end
+          end
+
+          context "when a reload command is not specified" do
+            it "should call '#{systemctl_path} reload service_name' if the service is running" do
+              current_resource.running(true)
+              expect(provider).to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} reload #{service_name}").and_return(shell_out_success)
+              provider.reload_service
+            end
+
+            it "should start the service if the service is not running" do
+              current_resource.running(false)
+              expect(provider).to receive(:start_service).and_return(true)
+              provider.reload_service
+            end
+          end
         end
-      end
-    end
 
-    it "should call the stop command if one is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      allow(@new_resource).to receive(:stop_command).and_return("/sbin/rsyslog stopyousillysally")
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog stopyousillysally")
-      @provider.stop_service
-    end
+        it "should call the stop command if one is specified" do
+          current_resource.running(true)
+          allow(new_resource).to receive(:stop_command).and_return("/sbin/rsyslog stopyousillysally")
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("/sbin/rsyslog stopyousillysally")
+          provider.stop_service
+        end
 
-    it "should call '/bin/systemctl stop service_name' if no stop command is specified" do
-      allow(@current_resource).to receive(:running).and_return(true)
-      expect(@provider).to receive(:shell_out_with_systems_locale!).with("/bin/systemctl stop #{@new_resource.service_name}").and_return(@shell_out_success)
-      @provider.stop_service
-    end
+        it "should call '#{systemctl_path} stop service_name' if no stop command is specified" do
+          current_resource.running(true)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} stop #{service_name}").and_return(shell_out_success)
+          provider.stop_service
+        end
 
-    it "should not call '/bin/systemctl stop service_name' if it is already stopped" do
-      allow(@current_resource).to receive(:running).and_return(false)
-      expect(@provider).not_to receive(:shell_out_with_systems_locale!).with("/bin/systemctl stop #{@new_resource.service_name}")
-      @provider.stop_service
-    end
-  end
+        it "should not call '#{systemctl_path} stop service_name' if it is already stopped" do
+          current_resource.running(false)
+          expect(provider).not_to receive(:shell_out_with_systems_locale!).with("#{systemctl_path} stop #{service_name}")
+          provider.stop_service
+        end
+      end
 
-  describe "enable and disable service" do
-    before(:each) do
-      @current_resource = Chef::Resource::Service.new('rsyslog.service')
-      allow(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
-      @provider.current_resource = @current_resource
-    end
+      describe "enable and disable service" do
+        before(:each) do
+          provider.current_resource = current_resource
+          current_resource.service_name(service_name)
+          allow(provider).to receive(:which).with("systemctl").and_return("#{systemctl_path}")
+        end
 
-    it "should call '/bin/systemctl enable service_name' to enable the service" do
-      expect(@provider).to receive(:shell_out!).with("/bin/systemctl enable #{@new_resource.service_name}").and_return(@shell_out_success)
-      @provider.enable_service
-    end
+        it "should call '#{systemctl_path} enable service_name' to enable the service" do
+          expect(provider).to receive(:shell_out!).with("#{systemctl_path} enable #{service_name}").and_return(shell_out_success)
+          provider.enable_service
+        end
 
-    it "should call '/bin/systemctl disable service_name' to disable the service" do
-      expect(@provider).to receive(:shell_out!).with("/bin/systemctl disable #{@new_resource.service_name}").and_return(@shell_out_success)
-      @provider.disable_service
-    end
-  end
+        it "should call '#{systemctl_path} disable service_name' to disable the service" do
+          expect(provider).to receive(:shell_out!).with("#{systemctl_path} disable #{service_name}").and_return(shell_out_success)
+          provider.disable_service
+        end
+      end
 
-  describe "is_active?" do
-    before(:each) do
-      @current_resource = Chef::Resource::Service.new('rsyslog.service')
-      allow(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
-    end
+      describe "is_active?" do
+        before(:each) do
+          provider.current_resource = current_resource
+          current_resource.service_name(service_name)
+          allow(provider).to receive(:which).with("systemctl").and_return("#{systemctl_path}")
+        end
 
-    it "should return true if '/bin/systemctl is-active service_name' returns 0" do
-      expect(@provider).to receive(:shell_out).with('/bin/systemctl is-active rsyslog.service --quiet').and_return(@shell_out_success)
-      expect(@provider.is_active?).to be_truthy
-    end
+        it "should return true if '#{systemctl_path} is-active service_name' returns 0" do
+          expect(provider).to receive(:shell_out).with("#{systemctl_path} is-active #{service_name} --quiet").and_return(shell_out_success)
+          expect(provider.is_active?).to be_true
+        end
 
-    it "should return false if '/bin/systemctl is-active service_name' returns anything except 0" do
-      expect(@provider).to receive(:shell_out).with('/bin/systemctl is-active rsyslog.service --quiet').and_return(@shell_out_failure)
-      expect(@provider.is_active?).to be_falsey
-    end
-  end
+        it "should return false if '#{systemctl_path} is-active service_name' returns anything except 0" do
+          expect(provider).to receive(:shell_out).with("#{systemctl_path} is-active #{service_name} --quiet").and_return(shell_out_failure)
+          expect(provider.is_active?).to be_false
+        end
+      end
 
-  describe "is_enabled?" do
-    before(:each) do
-      @current_resource = Chef::Resource::Service.new('rsyslog.service')
-      allow(Chef::Resource::Service).to receive(:new).and_return(@current_resource)
-    end
+      describe "is_enabled?" do
+        before(:each) do
+          provider.current_resource = current_resource
+          current_resource.service_name(service_name)
+          allow(provider).to receive(:which).with("systemctl").and_return("#{systemctl_path}")
+        end
 
-    it "should return true if '/bin/systemctl is-enabled service_name' returns 0" do
-      expect(@provider).to receive(:shell_out).with('/bin/systemctl is-enabled rsyslog.service --quiet').and_return(@shell_out_success)
-      expect(@provider.is_enabled?).to be_truthy
-    end
+        it "should return true if '#{systemctl_path} is-enabled service_name' returns 0" do
+          expect(provider).to receive(:shell_out).with("#{systemctl_path} is-enabled #{service_name} --quiet").and_return(shell_out_success)
+          expect(provider.is_enabled?).to be_true
+        end
 
-    it "should return false if '/bin/systemctl is-enabled service_name' returns anything except 0" do
-      expect(@provider).to receive(:shell_out).with('/bin/systemctl is-enabled rsyslog.service --quiet').and_return(@shell_out_failure)
-      expect(@provider.is_enabled?).to be_falsey
+        it "should return false if '#{systemctl_path} is-enabled service_name' returns anything except 0" do
+          expect(provider).to receive(:shell_out).with("#{systemctl_path} is-enabled #{service_name} --quiet").and_return(shell_out_failure)
+          expect(provider.is_enabled?).to be_false
+        end
+      end
     end
   end
 end