Merge branch 'feature/2.7.x/7183-implement-invisible-glob-version-matching-for-faces' into 2.7.x

7 files changed, 125 insertions, 75 deletions

diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 5c8ade749..ced00863d 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -26,18 +26,13 @@ class Puppet::Interface
       Puppet::Interface::FaceCollection.faces
     end
 
-    def face?(name, version)
-      Puppet::Interface::FaceCollection.face?(name, version)
-    end
-
     def register(instance)
       Puppet::Interface::FaceCollection.register(instance)
     end
 
     def define(name, version, &block)
-      if face?(name, version)
-        face = Puppet::Interface::FaceCollection[name, version]
-      else
+      face = Puppet::Interface::FaceCollection[name, version]
+      if face.nil? then
         face = self.new(name, version)
         Puppet::Interface::FaceCollection.register(face)
         # REVISIT: Shouldn't this be delayed until *after* we evaluate the
@@ -50,10 +45,14 @@ class Puppet::Interface
       return face
     end
 
+    def face?(name, version)
+      Puppet::Interface::FaceCollection[name, version]
+    end
+
     def [](name, version)
       unless face = Puppet::Interface::FaceCollection[name, version]
         if current = Puppet::Interface::FaceCollection[name, :current]
-          raise Puppet::Error, "Could not find version #{version} of #{current}"
+          raise Puppet::Error, "Could not find version #{version} of #{name}"
         else
           raise Puppet::Error, "Could not find Puppet Face #{name.inspect}"
         end
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 591471d4b..6e6afc545 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -24,19 +24,21 @@ module Puppet::Interface::FaceCollection
         end
       end
     end
-    return @faces.keys
+    return @faces.keys.select {|name| @faces[name].length > 0 }
   end
 
   def self.validate_version(version)
     !!(SEMVER_VERSION =~ version.to_s)
   end
 
+  def self.semver_to_array(v)
+    parts = SEMVER_VERSION.match(v).to_a[1..4]
+    parts[0..2] = parts[0..2].map { |e| e.to_i }
+    parts
+  end
+
   def self.cmp_semver(a, b)
-    a, b = [a, b].map do |x|
-      parts = SEMVER_VERSION.match(x).to_a[1..4]
-      parts[0..2] = parts[0..2].map { |e| e.to_i }
-      parts
-    end
+    a, b = [a, b].map do |x| semver_to_array(x) end
 
     cmp = a[0..2] <=> b[0..2]
     if cmp == 0
@@ -47,18 +49,38 @@ module Puppet::Interface::FaceCollection
     cmp
   end
 
-  def self.[](name, version)
-    @faces[underscorize(name)][version] if face?(name, version)
+  def self.prefix_match?(desired, target)
+    # Can't meaningfully do a prefix match with current on either side.
+    return false if desired == :current
+    return false if target  == :current
+
+    # REVISIT: Should probably fail if the matcher is not valid.
+    prefix = desired.split('.').map {|x| x =~ /^\d+$/ and x.to_i }
+    have   = semver_to_array(target)
+
+    while want = prefix.shift do
+      return false unless want == have.shift
+    end
+    return true
   end
 
-  def self.face?(name, version)
+  def self.[](name, version)
     name = underscorize(name)
+    get_face(name, version) or load_face(name, version)
+  end
 
-    # Note: be careful not to accidentally create the top level key, either,
-    # because it will result in confusion when people try to enumerate the
-    # list of valid faces later. --daniel 2011-04-11
-    return true if @faces.has_key?(name) and @faces[name].has_key?(version)
+  # get face from memory, without loading.
+  def self.get_face(name, desired_version)
+    return nil unless @faces.has_key? name
 
+    return @faces[name][:current] if desired_version == :current
+
+    found = @faces[name].keys.select {|v| prefix_match?(desired_version, v) }.sort.last
+    return @faces[name][found]
+  end
+
+  # try to load the face, and return it.
+  def self.load_face(name, version)
     # We always load the current version file; the common case is that we have
     # the expected version and any compatibility versions in the same file,
     # the default.  Which means that this is almost always the case.
@@ -104,17 +126,7 @@ module Puppet::Interface::FaceCollection
       # ...guess we didn't find the file; return a much better problem.
     end
 
-    # Now, either we have the version in our set of faces, or we didn't find
-    # the version they were looking for.  In the future we will support
-    # loading versioned stuff from some look-aside part of the Ruby load path,
-    # but we don't need that right now.
-    #
-    # So, this comment is a place-holder for that.  --daniel 2011-04-06
-    #
-    # Note: be careful not to accidentally create the top level key, either,
-    # because it will result in confusion when people try to enumerate the
-    # list of valid faces later. --daniel 2011-04-11
-    return !! (@faces.has_key?(name) and @faces[name].has_key?(version))
+    return get_face(name, version)
   end
 
   def self.register(face)
diff --git a/spec/lib/puppet/face/version_matching.rb b/spec/lib/puppet/face/version_matching.rb
new file mode 100644
index 000000000..bfd0013f7
--- /dev/null
+++ b/spec/lib/puppet/face/version_matching.rb
@@ -0,0 +1,10 @@
+require 'puppet/face'
+
+# The set of versions here are used explicitly in the interface_spec; if you
+# change this you need to ensure that is still correct. --daniel 2011-04-21
+['1.0.0', '1.0.1', '1.1.0', '1.1.1', '2.0.0'].each do |version|
+  Puppet::Face.define(:version_matching, version) do
+    summary "version matching face #{version}"
+    script :version do version end
+  end
+end
diff --git a/spec/monkey_patches/disable_signal_trap.rb b/spec/monkey_patches/disable_signal_trap.rb
new file mode 100644
index 000000000..5159626e3
--- /dev/null
+++ b/spec/monkey_patches/disable_signal_trap.rb
@@ -0,0 +1,5 @@
+module Signal
+  def trap(*args)
+    # The goggles, they do nothing!
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index b04ec6ffd..01ffabc48 100755
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -22,6 +22,7 @@ require 'lib/puppet_spec/files'
 require 'lib/puppet_spec/fixtures'
 require 'monkey_patches/alias_should_to_must'
 require 'monkey_patches/publicize_methods'
+require 'monkey_patches/disable_signal_trap'
 
 Pathname.glob("#{dir}/shared_behaviours/**/*.rb") do |behaviour|
   require behaviour.relative_path_from(Pathname.new(dir))
@@ -38,6 +39,10 @@ RSpec.configure do |config|
     # these globals are set by Application
     $puppet_application_mode = nil
     $puppet_application_name = nil
+
+    # REVISIT: I think this conceals other bad tests, but I don't have time to
+    # fully diagnose those right now.  When you read this, please come tell me
+    # I suck for letting this float. --daniel 2011-04-21
     Signal.stubs(:trap)
 
     # Set the confdir and vardir to gibberish so that tests
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index f9498cbb8..890e06a9e 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -24,29 +24,33 @@ describe Puppet::Interface::FaceCollection do
     $".clear ; @original_required.each do |item| $" << item end
   end
 
-  describe "::validate_version" do
-    it 'should permit three number versions' do
-      subject.validate_version('10.10.10').should == true
-    end
-
-    it 'should permit versions with appended descriptions' do
-      subject.validate_version('10.10.10beta').should == true
-    end
-
-    it 'should not permit versions with more than three numbers' do
-      subject.validate_version('1.2.3.4').should == false
-    end
-
-    it 'should not permit versions with only two numbers' do
-      subject.validate_version('10.10').should == false
-    end
-
-    it 'should not permit versions with only one number' do
-      subject.validate_version('123').should == false
+  describe "::prefix_match?" do
+    #   want     have
+    { ['1.0.0', '1.0.0'] => true,
+      ['1.0',   '1.0.0'] => true,
+      ['1',     '1.0.0'] => true,
+      ['1.0.0', '1.1.0'] => false,
+      ['1.0',   '1.1.0'] => false,
+      ['1',     '1.1.0'] => true,
+      ['1.0.1', '1.0.0'] => false,
+    }.each do |data, result|
+      it "should return #{result.inspect} for prefix_match?(#{data.join(', ')})" do
+        subject.prefix_match?(*data).should == result
+      end
     end
+  end
 
-    it 'should not permit versions with text in any position but at the end' do
-      subject.validate_version('v1.1.1').should == false
+  describe "::validate_version" do
+    { '10.10.10'     => true,
+      '1.2.3.4'      => false,
+      '10.10.10beta' => true,
+      '10.10'        => false,
+      '123'          => false,
+      'v1.1.1'       => false,
+    }.each do |input, result|
+      it "should#{result ? '' : ' not'} permit #{input.inspect}" do
+        subject.validate_version(input).should(result ? be_true : be_false)
+      end
     end
   end
 
@@ -68,12 +72,10 @@ describe Puppet::Interface::FaceCollection do
       subject.expects(:require).with('puppet/face/fozzie')
       subject['fozzie', :current]
     end
-  end
 
-  describe "::face?" do
     it "should return true if the face specified is registered" do
       subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
-      subject.face?("foo", '0.0.1').should == true
+      subject["foo", '0.0.1'].should == 10
     end
 
     it "should attempt to require the face if it is not registered" do
@@ -81,24 +83,18 @@ describe Puppet::Interface::FaceCollection do
         subject.instance_variable_get("@faces")[:bar]['0.0.1'] = true
         file == 'puppet/face/bar'
       end
-      subject.face?("bar", '0.0.1').should == true
-    end
-
-    it "should return true if requiring the face registered it" do
-      subject.stubs(:require).with do
-        subject.instance_variable_get("@faces")[:bar]['0.0.1'] = 20
-      end
+      subject["bar", '0.0.1'].should be_true
     end
 
     it "should return false if the face is not registered" do
       subject.stubs(:require).returns(true)
-      subject.face?("bar", '0.0.1').should be_false
+      subject["bar", '0.0.1'].should be_false
     end
 
     it "should return false if the face file itself is missing" do
       subject.stubs(:require).
         raises(LoadError, 'no such file to load -- puppet/face/bar')
-      subject.face?("bar", '0.0.1').should be_false
+      subject["bar", '0.0.1'].should be_false
     end
 
     it "should register the version loaded by `:current` as `:current`" do
@@ -106,26 +102,26 @@ describe Puppet::Interface::FaceCollection do
         subject.instance_variable_get("@faces")[:huzzah]['2.0.1'] = :huzzah_face
         file == 'puppet/face/huzzah'
       end
-      subject.face?("huzzah", :current)
+      subject["huzzah", :current]
       subject.instance_variable_get("@faces")[:huzzah][:current].should == :huzzah_face
     end
 
     context "with something on disk" do
       it "should register the version loaded from `puppet/face/{name}` as `:current`" do
-        subject.should be_face "huzzah", '2.0.1'
-        subject.should be_face "huzzah", :current
+        subject["huzzah", '2.0.1'].should be
+        subject["huzzah", :current].should be
         Puppet::Face[:huzzah, '2.0.1'].should == Puppet::Face[:huzzah, :current]
       end
 
       it "should index :current when the code was pre-required" do
         subject.instance_variable_get("@faces")[:huzzah].should_not be_key :current
         require 'puppet/face/huzzah'
-        subject.face?(:huzzah, :current).should be_true
+        subject[:huzzah, :current].should be_true
       end
     end
 
     it "should not cause an invalid face to be enumerated later" do
-      subject.face?(:there_is_no_face, :current).should be_false
+      subject[:there_is_no_face, :current].should be_false
       subject.faces.should_not include :there_is_no_face
     end
   end
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index b4fef0307..a1d70cf64 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -5,16 +5,17 @@ require 'puppet/interface'
 describe Puppet::Interface do
   subject { Puppet::Interface }
 
-  before :all do
-    @faces = Puppet::Interface::FaceCollection.instance_variable_get("@faces").dup
-  end
-
   before :each do
+    @faces = Puppet::Interface::FaceCollection.
+      instance_variable_get("@faces").dup
+    @dq = $".dup
+    $".delete_if do |path| path =~ %r{/face/.*\.rb$} end
     Puppet::Interface::FaceCollection.instance_variable_get("@faces").clear
   end
 
-  after :all do
+  after :each do
     Puppet::Interface::FaceCollection.instance_variable_set("@faces", @faces)
+    $".clear ; @dq.each do |item| $" << item end
   end
 
   describe "#[]" do
@@ -29,6 +30,28 @@ describe Puppet::Interface do
     it "should raise an exception when the requested face doesn't exist" do
       expect { subject[:burrble_toot, :current] }.should raise_error, Puppet::Error
     end
+
+    describe "version matching" do
+      { '1'     => '1.1.1',
+        '1.0'   => '1.0.1',
+        '1.0.1' => '1.0.1',
+        '1.1'   => '1.1.1',
+        '1.1.1' => '1.1.1'
+      }.each do |input, expect|
+        it "should match #{input.inspect} to #{expect.inspect}" do
+          face = subject[:version_matching, input]
+          face.should be
+          face.version.should == expect
+        end
+      end
+
+      %w{1.0.2 1.2}.each do |input|
+        it "should not match #{input.inspect} to any version" do
+          expect { subject[:version_matching, input] }.
+            to raise_error Puppet::Error, /Could not find version/
+        end
+      end
+    end
   end
 
   describe "#define" do