Fix #1934 - detailed-exitcodes for puppetd

This option only works when --onetime is specified, as it doesn't make
much sense to worry about exit codes in the context of a long-running
daemon.

This required a refactoring of the existing --detailed-exitcodes code,
as "puppetd" wasn't directly creating a transaction object (like
"puppet" does).

Added Report::exit_status, which did what was previously hard-coded
into the "puppet" executable.

An Agent's "run" method now returns a value (the result of the
individual client class' "run" method)

The "puppetd" agent's "run" method now returns a transaction report, as
that seems like the logical thing to return as the result of applying a
catalog.

Signed-off-by: Deepak Giridharagopal <deepak@brownman.org>

5 files changed, 36 insertions, 10 deletions

diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index a188df39e..7bd97415f 100644
--- a/lib/puppet/agent.rb
+++ b/lib/puppet/agent.rb
@@ -48,14 +48,16 @@ class Puppet::Agent
             return
         end
         splay
+        result = nil
         with_client do |client|
             begin
-                sync.synchronize { lock { client.run(*args) } }
+                sync.synchronize { lock { result = client.run(*args) } }
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
                 Puppet.err "Could not run %s: %s" % [client_class, detail]
             end
         end
+        result
     end
 
     def stop
diff --git a/lib/puppet/application/puppet.rb b/lib/puppet/application/puppet.rb
index 6fdd5a0c6..b4c06d279 100644
--- a/lib/puppet/application/puppet.rb
+++ b/lib/puppet/application/puppet.rb
@@ -127,13 +127,12 @@ Puppet::Application.new(:puppet) do
             # And apply it
             transaction = catalog.apply
 
-            status = 0
             if not Puppet[:noop] and options[:detailed_exitcodes] then
                 transaction.generate_report
-                status |= 2 if transaction.report.metrics["changes"][:total] > 0
-                status |= 4 if transaction.report.metrics["resources"][:failed] > 0
+                exit(transaction.report.exit_status)
+            else
+                exit(0)
             end
-            exit(status)
         rescue => detail
             if Puppet[:trace]
                 puts detail.backtrace
diff --git a/lib/puppet/application/puppetd.rb b/lib/puppet/application/puppetd.rb
index 4799d5571..c1f733183 100644
--- a/lib/puppet/application/puppetd.rb
+++ b/lib/puppet/application/puppetd.rb
@@ -21,6 +21,7 @@ Puppet::Application.new(:puppetd) do
         {
             :waitforcert => 120,  # Default to checking for certs every 5 minutes
             :onetime => false,
+            :detailed_exitcodes => false,
             :verbose => false,
             :debug => false,
             :centrallogs => false,
@@ -65,6 +66,10 @@ Puppet::Application.new(:puppetd) do
         options[:waitforcert] = 0 unless @explicit_waitforcert
     end
 
+    option("--detailed-exitcodes") do |arg|
+        options[:detailed_exitcodes] = true
+    end
+
     option("--logdest DEST", "-l DEST") do |arg|
         begin
             Puppet::Util::Log.newdestination(arg)
@@ -95,19 +100,25 @@ Puppet::Application.new(:puppetd) do
         unless options[:client]
             $stderr.puts "onetime is specified but there is no client"
             exit(43)
+            return
         end
 
         @daemon.set_signal_traps
 
         begin
-            @agent.run
+            report = @agent.run
         rescue => detail
             if Puppet[:trace]
                 puts detail.backtrace
             end
             Puppet.err detail.to_s
         end
-        exit(0)
+
+        if not Puppet[:noop] and options[:detailed_exitcodes] then
+            exit(report.exit_status)
+        else
+            exit(0)
+        end
     end
 
     command(:main) do
@@ -125,6 +136,7 @@ Puppet::Application.new(:puppetd) do
         Puppet.settings.handlearg("--no-daemonize")
         options[:verbose] = true
         options[:onetime] = true
+        options[:detailed_exitcodes] = true
         options[:waitforcert] = 0
     end
 
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index efda545c5..b63df0754 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -144,13 +144,17 @@ class Puppet::Configurer
 
         begin
             benchmark(:notice, "Finished catalog run") do
-                catalog.apply(options)
+                transaction = catalog.apply(options)
+                transaction.generate_report
+                report = transaction.report
             end
+            report
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
             Puppet.err "Failed to apply catalog: %s" % detail
+            return
         end
-
+    ensure
         # Now close all of our existing http connections, since there's no
         # reason to leave them lying open.
         Puppet::Network::HttpPool.clear_http_instances
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index e5b8650bb..6a486335b 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -83,5 +83,14 @@ class Puppet::Transaction::Report
         end
         return ret
     end
-end
 
+    # Based on the contents of this report's metrics, compute a single number
+    # that represents the report. The resulting number is a bitmask where
+    # individual bits represent the presence of different metrics.
+    def exit_status
+        status = 0
+        status |= 2 if @metrics["changes"][:total] > 0
+        status |= 4 if @metrics["resources"][:failed] > 0
+        return status
+    end
+end