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>

3 files changed, 64 insertions, 28 deletions

diff --git a/spec/unit/application/puppet.rb b/spec/unit/application/puppet.rb
index f4d606535..8fb364cdd 100755
--- a/spec/unit/application/puppet.rb
+++ b/spec/unit/application/puppet.rb
@@ -283,52 +283,37 @@ describe "Puppet" do
                 @puppet.main
             end
 
-            it "should generate a report if not noop" do
-                Puppet.stubs(:[]).with(:noop).returns(false)
-                @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
-                metrics = stub 'metrics', :[] => { :total => 10, :failed => 0}
-                report = stub 'report', :metrics => metrics
-                @transaction.stubs(:report).returns(report)
-
-                @transaction.expects(:generate_report)
-
-                @puppet.main
-            end
-
             describe "with detailed_exitcodes" do
-                before :each do
+                it "should exit with report's computed exit status" do
                     Puppet.stubs(:[]).with(:noop).returns(false)
                     @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
-                end
-
-                it "should exit with exit code of 2 if changes" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 1}, "resources" => {:failed => 0} }
-                    @transaction.stubs(:generate_report).returns(report)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(2)
+                    @puppet.expects(:exit).with(666)
 
                     @puppet.main
                 end
 
-                it "should exit with exit code of 4 if failures" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 0}, "resources" => {:failed => 1} }
-                    @transaction.stubs(:generate_report).returns(report)
+                it "should always exit with 0 if option is disabled" do
+                    Puppet.stubs(:[]).with(:noop).returns(false)
+                    @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(false)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(4)
+                    @puppet.expects(:exit).with(0)
 
                     @puppet.main
                 end
 
-                it "should exit with exit code of 6 if changes and failures" do
-                    report = stub 'report', :metrics => { "changes" => {:total => 1}, "resources" => {:failed => 1} }
-                    @transaction.stubs(:generate_report).returns(report)
+                it "should always exit with 0 if --noop" do
+                    Puppet.stubs(:[]).with(:noop).returns(true)
+                    @puppet.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
+                    report = stub 'report', :exit_status => 666
                     @transaction.stubs(:report).returns(report)
-                    @puppet.expects(:exit).with(6)
+                    @puppet.expects(:exit).with(0)
 
                     @puppet.main
                 end
             end
-
         end
 
         describe "the 'apply' command" do
diff --git a/spec/unit/application/puppetd.rb b/spec/unit/application/puppetd.rb
index d34ec9f2f..380608506 100755
--- a/spec/unit/application/puppetd.rb
+++ b/spec/unit/application/puppetd.rb
@@ -205,6 +205,10 @@ describe "puppetd" do
                 @puppetd.options.expects(:[]=).with(:onetime,true)
                 @puppetd.setup_test
             end
+            it "should set options[:detailed_exitcodes] to true" do
+                @puppetd.options.expects(:[]=).with(:detailed_exitcodes,true)
+                @puppetd.setup_test
+            end
             it "should set waitforcert to 0" do
                 @puppetd.options.expects(:[]=).with(:waitforcert,0)
                 @puppetd.setup_test
@@ -453,6 +457,7 @@ describe "puppetd" do
 
             before :each do
                 @puppetd.options.stubs(:[]).with(:client).returns(:client)
+                @puppetd.options.stubs(:[]).with(:detailed_exitcodes).returns(false)
                 @puppetd.stubs(:exit).with(0)
                 Puppet.stubs(:newservice)
             end
@@ -484,6 +489,29 @@ describe "puppetd" do
                 @puppetd.onetime
             end
 
+            describe "and --detailed-exitcodes" do
+                before :each do
+                    @puppetd.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
+                end
+
+                it "should exit with report's computed exit status" do
+                    Puppet.stubs(:[]).with(:noop).returns(false)
+                    report = stub 'report', :exit_status => 666
+                    @agent.stubs(:run).returns(report)
+                    @puppetd.expects(:exit).with(666)
+
+                    @puppetd.onetime
+                end
+
+                it "should always exit with 0 if --noop" do
+                    Puppet.stubs(:[]).with(:noop).returns(true)
+                    report = stub 'report', :exit_status => 666
+                    @agent.stubs(:run).returns(report)
+                    @puppetd.expects(:exit).with(0)
+
+                    @puppetd.onetime
+                end
+            end
         end
 
         describe "without --onetime" do
diff --git a/spec/unit/transaction/report.rb b/spec/unit/transaction/report.rb
index 12765735b..7cd5e1451 100755
--- a/spec/unit/transaction/report.rb
+++ b/spec/unit/transaction/report.rb
@@ -38,3 +38,26 @@ describe Puppet::Transaction::Report, " when being indirect" do
         Puppet::Util::Cacher.expire
     end
 end
+
+describe Puppet::Transaction::Report, " when computing exit status" do
+    it "should compute 2 if changes present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 1})
+        report.newmetric("resources", {:failed => 0})
+        report.exit_status.should == 2
+    end
+
+    it "should compute 4 if failures present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 0})
+        report.newmetric("resources", {:failed => 1})
+        report.exit_status.should == 4
+    end
+
+    it "should compute 6 if both changes and present" do
+        report = Puppet::Transaction::Report.new
+        report.newmetric("changes", {:total => 1})
+        report.newmetric("resources", {:failed => 1})
+        report.exit_status.should == 6
+    end
+end