Merge branch 'bug/2.7.x/7122-puppet-facts-find-raises' into 2.7.x

10 files changed, 192 insertions, 66 deletions

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index d02769412..69c3ad5ad 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -214,12 +214,55 @@ class Puppet::Application::FaceBase < Puppet::Application
     # Call the method associated with the provided action (e.g., 'find').
     if @action
       begin
+        # We need to do arity checking here because this is generic code
+        # calling generic methods – that have argument defaulting.  We need to
+        # make sure we don't accidentally pass the options as the first
+        # argument to a method that takes one argument.  eg:
+        #
+        #   puppet facts find
+        #   => options => {}
+        #      @arguments => [{}]
+        #   => @face.send :bar, {}
+        #
+        #   def face.bar(argument, options = {})
+        #   => bar({}, {})  # oops!  we thought the options were the
+        #                   # positional argument!!
+        #
+        # We could also fix this by making it mandatory to pass the options on
+        # every call, but that would make the Ruby API much more annoying to
+        # work with; having the defaulting is a much nicer convention to have.
+        #
+        # We could also pass the arguments implicitly, by having a magic
+        # 'options' method that was visible in the scope of the action, which
+        # returned the right stuff.
+        #
+        # That sounds attractive, but adds complications to all sorts of
+        # things, especially when you think about how to pass options when you
+        # are writing Ruby code that calls multiple faces.  Especially if
+        # faces are involved in that. ;)
+        #
+        # --daniel 2011-04-27
+        if (arity = @action.positional_arg_count) > 0
+          unless (count = arguments.length) == arity then
+            raise ArgumentError, "wrong number of arguments (#{count} for #{arity})"
+          end
+        end
+
         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
+
+        case detail
+        when ArgumentError then
+          got, want = /\((\d+) for (\d+)\)/.match(detail.to_s).to_a.map {|x| x.to_i }
+          Puppet.err "puppet #{@face.name} #{@action.name}: #{want} argument expected but #{got} given"
+          Puppet.err "Try 'puppet help #{@face.name} #{@action.name}' for usage"
+
+        else # generic exception handling, alas.
+          Puppet.err detail.to_s
+        end
       end
     else
       puts "#{face} does not respond to action #{arguments.first}"
diff --git a/lib/puppet/face/indirector.rb b/lib/puppet/face/indirector.rb
index a7ff7e1f0..16ffcd311 100644
--- a/lib/puppet/face/indirector.rb
+++ b/lib/puppet/face/indirector.rb
@@ -25,11 +25,9 @@ that we should describe in this file somehow.
     Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
   end
 
-  def call_indirection_method(method, *args)
-    options = args.last
-
+  def call_indirection_method(method, key, options)
     begin
-      result = indirection.__send__(method, *args)
+      result = indirection.__send__(method, key, options)
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
       raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
@@ -39,19 +37,19 @@ that we should describe in this file somehow.
   end
 
   action :destroy do
-    when_invoked { |*args| call_indirection_method(:destroy, *args) }
+    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
   end
 
   action :find do
-    when_invoked { |*args| call_indirection_method(:find, *args) }
+    when_invoked { |key, options| call_indirection_method(:find, key, options) }
   end
 
   action :save do
-    when_invoked { |*args| call_indirection_method(:save, *args) }
+    when_invoked { |key, options| call_indirection_method(:save, key, options) }
   end
 
   action :search do
-    when_invoked { |*args| call_indirection_method(:search, *args) }
+    when_invoked { |key, options| call_indirection_method(:search, key, options) }
   end
 
   # Print the configuration for the current terminus class
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index ac66d2946..9c9741b52 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -168,11 +168,12 @@ class Puppet::Interface::Action
   # this stuff work, because it would have been cleaner.  Which gives you an
   # idea how motivated we were to make this cleaner.  Sorry.
   # --daniel 2011-03-31
+  attr_reader :positional_arg_count
   def when_invoked=(block)
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
 
-    arity = block.arity
+    arity = @positional_arg_count = block.arity
     if arity == 0 then
       # This will never fire on 1.8.7, which treats no arguments as "*args",
       # but will on 1.9.2, which treats it as "no arguments".  Which bites,
diff --git a/spec/lib/puppet_spec/matchers.rb b/spec/lib/puppet_spec/matchers.rb
new file mode 100644
index 000000000..77f580330
--- /dev/null
+++ b/spec/lib/puppet_spec/matchers.rb
@@ -0,0 +1,87 @@
+require 'stringio'
+
+########################################################################
+# Backward compatibility for Jenkins outdated environment.
+module RSpec
+  module Matchers
+    module BlockAliases
+      alias_method :to,     :should      unless method_defined? :to
+      alias_method :to_not, :should_not  unless method_defined? :to_not
+      alias_method :not_to, :should_not  unless method_defined? :not_to
+    end
+  end
+end
+
+
+########################################################################
+# Custom matchers...
+RSpec::Matchers.define :have_matching_element do |expected|
+  match do |actual|
+    actual.any? { |item| item =~ expected }
+  end
+end
+
+
+RSpec::Matchers.define :exit_with do |expected|
+  actual = nil
+  match do |block|
+    begin
+      block.call
+    rescue SystemExit => e
+      actual = e.status
+    end
+    actual and actual == expected
+  end
+  failure_message_for_should do |block|
+    "expected exit with code #{expected} but " +
+      (actual.nil? ? " exit was not called" : "we exited with #{actual} instead")
+  end
+  failure_message_for_should_not do |block|
+    "expected that exit would not be called with #{expected}"
+  end
+  description do
+    "expect exit with #{expected}"
+  end
+end
+
+
+RSpec::Matchers.define :have_printed do |expected|
+  match do |block|
+    $stderr = $stdout = StringIO.new
+
+    begin
+      block.call
+    ensure
+      $stdout.rewind
+      @actual = $stdout.read
+
+      $stdout = STDOUT
+      $stderr = STDERR
+    end
+
+    if @actual then
+      case expected
+      when String
+        @actual.include? expected
+      when Regexp
+        expected.match @actual
+      else
+        raise ArgumentError, "No idea how to match a #{@actual.class.name}"
+      end
+    end
+  end
+
+  failure_message_for_should do |actual|
+    if actual.nil? then
+      "expected #{expected.inspect}, but nothing was printed"
+    else
+      "expected #{expected.inspect} to be printed; got:\n#{actual}"
+    end
+  end
+
+  description do
+    "expect #{expected.inspect} to be printed"
+  end
+
+  diffable
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index a01ae56cd..6b6b1c2fb 100755
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -17,9 +17,10 @@ end
 require 'pathname'
 require 'tmpdir'
 
-require 'lib/puppet_spec/verbose'
-require 'lib/puppet_spec/files'
-require 'lib/puppet_spec/fixtures'
+require 'puppet_spec/verbose'
+require 'puppet_spec/files'
+require 'puppet_spec/fixtures'
+require 'puppet_spec/matchers'
 require 'monkey_patches/alias_should_to_must'
 require 'monkey_patches/publicize_methods'
 require 'monkey_patches/disable_signal_trap'
@@ -74,43 +75,3 @@ RSpec.configure do |config|
     GC.enable
   end
 end
-
-RSpec::Matchers.define :have_matching_element do |expected|
-  match do |actual|
-    actual.any? { |item| item =~ expected }
-  end
-end
-
-RSpec::Matchers.define :exit_with do |expected|
-  actual = nil
-  match do |block|
-    begin
-      block.call
-    rescue SystemExit => e
-      actual = e.status
-    end
-    actual and actual == expected
-  end
-  failure_message_for_should do |block|
-    "expected exit with code #{expected} but " +
-      (actual.nil? ? " exit was not called" : "we exited with #{actual} instead")
-  end
-  failure_message_for_should_not do |block|
-    "expected that exit would not be called with #{expected}"
-  end
-  description do
-    "expect exit with #{expected}"
-  end
-end
-
-# Backward compatibility for Jenkins outdated environment.
-module RSpec
-  module Matchers
-    module BlockAliases
-      alias_method :to,     :should      unless method_defined? :to
-      alias_method :to_not, :should_not  unless method_defined? :to_not
-      alias_method :not_to, :should_not  unless method_defined? :not_to
-    end
-  end
-end
-
diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb
index 275702085..d8c970157 100755
--- a/spec/unit/application/face_base_spec.rb
+++ b/spec/unit/application/face_base_spec.rb
@@ -206,11 +206,11 @@ describe Puppet::Application::FaceBase do
 
       app.render_as = :json
       app.face      = Puppet::Face[:basetest, '0.0.1']
-      app.arguments = []
+      app.arguments = [{}]      # we always have options in there...
     end
 
     it "should exit 0 when the action returns true" do
-      app.action = app.face.get_action :return_true
+      app.action    = app.face.get_action :return_true
       expect { app.main }.to exit_with 0
     end
 
@@ -309,13 +309,15 @@ EOT
       Puppet::Face[:help, :current].expects(:help).never
 
       expect {
-        expect { app.setup; app.run }.to exit_with 1
-      }.to print(/I don't know how to render 'interpretive-dance'/)
+        expect { app.run }.to exit_with 1
+      }.to have_printed(/I don't know how to render 'interpretive-dance'/)
     end
 
     it "should work if asked to render a NetworkHandler format" do
-      app.command_line.stubs(:args).returns %w{facts find dummy --render-as yaml}
-      expect { app.parse_options; app.setup; app.run }.to exit_with 0
+      app.command_line.stubs(:args).returns %w{dummy find dummy --render-as yaml}
+      expect {
+        expect { app.run }.to exit_with 0
+      }.to have_printed(/--- 3/)
     end
   end
 end
diff --git a/spec/unit/application/facts_spec.rb b/spec/unit/application/facts_spec.rb
new file mode 100755
index 000000000..aed042675
--- /dev/null
+++ b/spec/unit/application/facts_spec.rb
@@ -0,0 +1,27 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/application/facts'
+
+describe Puppet::Application::Facts do
+  before :each do
+    subject.command_line.stubs(:subcommand_name).returns 'facts'
+  end
+
+  it "should fail if no key is given to find" do
+    subject.command_line.stubs(:args).returns %w{find}
+    expect {
+      expect { subject.run }.to exit_with 1
+    }.to have_printed /1 argument expected but 0 given/
+    @logs.first.to_s.should =~ /1 argument expected but 0 given/
+  end
+
+  it "should return facts if a key is given to find" do
+    subject.command_line.stubs(:args).returns %w{find whatever --render-as yaml}
+
+    expect {
+      expect { subject.run }.to exit_with 0
+    }.should have_printed(/object:Puppet::Node::Facts/)
+
+    @logs.should be_empty
+  end
+end
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 833fb424e..e0a9bebb4 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -28,10 +28,11 @@ describe Puppet::Application::IndirectionBase do
     Puppet::Indirector::Indirection.expects(:instance).
       with(:testindirection).returns(terminus)
 
-    subject.command_line.instance_variable_set('@args', %w{--terminus foo save})
+    subject.command_line.instance_variable_set('@args', %w{--terminus foo save bar})
 
     # Not a very nice thing. :(
     $stderr.stubs(:puts)
+    Puppet.stubs(:err)
 
     expect { subject.run }.to exit_with 0
   end
diff --git a/spec/unit/face/facts_spec.rb b/spec/unit/face/facts_spec.rb
index 06b229aa1..27b5b9e3d 100755
--- a/spec/unit/face/facts_spec.rb
+++ b/spec/unit/face/facts_spec.rb
@@ -9,9 +9,15 @@ describe Puppet::Face[:facts, '0.0.1'] do
 
   describe "when uploading" do
     it "should set the terminus_class to :facter"
+    it "should set the cache_class to :rest"
+    it "should find the current certname"
+  end
 
-    it "should set the cach_eclass to :rest"
+  describe "#find" do
+    it { should be_action :find }
 
-    it "should find the current certname"
+    it "should fail without a key" do
+      expect { subject.find }.to raise_error ArgumentError, /wrong number of arguments/
+    end
   end
 end
diff --git a/spec/unit/face/indirector_spec.rb b/spec/unit/face/indirector_spec.rb
index bb06fcfe2..e7dd44f3d 100755
--- a/spec/unit/face/indirector_spec.rb
+++ b/spec/unit/face/indirector_spec.rb
@@ -34,12 +34,12 @@ describe Puppet::Face::Indirector do
     end
 
     it "should call the indirection method with options when the '#{method}' action is invoked" do
-      subject.indirection.expects(method).with(:test, "myargs", {})
-      subject.send(method, :test, "myargs")
+      subject.indirection.expects(method).with(:test, {})
+      subject.send(method, :test)
     end
     it "should forward passed options" do
-      subject.indirection.expects(method).with(:test, "action", {'one'=>'1'})
-      subject.send(method, :test, 'action', {'one'=>'1'})
+      subject.indirection.expects(method).with(:test, {'one'=>'1'})
+      subject.send(method, :test, {'one'=>'1'})
     end
   end