Merge branch 'refactor/2.7.x/8561-and-7290-improved-option-handling' into 2.7.x

4 files changed, 109 insertions, 51 deletions

diff --git a/lib/puppet/indirector/face.rb b/lib/puppet/indirector/face.rb
index ead3f4b46..adb6b688b 100644
--- a/lib/puppet/indirector/face.rb
+++ b/lib/puppet/indirector/face.rb
@@ -48,16 +48,26 @@ class Puppet::Indirector::Face < Puppet::Face
     return result
   end
 
+  option "--extra HASH" do
+    summary "Extra arguments to pass to the indirection request"
+    description <<-end
+      A terminus can take additional arguments to refine the operation, which
+      are passed as an arbitrary hash to the back-end.  Anything passed as
+      the extra value is just send direct to the back-end.
+    end
+    default_to do Hash.new end
+  end
+
   action :destroy do
     summary "Delete an object."
     arguments "<key>"
-    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
+    when_invoked {|key, options| call_indirection_method :destroy, key, options[:extra] }
   end
 
   action :find do
     summary "Retrieve an object by name."
     arguments "<key>"
-    when_invoked { |key, options| call_indirection_method(:find, key, options) }
+    when_invoked {|key, options| call_indirection_method :find, key, options[:extra] }
   end
 
   action :save do
@@ -68,13 +78,13 @@ class Puppet::Indirector::Face < Puppet::Face
       currently accept data from STDIN, save actions cannot currently be invoked
       from the command line.
     EOT
-    when_invoked { |key, options| call_indirection_method(:save, key, options) }
+    when_invoked {|key, options| call_indirection_method :save, key, options[:extra] }
   end
 
   action :search do
     summary "Search for an object or retrieve multiple objects."
     arguments "<query>"
-    when_invoked { |key, options| call_indirection_method(:search, key, options) }
+    when_invoked {|key, options| call_indirection_method :search, key, options[:extra] }
   end
 
   # Print the configuration for the current terminus class
@@ -86,11 +96,11 @@ class Puppet::Indirector::Face < Puppet::Face
       run mode with the '--mode' option.
     EOT
 
-    when_invoked do |*args|
+    when_invoked do |options|
       if t = indirection.terminus_class
-        puts "Run mode '#{Puppet.run_mode.name}': #{t}"
+        "Run mode '#{Puppet.run_mode.name}': #{t}"
       else
-        $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
+        "No default terminus class for run mode '#{Puppet.run_mode.name}'"
       end
     end
   end
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index ce9c60b49..bd47a36ea 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -196,15 +196,12 @@ class Puppet::Interface::Action
     wrapper = <<WRAPPER
 def #{@name}(#{decl.join(", ")})
   #{optn}
-  args = #{args}
-  options = args.last
-
-  action = get_action(#{name.inspect})
-  action.add_default_args(args)
-  action.validate_args(args)
-  __invoke_decorations(:before, action, args, options)
+  args    = #{args}
+  action  = get_action(#{name.inspect})
+  args   << action.validate_and_clean(args.pop)
+  __invoke_decorations(:before, action, args, args.last)
   rval = self.__send__(#{internal_name.inspect}, *args)
-  __invoke_decorations(:after, action, args, options)
+  __invoke_decorations(:after, action, args, args.last)
   return rval
 end
 WRAPPER
@@ -254,35 +251,59 @@ WRAPPER
     option
   end
 
-  def add_default_args(args)
-    options.map {|x| get_option(x) }.each do |option|
-      if option.has_default? and not option.aliases.any? {|x| args.last.has_key? x}
-        args.last[option.name] = option.default
+  def validate_and_clean(original)
+    # The final set of arguments; effectively a hand-rolled shallow copy of
+    # the original, which protects the caller from the surprises they might
+    # get if they passed us a hash and we mutated it...
+    result = {}
+
+    # Check for multiple aliases for the same option, and canonicalize the
+    # name of the argument while we are about it.
+    overlap = Hash.new do |h, k| h[k] = [] end
+    unknown = []
+    original.keys.each do |name|
+      if option = get_option(name) then
+        canonical = option.name
+        if result.has_key? canonical
+          overlap[canonical] << name
+        else
+          result[canonical] = original[name]
+        end
+      else
+        unknown << name
       end
     end
-  end
 
-  def validate_args(args)
-    # Check for multiple aliases for the same option...
-    args.last.keys.each do |name|
-      # #7290: If this isn't actually an option, ignore it for now.  We should
-      # probably fail, but that wasn't our API, and I don't want to perturb
-      # behaviour this late in the RC cycle. --daniel 2011-04-29
-      if option = get_option(name) then
-        overlap = (option.aliases & args.last.keys)
-        unless overlap.length == 1 then
-          raise ArgumentError, "Multiple aliases for the same option passed: #{overlap.join(', ')}"
-        end
+    unless overlap.empty?
+      msg = overlap.map {|k, v| "(#{k}, #{v.sort.join(', ')})" }.join(", ")
+      raise ArgumentError, "Multiple aliases for the same option passed: #{msg}"
+    end
+
+    unless unknown.empty?
+      msg = unknown.sort.join(", ")
+      raise ArgumentError, "Unknown options passed: #{msg}"
+    end
+
+    # Inject default arguments and check for missing mandating options.
+    missing = []
+    options.map {|x| get_option(x) }.each do |option|
+      name = option.name
+      next if result.has_key? name
+
+      if option.has_default?
+        result[name] = option.default
+      elsif option.required?
+        missing << name
       end
     end
 
-    # Check for missing mandatory options.
-    required = options.map do |name|
-      get_option(name)
-    end.select(&:required?).collect(&:name) - args.last.keys
+    unless missing.empty?
+      msg = missing.sort.join(', ')
+      raise ArgumentError, "The following options are required: #{msg}"
+    end
 
-    return if required.empty?
-    raise ArgumentError, "The following options are required: #{required.join(', ')}"
+    # All done.
+    return result
   end
 
   ########################################################################
diff --git a/spec/unit/indirector/face_spec.rb b/spec/unit/indirector/face_spec.rb
index 943ff7991..8e324f019 100755
--- a/spec/unit/indirector/face_spec.rb
+++ b/spec/unit/indirector/face_spec.rb
@@ -12,6 +12,8 @@ describe Puppet::Indirector::Face do
     instance
   end
 
+  it { should be_option :extra }
+
   it "should be able to return a list of indirections" do
     Puppet::Indirector::Face.indirections.should be_include("catalog")
   end
@@ -48,7 +50,7 @@ describe Puppet::Indirector::Face do
     end
     it "should forward passed options" do
       subject.indirection.expects(method).with(:test, {'one'=>'1'})
-      subject.send(method, :test, {'one'=>'1'})
+      subject.send(method, :test, :extra => {'one'=>'1'})
     end
   end
 
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index d71a7d000..c3f08e817 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -121,6 +121,7 @@ describe Puppet::Interface::Action do
       let :face do
         Puppet::Interface.new(:ruby_api, '1.0.0') do
           action :bar do
+            option "--bar"
             when_invoked do |*args|
               args.last
             end
@@ -138,8 +139,8 @@ describe Puppet::Interface::Action do
         options.should == { :bar => "beer" }
       end
 
-      it "should call #validate_args on the action when invoked" do
-        face.get_action(:bar).expects(:validate_args).with([1, :two, 'three', {}])
+      it "should call #validate_and_clean on the action when invoked" do
+        face.get_action(:bar).expects(:validate_and_clean).with({}).returns({})
         face.bar 1, :two, 'three'
       end
     end
@@ -171,14 +172,28 @@ describe Puppet::Interface::Action do
       face.get_action(:foo).options.should =~ [:bar]
     end
 
-    it "should only list options and not aliases" do
-      face = Puppet::Interface.new(:action_level_options, '0.0.1') do
-        action :foo do
-          when_invoked do |options| true end
-          option "--bar", "-b", "--foo-bar"
+    describe "option aliases" do
+      let :option do action.get_option :bar end
+      let :action do face.get_action :foo end
+      let :face do
+        Puppet::Interface.new(:action_level_options, '0.0.1') do
+          action :foo do
+            when_invoked do |options| options end
+            option "--bar", "--foo", "-b"
+          end
+        end
+      end
+
+      it "should only list options and not aliases" do
+        action.options.should =~ [:bar]
+      end
+
+      it "should use the canonical option name when passed aliases" do
+        name = option.name
+        option.aliases.each do |input|
+          face.foo(input => 1).should == { name => 1 }
         end
       end
-      face.get_action(:foo).options.should =~ [:bar]
     end
 
     describe "with both face and action options" do
@@ -436,12 +451,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :child_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :child_before ]
         end
       end
@@ -453,12 +468,12 @@ describe Puppet::Interface::Action do
         end
 
         it "should be invoked when calling a child action" do
-          subject.on_child(:foo => true, :bar => true).should == :on_child
+          subject.on_child(:foo => true).should == :on_child
           subject.reported.should == [ :parent_before ]
         end
 
         it "should be invoked when calling a parent action" do
-          subject.on_parent(:foo => true, :bar => true).should == :on_parent
+          subject.on_parent(:foo => true).should == :on_parent
           subject.reported.should == [ :parent_before ]
         end
       end
@@ -534,7 +549,7 @@ describe Puppet::Interface::Action do
     it "should return the block if asked"
   end
 
-  context "#validate_args" do
+  context "#validate_and_clean" do
     subject do
       Puppet::Interface.new(:validate_args, '1.0.0') do
         script :test do |options| true end
@@ -551,6 +566,16 @@ describe Puppet::Interface::Action do
       expect { subject.test :foo => true, :f => true }.
         to raise_error ArgumentError, /Multiple aliases for the same option/
     end
+
+    it "should fail if an unknown option is passed" do
+      expect { subject.test :unknown => true }.
+        to raise_error ArgumentError, /Unknown options passed: unknown/
+    end
+
+    it "should report all the unknown options passed" do
+      expect { subject.test :unknown => true, :unseen => false }.
+        to raise_error ArgumentError, /Unknown options passed: unknown, unseen/
+    end
   end
 
   context "default option values" do