From 618495c7b70671ff1da1653c76d1524ff3f615b7 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 31 May 2011 15:02:41 -0700
Subject: #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.
---
 lib/puppet/application/face_base.rb | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

(limited to 'lib')

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
-- 
cgit 


From dd8108cd8f48d399ab728c3437e3fef38f0eb46c Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 31 May 2011 15:05:54 -0700
Subject: #7211: nasty logic error with global Face options taking arguments.

A logic error meant that global Face options that took arguments were
mishandled: we never consumed the argument, so we read this:

    puppet facts --render-as json find $(hostname)

...as meaning "invoke the 'json' action on the 'facts' face"...

This fixes that problem, so we now correctly handle both optional and
non-optional arguments to global Face options.
---
 lib/puppet/application/face_base.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'lib')

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index 2f07faae1..ea5ba4aaf 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -91,7 +91,7 @@ class Puppet::Application::FaceBase < Puppet::Application
             index += 1          # ...so skip the argument.
           end
         elsif option = find_application_argument(item) then
-          index += 1 if (option[:argument] and option[:optional])
+          index += 1 if (option[:argument] and not option[:optional])
         else
           raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
         end
-- 
cgit