Fixing #2914 - pre/post hooks now work for transactions

This was built to be used with etckeeper to version control
files in /etc, but can be used for essentially anything.

This patch was built to be added to 0.25.4, so it's a least-modify
approach.  A better approach would be to refactor application/puppet.rb
just a bit so it uses Configurer more.

This is a simple patch - it just defines 'prerun_command' and 'postrun_command'
settings, and runs the appropriate command around each transaction
if they're set.

Signed-off-by: Luke Kanies <luke@reductivelabs.com>

6 files changed, 123 insertions, 6 deletions

diff --git a/lib/puppet/application/puppet.rb b/lib/puppet/application/puppet.rb
index 2929d54e8..273c7d0e7 100644
--- a/lib/puppet/application/puppet.rb
+++ b/lib/puppet/application/puppet.rb
@@ -1,5 +1,6 @@
 require 'puppet'
 require 'puppet/application'
+require 'puppet/configurer'
 require 'puppet/network/handler'
 require 'puppet/network/client'
 
@@ -124,9 +125,14 @@ Puppet::Application.new(:puppet) do
 
             catalog.retrieval_duration = Time.now - starttime
 
+            configurer = Puppet::Configurer.new
+            configurer.execute_prerun_command
+
             # And apply it
             transaction = catalog.apply
 
+            configurer.execute_postrun_command
+
             status = 0
             if not Puppet[:noop] and options[:detailed_exitcodes] then
                 transaction.generate_report
@@ -135,9 +141,7 @@ Puppet::Application.new(:puppet) do
             end
             exit(status)
         rescue => detail
-            if Puppet[:trace]
-                puts detail.backtrace
-            end
+            puts detail.backtrace if Puppet[:trace]
             if detail.is_a?(XMLRPC::FaultException)
                 $stderr.puts detail.message
             else
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 3e72fa574..ec61272ef 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -5,6 +5,8 @@ require 'puppet/network/http_pool'
 require 'puppet/util'
 
 class Puppet::Configurer
+    class CommandHookError < RuntimeError; end
+
     require 'puppet/configurer/fact_handler'
     require 'puppet/configurer/plugin_handler'
 
@@ -39,6 +41,14 @@ class Puppet::Configurer
         @catalog = nil
     end
 
+    def execute_postrun_command
+        execute_from_setting(:postrun_command)
+    end
+
+    def execute_prerun_command
+        execute_from_setting(:prerun_command)
+    end
+
     # Initialize and load storage
     def dostorage
         begin
@@ -75,6 +85,8 @@ class Puppet::Configurer
         download_plugins()
 
         download_fact_plugins()
+
+        execute_prerun_command
     end
 
     # Get the remote catalog, yo.  Returns nil if no catalog can be found.
@@ -160,6 +172,8 @@ class Puppet::Configurer
         # Now close all of our existing http connections, since there's no
         # reason to leave them lying open.
         Puppet::Network::HttpPool.clear_http_instances
+    ensure
+        execute_postrun_command
     end
 
     private
@@ -180,4 +194,14 @@ class Puppet::Configurer
 
         return timeout
     end
+
+    def execute_from_setting(setting)
+        return if (command = Puppet[setting]) == ""
+
+        begin
+            Puppet::Util.execute([command])
+        rescue => detail
+            raise CommandHookError, "Could not run command from #{setting}: #{detail}"
+        end
+    end
 end
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 6b9c43b81..21cee7a05 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -199,7 +199,12 @@ module Puppet
             reports, allowing you to correlate changes on your hosts to the source version on the server."],
         :zlib => [true, 
             "Boolean; whether to use the zlib library",
-        ]
+        ],
+        :prerun_command => ["", "A command to run before every agent run.  If this command returns a non-zero
+            return code, the entire Puppet run will fail."],
+        :postrun_command => ["", "A command to run after every agent run.  If this command returns a non-zero
+            return code, the entire Puppet run will be considered to have failed, even though it might have
+            performed work during the normal run."]
     )
 
     hostname = Facter["hostname"].value
diff --git a/spec/integration/defaults.rb b/spec/integration/defaults.rb
index 292abbb87..e97035dea 100755
--- a/spec/integration/defaults.rb
+++ b/spec/integration/defaults.rb
@@ -215,4 +215,12 @@ describe "Puppet defaults" do
             Puppet.settings[:report_server].should == "report_server"
         end
     end
+
+    it "should have a 'prerun_command' that defaults to the empty string" do
+        Puppet.settings[:prerun_command].should == ""
+    end
+
+    it "should have a 'postrun_command' that defaults to the empty string" do
+        Puppet.settings[:postrun_command].should == ""
+    end
 end
diff --git a/spec/unit/application/puppet.rb b/spec/unit/application/puppet.rb
index 7703bac08..61dcf9061 100755
--- a/spec/unit/application/puppet.rb
+++ b/spec/unit/application/puppet.rb
@@ -173,6 +173,9 @@ describe "Puppet" do
         describe "the main command" do
             before :each do
                 Puppet.stubs(:[])
+                Puppet.settings.stubs(:use)
+                Puppet.stubs(:[]).with(:prerun_command).returns ""
+                Puppet.stubs(:[]).with(:postrun_command).returns ""
                 Puppet.stubs(:[]).with(:trace).returns(true)
 
                 @puppet.options.stubs(:[])
@@ -277,6 +280,16 @@ describe "Puppet" do
                 @puppet.main
             end
 
+            it "should call the prerun and postrun commands on a Configurer instance" do
+                configurer = stub 'configurer'
+
+                Puppet::Configurer.expects(:new).returns configurer
+                configurer.expects(:execute_prerun_command)
+                configurer.expects(:execute_postrun_command)
+
+                @puppet.main
+            end
+
             it "should apply the catalog" do
                 @catalog.expects(:apply)
 
diff --git a/spec/unit/configurer.rb b/spec/unit/configurer.rb
index cd5102546..32c0772ec 100755
--- a/spec/unit/configurer.rb
+++ b/spec/unit/configurer.rb
@@ -7,6 +7,11 @@ require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/configurer'
 
 describe Puppet::Configurer do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Configurer.new
+    end
+
     it "should include the Plugin Handler module" do
         Puppet::Configurer.ancestors.should be_include(Puppet::Configurer::PluginHandler)
     end
@@ -19,6 +24,52 @@ describe Puppet::Configurer do
         Puppet.settings.expects(:value).with(:puppetdlockfile).returns("/my/lock")
         Puppet::Configurer.lockfile_path.should == "/my/lock"
     end
+
+    describe "when executing a pre-run hook" do
+        it "should do nothing if the hook is set to an empty string" do
+            Puppet.settings[:prerun_command] = ""
+            Puppet::Util.expects(:exec).never
+
+            @agent.execute_prerun_command
+        end
+
+        it "should execute any pre-run command provided via the 'prerun_command' setting" do
+            Puppet.settings[:prerun_command] = "/my/command"
+            Puppet::Util.expects(:execute).with { |args| args[0] == "/my/command" }
+
+            @agent.execute_prerun_command
+        end
+
+        it "should fail if the command fails" do
+            Puppet.settings[:prerun_command] = "/my/command"
+            Puppet::Util.expects(:execute).raises Puppet::ExecutionFailure
+
+            lambda { @agent.execute_prerun_command }.should raise_error(Puppet::Configurer::CommandHookError)
+        end
+    end
+
+    describe "when executing a post-run hook" do
+        it "should do nothing if the hook is set to an empty string" do
+            Puppet.settings[:postrun_command] = ""
+            Puppet::Util.expects(:exec).never
+
+            @agent.execute_postrun_command
+        end
+
+        it "should execute any post-run command provided via the 'postrun_command' setting" do
+            Puppet.settings[:postrun_command] = "/my/command"
+            Puppet::Util.expects(:execute).with { |args| args[0] == "/my/command" }
+
+            @agent.execute_postrun_command
+        end
+
+        it "should fail if the command fails" do
+            Puppet.settings[:postrun_command] = "/my/command"
+            Puppet::Util.expects(:execute).raises Puppet::ExecutionFailure
+
+            lambda { @agent.execute_postrun_command }.should raise_error(Puppet::Configurer::CommandHookError)
+        end
+    end
 end
 
 describe Puppet::Configurer, "when executing a catalog run" do
@@ -74,6 +125,12 @@ describe Puppet::Configurer, "when executing a catalog run" do
         catalog.expects(:apply).never # because we're not yielding
         @agent.run
     end
+
+    it "should execute post-run hooks after the run" do
+        @agent.expects(:execute_postrun_command)
+
+        @agent.run
+    end
 end
 
 describe Puppet::Configurer, "when retrieving a catalog" do
@@ -213,6 +270,9 @@ describe Puppet::Configurer, "when preparing for a run" do
         Puppet.settings.stubs(:use).returns(true)
         @agent = Puppet::Configurer.new
         @agent.stubs(:dostorage)
+        @agent.stubs(:download_fact_plugins)
+        @agent.stubs(:download_plugins)
+        @agent.stubs(:execute_prerun_command)
         @facts = {"one" => "two", "three" => "four"}
     end
 
@@ -223,16 +283,19 @@ describe Puppet::Configurer, "when preparing for a run" do
     end
 
     it "should download fact plugins" do
-        @agent.stubs(:dostorage)
         @agent.expects(:download_fact_plugins)
 
         @agent.prepare
     end
 
     it "should download plugins" do
-        @agent.stubs(:dostorage)
         @agent.expects(:download_plugins)
 
         @agent.prepare
     end
+
+    it "should perform the pre-run commands" do
+        @agent.expects(:execute_prerun_command)
+        @agent.prepare
+    end
 end