This further normalizes the handling of init-style services (including

the redhat "service" wrapper script).  Removes special case handling of
non-zero exit code in redhat (base already did this) and centralizes
scattered @resource[:has_____] checks.  Tests that proper versions of
each are called and one level of fallbacks.

Signed-off-by: Markus Roberts <Markus@reality.com>

5 files changed, 298 insertions, 58 deletions

diff --git a/lib/puppet/provider/service/base.rb b/lib/puppet/provider/service/base.rb
index 462dd897d..0bf2b20ea 100755
--- a/lib/puppet/provider/service/base.rb
+++ b/lib/puppet/provider/service/base.rb
@@ -19,13 +19,11 @@ Puppet::Type.type(:service).provide :base do
     # parameter.
     def getpid
         unless @resource[:pattern]
-            @resource.fail "Either a stop command or a pattern must be specified"
+            @resource.fail "Either stop/status commands or a pattern must be specified"
         end
         ps = Facter["ps"].value
         unless ps and ps != ""
-            @resource.fail(
-                "You must upgrade Facter to a version that includes 'ps'"
-            )
+            @resource.fail "You must upgrade Facter to a version that includes 'ps'"
         end
         regex = Regexp.new(@resource[:pattern])
         self.debug "Executing '#{ps}'"
@@ -43,7 +41,7 @@ Puppet::Type.type(:service).provide :base do
 
     # How to restart the process.
     def restart
-        if @resource[:restart] or self.respond_to?(:restartcmd)
+        if @resource[:restart] or restartcmd
             ucommand(:restart)
         else
             self.stop
@@ -51,19 +49,22 @@ Puppet::Type.type(:service).provide :base do
         end
     end
 
+    # There is no default command, which causes other methods to be used  
+    def restartcmd
+    end
+
     # Check if the process is running.  Prefer the 'status' parameter,
     # then 'statuscmd' method, then look in the process table.  We give
     # the object the option to not return a status command, which might
     # happen if, for instance, it has an init script (and thus responds to
     # 'statuscmd') but does not have 'hasstatus' enabled.
     def status
-        if @resource[:status] or (
-            self.respond_to?(:statuscmd) and self.statuscmd
-        )
+        if @resource[:status] or statuscmd
             # Don't fail when the exit status is not 0.
-            output = ucommand(:status, false)
+            ucommand(:status, false)
 
-            if $? == 0
+            # Expicitly calling exitstatus to facilitate testing
+            if $?.exitstatus == 0
                 return :running
             else
                 return :stopped
@@ -76,6 +77,10 @@ Puppet::Type.type(:service).provide :base do
         end
     end
 
+    # There is no default command, which causes other methods to be used  
+    def statuscmd
+    end
+    
     # Run the 'start' parameter command, or the specified 'startcmd'.
     def start
         ucommand(:start)
@@ -98,7 +103,7 @@ Puppet::Type.type(:service).provide :base do
     # for the process in the process table.
     # This method will generally not be overridden by submodules.
     def stop
-        if @resource[:stop] or self.respond_to?(:stopcmd)
+        if @resource[:stop] or stopcmd
             ucommand(:stop)
         else
             pid = getpid
@@ -115,6 +120,10 @@ Puppet::Type.type(:service).provide :base do
             return true
         end
     end
+    
+    # There is no default command, which causes other methods to be used  
+    def stopcmd
+    end
 
     # A simple wrapper so execution failures are a bit more informative.
     def texecute(type, command, fof = true)
diff --git a/lib/puppet/provider/service/init.rb b/lib/puppet/provider/service/init.rb
index 1bf11fbba..965773d96 100755
--- a/lib/puppet/provider/service/init.rb
+++ b/lib/puppet/provider/service/init.rb
@@ -67,20 +67,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # Where is our init script?
     def initscript
-        if defined? @initscript
-            return @initscript
-        else
-            @initscript = self.search(@resource[:name])
-        end
-    end
-
-    def restart
-        if @resource[:hasrestart] == :true
-            command = [self.initscript, :restart]
-            texecute("restart", command)
-        else
-            super
-        end
+        @initscript ||= self.search(@resource[:name])
     end
 
     def search(name)
@@ -115,23 +102,24 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # The start command is just the init scriptwith 'start'.
     def startcmd
-        [self.initscript, :start]
+        [initscript, :start]
+    end
+
+    # The stop command is just the init script with 'stop'.
+    def stopcmd
+        [initscript, :stop]
+    end
+
+    def restartcmd
+        (@resource[:hasrestart] == :true) && [initscript, :restart]
     end
 
     # If it was specified that the init script has a 'status' command, then
     # we just return that; otherwise, we return false, which causes it to
     # fallback to other mechanisms.
     def statuscmd
-        if @resource[:hasstatus] == :true
-            return [self.initscript, :status]
-        else
-            return false
-        end
+        (@resource[:hasstatus] == :true) && [initscript, :status]
     end
 
-    # The stop command is just the init script with 'stop'.
-    def stopcmd
-        [self.initscript, :stop]
-    end
 end
 
diff --git a/lib/puppet/provider/service/redhat.rb b/lib/puppet/provider/service/redhat.rb
index 8c12782d5..211b66956 100755
--- a/lib/puppet/provider/service/redhat.rb
+++ b/lib/puppet/provider/service/redhat.rb
@@ -52,35 +52,16 @@ Puppet::Type.type(:service).provide :redhat, :parent => :init do
         end
     end
 
-    def restart
-        if @resource[:hasrestart] == :true
-            service(@resource[:name], "restart")
-        else
-            super
-        end
-    end
-
-    def status
-        if @resource[:status]
-            super
-        elsif @resource[:hasstatus] == :true
-            begin
-                service(@resource[:name], "status")
-                return :running
-            rescue
-                return :stopped
-            end
-        else
-            super
-        end
+    def initscript
+        raise Puppet::Error, "Do not directly call the init script for '%s'; use 'service' instead" % @resource[:name]
     end
 
     def statuscmd
-        [command(:service), @resource[:name], "status"]
+        (@resource[:hasstatus] == :true) && [command(:service), @resource[:name], "status"]
     end
 
     def restartcmd
-        [command(:service), @resource[:name], "restart"]
+        (@resource[:hasrestart] == :true) && [command(:service), @resource[:name], "restart"]
     end
 
     def startcmd
diff --git a/spec/unit/provider/service/init.rb b/spec/unit/provider/service/init.rb
new file mode 100644
index 000000000..297768e33
--- /dev/null
+++ b/spec/unit/provider/service/init.rb
@@ -0,0 +1,135 @@
+#!/usr/bin/env ruby
+#
+# Unit testing for the Init service Provider
+#
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+provider_class = Puppet::Type.type(:service).provider(:init)
+
+describe provider_class do
+
+    before(:each) do
+        # Create a mock resource
+        @resource = stub 'resource'
+
+        @provider = provider_class.new
+        # A catch all; no parameters set
+        @resource.stubs(:[]).returns(nil)
+
+        # But set name, source and path (because we won't run
+        # the thing that will fetch the resource path from the provider)
+        @resource.stubs(:[]).with(:name).returns "myservice"
+        @resource.stubs(:[]).with(:ensure).returns :enabled
+        @resource.stubs(:[]).with(:path).returns ["/service/path","/alt/service/path"]
+        @resource.stubs(:ref).returns "Service[myservice]"
+        
+        @provider.stubs(:resource).returns @resource
+        @provider.resource = @resource
+    end
+
+    it "should have a start method" do
+        @provider.should respond_to(:start)
+    end
+
+    it "should have a stop method" do
+        @provider.should respond_to(:stop)
+    end
+
+    it "should have a restart method" do
+        @provider.should respond_to(:restart)
+    end
+
+    it "should have a status method" do
+        @provider.should respond_to(:status)
+    end
+
+    describe "when serching for the init script" do
+        it "should be able to find the init script in the service path" do
+            File.expects(:stat).with("/service/path/myservice").returns true
+            @provider.initscript.should == "/service/path/myservice"
+        end
+        it "should be able to find the init script in the service path" do
+            File.expects(:stat).with("/alt/service/path/myservice").returns true
+            @provider.initscript.should == "/alt/service/path/myservice"
+        end
+        it "should fail if the service isn't there" do
+            lambda { @provider.initscript }.should raise_error(Puppet::Error, "Could not find init script for 'myservice'")
+        end
+    end
+    
+    describe "if the init script is present" do
+        before :each do
+            File.stubs(:stat).with("/service/path/myservice").returns true
+        end
+        
+        describe "when starting" do
+            it "should execute the init script with start" do
+                @provider.expects(:texecute).with(:start, ['/service/path/myservice', :start], true).returns("")
+                @provider.start
+            end
+        end
+
+        describe "when stopping" do
+            it "should execute the init script with stop" do
+              @provider.expects(:texecute).with(:stop, ['/service/path/myservice', :stop], true).returns("")
+                @provider.stop
+            end
+        end
+
+        describe "when checking status" do
+            describe "when hasstatus is :true" do
+                before :each do
+                    @resource.stubs(:[]).with(:hasstatus).returns :true
+                end
+                it "should execute the command" do
+                    @provider.expects(:texecute).with(:status, ['/service/path/myservice', :status], false).returns("")
+                    @provider.status
+                end
+                it "should consider the process running if the command returns 0" do
+                    @provider.expects(:texecute).with(:status, ['/service/path/myservice', :status], false).returns("")
+                    $?.stubs(:exitstatus).returns(0)
+                    @provider.status.should == :running
+                end
+                [-10,-1,1,10].each { |ec|
+                    it "should consider the process stopped if the command returns something non-0" do
+                        @provider.expects(:texecute).with(:status, ['/service/path/myservice', :status], false).returns("")
+                        $?.stubs(:exitstatus).returns(ec)
+                        @provider.status.should == :stopped
+                    end
+                }
+            end
+            describe "when hasstatus is not :true" do
+                it "should consider the service :running if it has a pid" do
+                    @provider.expects(:getpid).returns "1234"
+                    @provider.status.should == :running
+                end
+                it "should consider the service :stopped if it doesn't have a pid" do
+                    @provider.expects(:getpid).returns nil
+                    @provider.status.should == :stopped
+                end
+            end
+        end
+
+        describe "when restarting" do
+            describe "when hasrestart is :true" do
+                before :each do
+                    @resource.stubs(:[]).with(:hasrestart).returns :true
+                end
+                it "should execute the command" do
+                    @provider.expects(:texecute).with(:restart, ['/service/path/myservice', :restart], true).returns("")
+                    $?.stubs(:exitstatus).returns(0)
+                    @provider.restart
+                end
+            end
+            describe "when hasrestart is not :true" do
+                it "should stop and restart the process" do
+                    @provider.expects(:texecute).with(:stop, ['/service/path/myservice', :stop ], true).returns("")
+                    @provider.expects(:texecute).with(:start,['/service/path/myservice', :start], true).returns("")
+                    $?.stubs(:exitstatus).returns(0)
+                    @provider.restart
+                end
+            end
+        end
+    end
+end
diff --git a/spec/unit/provider/service/redhat.rb b/spec/unit/provider/service/redhat.rb
new file mode 100644
index 000000000..df2c93f57
--- /dev/null
+++ b/spec/unit/provider/service/redhat.rb
@@ -0,0 +1,127 @@
+#!/usr/bin/env ruby
+#
+# Unit testing for the RedHat service Provider
+#
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+provider_class = Puppet::Type.type(:service).provider(:redhat)
+
+describe provider_class do
+
+    before(:each) do
+        # Create a mock resource
+        @resource = stub 'resource'
+
+        @provider = provider_class.new
+        # A catch all; no parameters set
+        @resource.stubs(:[]).returns(nil)
+
+        # But set name, source and path (because we won't run
+        # the thing that will fetch the resource path from the provider)
+        @resource.stubs(:[]).with(:name).returns "myservice"
+        @resource.stubs(:[]).with(:ensure).returns :enabled
+        @resource.stubs(:[]).with(:path).returns ["/service/path","/alt/service/path"]
+        @resource.stubs(:ref).returns "Service[myservice]"
+        
+        @provider.stubs(:resource).returns @resource
+        @provider.resource = @resource
+    end
+
+    it "should have a start method" do
+        @provider.should respond_to(:start)
+    end
+
+    it "should have a stop method" do
+        @provider.should respond_to(:stop)
+    end
+
+    it "should have a restart method" do
+        @provider.should respond_to(:restart)
+    end
+
+    it "should have a status method" do
+        @provider.should respond_to(:status)
+    end
+
+    it "should have an enabled? method" do
+        @provider.should respond_to(:enabled?)
+    end
+
+    it "should have an enable method" do
+        @provider.should respond_to(:enable)
+    end
+
+    it "should have a disable method" do
+        @provider.should respond_to(:disable)
+    end
+
+    describe "when starting" do
+        it "should execute the service script with start" do
+            @provider.expects(:texecute).with(:start, ['/sbin/service', 'myservice', 'start'], true)
+            @provider.start
+        end
+    end
+
+    describe "when stopping" do
+        it "should execute the init script with stop" do
+            @provider.expects(:texecute).with(:stop, ['/sbin/service', 'myservice', 'stop'], true)
+            @provider.stop
+        end
+    end
+
+    describe "when checking status" do
+        describe "when hasstatus is :true" do
+            before :each do
+                @resource.stubs(:[]).with(:hasstatus).returns :true
+            end
+            it "should execute the command" do
+                @provider.expects(:texecute).with(:status, ['/sbin/service', 'myservice', 'status'], false)
+                @provider.status
+            end
+            it "should consider the process running if the command returns 0" do
+                @provider.expects(:texecute).with(:status, ['/sbin/service', 'myservice', 'status'], false)
+                $?.stubs(:exitstatus).returns(0)
+                @provider.status.should == :running
+            end
+            [-10,-1,1,10].each { |ec|
+                it "should consider the process stopped if the command returns something non-0" do
+                    @provider.expects(:texecute).with(:status, ['/sbin/service', 'myservice', 'status'], false)
+                    $?.stubs(:exitstatus).returns(ec)
+                    @provider.status.should == :stopped
+                end
+            }
+        end
+        describe "when hasstatus is not :true" do
+            it "should consider the service :running if it has a pid" do
+                @provider.expects(:getpid).returns "1234"
+                @provider.status.should == :running
+            end
+            it "should consider the service :stopped if it doesn't have a pid" do
+                  @provider.expects(:getpid).returns nil
+                  @provider.status.should == :stopped
+            end
+        end
+    end
+
+    describe "when restarting" do
+        describe "when hasrestart is :true" do
+            before :each do
+                @resource.stubs(:[]).with(:hasrestart).returns :true
+            end
+            it "should execute the command" do
+                @provider.expects(:texecute).with(:restart, ['/sbin/service', 'myservice', 'restart'], true)
+                $?.stubs(:exitstatus).returns(0)
+                @provider.restart
+            end
+        end
+        describe "when hasrestart is not :true" do
+            it "should stop and restart the process" do
+                @provider.expects(:texecute).with(:stop,  ['/sbin/service', 'myservice', 'stop'],  true)
+                @provider.expects(:texecute).with(:start, ['/sbin/service', 'myservice', 'start'], true)
+                $?.stubs(:exitstatus).returns(0)
+                @provider.restart
+            end
+        end
+    end
+end