#7211: more helpful error messages in various cases.

We were emitting a bunch of unhelpful failure messages, surrounding invalid
actions and especially default actions interacting with the command-line.

This cleans those up, to give a helpful, informative, and correct message in
all cases.  Notably, we no longer report that there is no "default" action
when you specify an unknown action on a face.

This change revealed some other weaknesses in our unit tests, now correctly,
that result in slightly more robust code.

3 files changed, 46 insertions, 10 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index 7a5ce3400..2f07faae1 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -66,9 +66,9 @@ class Puppet::Application::FaceBase < Puppet::Application
     # Now, walk the command line and identify the action.  We skip over
     # arguments based on introspecting the action and all, and find the first
     # non-option word to use as the action.
-    action = nil
-    index  = -1
-    until @action or (index += 1) >= command_line.args.length do
+    action_name = nil
+    index       = -1
+    until action_name or (index += 1) >= command_line.args.length do
       item = command_line.args[index]
       if item =~ /^-/ then
         option = @face.options.find do |name|
@@ -96,7 +96,11 @@ class Puppet::Application::FaceBase < Puppet::Application
           raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
         end
       else
-        @action = @face.get_action(item.to_sym)
+        # Stash away the requested action name for later, and try to fetch the
+        # action object it represents; if this is an invalid action name that
+        # will be nil, and handled later.
+        action_name = item.to_sym
+        @action = @face.get_action(action_name)
       end
     end
 
@@ -104,8 +108,18 @@ class Puppet::Application::FaceBase < Puppet::Application
       if @action = @face.get_default_action() then
         @is_default_action = true
       else
-        Puppet.err "#{face.name} does not have a default action, and no action was given"
-        Puppet.err Puppet::Face[:help, :current].help(@face.name)
+        # REVISIT: ...and this horror thanks to our log setup, which doesn't
+        # initialize destinations until the setup method, which we will never
+        # reach.  We could also just print here, but that is actually a little
+        # uglier and nastier in the long term, in which we should do log setup
+        # earlier if at all possible. --daniel 2011-05-31
+        Puppet::Util::Log.newdestination(:console)
+
+        face   = @face.name
+        action = action_name.nil? ? 'default' : "'#{action_name}'"
+        msg = "'#{face}' has no #{action} action.  See `puppet help #{face}`."
+        Puppet.err(msg)
+
         exit false
       end
     end
diff --git a/spec/lib/puppet/face/basetest.rb b/spec/lib/puppet/face/basetest.rb
index 9398f5b2d..e5c9a9dc5 100755
--- a/spec/lib/puppet/face/basetest.rb
+++ b/spec/lib/puppet/face/basetest.rb
@@ -38,4 +38,9 @@ Puppet::Face.define(:basetest, '0.0.1') do
     when_invoked do |options| "this is not the hook you are looking for" end
     when_rendering :s do |value| "you invoked the 's' rendering hook" end
   end
+
+  action :count_args do
+    summary "return the count of arguments given"
+    when_invoked do |*args| args.length - 1 end
+  end
 end
diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index 3318e061e..ae8c7733c 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -52,6 +52,12 @@ describe Puppet::Application::FaceBase do
       end
     end
 
+    it "should stop if the first thing found is not an action" do
+      app.command_line.stubs(:args).returns %w{banana count_args}
+      expect { app.run }.to exit_with 1
+      @logs.first.message.should =~ /has no 'banana' action/
+    end
+
     it "should use the default action if not given any arguments" do
       app.command_line.stubs(:args).returns []
       action = stub(:options => [], :render_as => nil)
@@ -77,7 +83,18 @@ describe Puppet::Application::FaceBase do
       Puppet::Face[:basetest, '0.0.1'].expects(:get_default_action).returns(nil)
       app.stubs(:main)
       expect { app.run }.to exit_with 1
-      @logs.first.message.should =~ /does not have a default action/
+      @logs.first.message.should =~ /has no 'bar' action./
+    end
+
+    [%w{something_I_cannot_do},
+     %w{something_I_cannot_do argument}].each do |input|
+      it "should report unknown actions nicely" do
+        app.command_line.stubs(:args).returns input
+        Puppet::Face[:basetest, '0.0.1'].expects(:get_default_action).returns(nil)
+        app.stubs(:main)
+        expect { app.run }.to exit_with 1
+        @logs.first.message.should =~ /has no 'something_I_cannot_do' action/
+      end
     end
 
     it "should report a sensible error when options with = fail" do
@@ -149,7 +166,7 @@ describe Puppet::Application::FaceBase do
     end
 
     it "should handle application-level options", :'fails_on_ruby_1.9.2' => true do
-      app.command_line.stubs(:args).returns %w{basetest --verbose return_true}
+      app.command_line.stubs(:args).returns %w{--verbose return_true}
       app.preinit
       app.parse_options
       app.face.name.should == :basetest
@@ -304,7 +321,7 @@ EOT
     end
 
     it "should fail early if asked to render an invalid format" do
-      app.command_line.stubs(:args).returns %w{--render-as interpretive-dance help help}
+      app.command_line.stubs(:args).returns %w{--render-as interpretive-dance return_true}
       # We shouldn't get here, thanks to the exception, and our expectation on
       # it, but this helps us fail if that slips up and all. --daniel 2011-04-27
       Puppet::Face[:help, :current].expects(:help).never
@@ -315,7 +332,7 @@ EOT
     end
 
     it "should work if asked to render a NetworkHandler format" do
-      app.command_line.stubs(:args).returns %w{dummy find dummy --render-as yaml}
+      app.command_line.stubs(:args).returns %w{count_args a b c --render-as yaml}
       expect {
         expect { app.run }.to exit_with 0
       }.to have_printed(/--- 3/)