(#7157) Return a non-zero exit code on face failure.

When a face or action fails we should exit non-zero on the CLI to signal this
to our caller.  "Fails" is defined as "raises an exception"; we don't treat
any return value as a significant failure.

Reviewed-By: Jesse Wolf <jesse@puppetlabs.com>

3 files changed, 79 insertions, 12 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index 7bebd18bb..257d51f75 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -27,13 +27,6 @@ class Puppet::Application::FaceBase < Puppet::Application
 
 
   attr_accessor :face, :action, :type, :arguments, :render_as
-  attr_writer :exit_code
-
-  # This allows you to set the exit code if you don't want to just exit
-  # immediately but you need to indicate a failure.
-  def exit_code
-    @exit_code || 0
-  end
 
   def render(result)
     format = render_as || action.render_as || :for_humans
@@ -198,19 +191,28 @@ class Puppet::Application::FaceBase < Puppet::Application
 
 
   def main
+    status = false
+
     # Call the method associated with the provided action (e.g., 'find').
     if @action
-      result = @face.send(@action.name, *arguments)
-      puts render(result) unless result.nil?
+      begin
+        result = @face.send(@action.name, *arguments)
+        puts render(result) unless result.nil?
+        status = true
+      rescue Exception => detail
+        puts detail.backtrace if Puppet[:trace]
+        Puppet.err detail.to_s
+      end
     else
       if arguments.first.is_a? Hash
-        puts "#{@face} does not have a default action"
+        puts "#{face} does not have a default action"
       else
-        puts "#{@face} does not respond to action #{arguments.first}"
+        puts "#{face} does not respond to action #{arguments.first}"
       end
 
       puts Puppet::Face[:help, :current].help(@face.name, *arguments)
     end
-    exit(exit_code)
+
+    exit status
   end
 end
diff --git a/spec/lib/puppet/face/basetest.rb b/spec/lib/puppet/face/basetest.rb
index 9a658b685..a98bc382f 100755
--- a/spec/lib/puppet/face/basetest.rb
+++ b/spec/lib/puppet/face/basetest.rb
@@ -1,3 +1,33 @@
+require 'puppet/face'
+
 Puppet::Face.define(:basetest, '0.0.1') do
   summary "This is just so tests don't fail"
+
+  option "--[no-]boolean"
+  option "--mandatory ARGUMENT"
+
+  action :foo do
+    option("--action")
+    when_invoked do |*args| args.length end
+  end
+
+  action :return_true do
+    summary "just returns true"
+    when_invoked do |options| true end
+  end
+
+  action :return_false do
+    summary "just returns false"
+    when_invoked do |options| false end
+  end
+
+  action :return_nil do
+    summary "just returns nil"
+    when_invoked do |options| nil end
+  end
+
+  action :raise do
+    summary "just raises an exception"
+    when_invoked do |options| raise ArgumentError, "your failure" end
+  end
 end
diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index e6e598430..7b3a69f61 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -201,6 +201,41 @@ describe Puppet::Application::FaceBase do
     end
   end
 
+  describe "error reporting" do
+    before :each do
+      app.stubs(:puts)          # don't dump text to screen.
+
+      app.face      = Puppet::Face[:basetest, '0.0.1']
+      app.arguments = []
+    end
+
+    it "should exit 0 when the action returns true" do
+      app.action = app.face.get_action :return_true
+      expect { app.main }.to exit_with 0
+    end
+
+    it "should exit 0 when the action returns false" do
+      app.action = app.face.get_action :return_false
+      expect { app.main }.to exit_with 0
+    end
+
+    it "should exit 0 when the action returns nil" do
+      app.action = app.face.get_action :return_nil
+      expect { app.main }.to exit_with 0
+    end
+
+    it "should exit non-0 when the action raises" do
+      app.action = app.face.get_action :return_raise
+      expect { app.main }.not_to exit_with 0
+    end
+
+    it "should exit non-0 when the action does not exist" do
+      app.action = nil
+      app.arguments = ["foo"]
+      expect { app.main }.to exit_with 1
+    end
+  end
+
   describe "#render" do
     before :each do
       app.face   = Puppet::Face[:basetest, '0.0.1']