(#7184) Load the core of obsolete versions of Faces.

When we define an action on an older version of a Face, we must be sure to
directly load the core of that version, not just define it with the external
Action(s) that it had.

Otherwise we break our contract, which is that any core Actions for a specific
version will be available to your external Action for as long as we support
that core version.

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>

5 files changed, 61 insertions, 18 deletions

diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 868997b67..b1f6ba398 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -56,9 +56,7 @@ module Puppet::Interface::FaceCollection
     #
     # We use require to avoid executing the code multiple times, like any
     # other Ruby library that we might want to use.  --daniel 2011-04-06
-    begin
-      require "puppet/face/#{name}"
-
+    if safely_require name then
       # If we wanted :current, we need to index to find that; direct version
       # requests just work™ as they go. --daniel 2011-04-06
       if version == :current then
@@ -90,18 +88,32 @@ module Puppet::Interface::FaceCollection
         latest_ver = @faces[name].keys.sort.last
         @faces[name][:current] = @faces[name][latest_ver]
       end
-    rescue LoadError => e
-      raise unless e.message =~ %r{-- puppet/face/#{name}$}
-      # ...guess we didn't find the file; return a much better problem.
-    rescue SyntaxError => e
-      raise unless e.message =~ %r{puppet/face/#{name}\.rb:\d+: }
-      Puppet.err "Failed to load face #{name}:\n#{e}"
-      # ...but we just carry on after complaining.
+    end
+
+    unless version == :current or get_face(name, version)
+      # Try an obsolete version of the face, if needed, to see if that helps?
+      safely_require name, version
     end
 
     return get_face(name, version)
   end
 
+  def self.safely_require(name, version = nil)
+    path = File.join 'puppet' ,'face', version.to_s, name.to_s
+    require path
+    true
+
+  rescue LoadError => e
+    raise unless e.message =~ %r{-- #{path}$}
+    # ...guess we didn't find the file; return a much better problem.
+    nil
+  rescue SyntaxError => e
+    raise unless e.message =~ %r{#{path}\.rb:\d+: }
+    Puppet.err "Failed to load face #{name}:\n#{e}"
+    # ...but we just carry on after complaining.
+    nil
+  end
+
   def self.register(face)
     @faces[underscorize(face.name)][face.version] = face
   end
diff --git a/spec/lib/puppet/face/1.0.0/huzzah.rb b/spec/lib/puppet/face/1.0.0/huzzah.rb
new file mode 100755
index 000000000..00f20f096
--- /dev/null
+++ b/spec/lib/puppet/face/1.0.0/huzzah.rb
@@ -0,0 +1,9 @@
+require 'puppet/face'
+Puppet::Face.define(:huzzah, '1.0.0') do
+  copyright "Puppet Labs", 2011
+  license   "Apache 2 license; see COPYING"
+  summary "life is a thing for celebration"
+  script :obsolete_in_core do |_| "you are in obsolete core now!" end
+
+  script :call_newer do method_on_newer end
+end
diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb
index ab465d9e0..47cc3f32c 100755
--- a/spec/lib/puppet/face/huzzah.rb
+++ b/spec/lib/puppet/face/huzzah.rb
@@ -4,4 +4,6 @@ Puppet::Face.define(:huzzah, '2.0.1') do
   license   "Apache 2 license; see COPYING"
   summary "life is a thing for celebration"
   script :bar do |options| "is where beer comes from" end
+
+  script :call_older do method_on_older end
 end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 8f9c349b6..514a624b1 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -35,7 +35,8 @@ describe Puppet::Interface::FaceCollection do
     end
 
     it "should attempt to load the face if it isn't found" do
-      subject.expects(:require).with('puppet/face/bar')
+      subject.expects(:require).once.with('puppet/face/bar')
+      subject.expects(:require).once.with('puppet/face/0.0.1/bar')
       subject["bar", '0.0.1']
     end
 
@@ -64,7 +65,8 @@ describe Puppet::Interface::FaceCollection do
 
     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')
+        raises(LoadError, 'no such file to load -- puppet/face/bar').then.
+        raises(LoadError, 'no such file to load -- puppet/face/0.0.1/bar')
       subject["bar", '0.0.1'].should be_false
     end
 
@@ -110,6 +112,27 @@ describe Puppet::Interface::FaceCollection do
       action.should be_an_instance_of Puppet::Interface::Action
       action.face.version.should == SemVer.new('1.0.0')
     end
+
+    it "should load the full older version of a face" do
+      action = Puppet::Face::FaceCollection.
+        get_action_for_face(:huzzah, :obsolete, :current)
+
+      action.face.version.should == SemVer.new('1.0.0')
+      action.face.should be_action :obsolete_in_core
+    end
+
+    it "should not add obsolete actions to the current version" do
+      action = Puppet::Face::FaceCollection.
+        get_action_for_face(:huzzah, :obsolete, :current)
+
+      action.face.version.should == SemVer.new('1.0.0')
+      action.face.should be_action :obsolete_in_core
+
+      current = Puppet::Face[:huzzah, :current]
+      current.version.should == SemVer.new('2.0.1')
+      current.should_not be_action :obsolete_in_core
+      current.should_not be_action :obsolete
+    end
   end
 
   describe "::register" do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 4cb1f8743..4ff71ac3d 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -126,14 +126,11 @@ describe Puppet::Interface do
   end
 
   it "should try to require faces that are not known" do
-    pending "mocking require causes random stack overflow"
-    subject::FaceCollection.expects(:require).with "puppet/face/foo"
-    subject[:foo, '0.0.1']
+    subject::FaceCollection.expects(:load_face).with(:foo, :current)
+    subject::FaceCollection.expects(:load_face).with(:foo, '0.0.1')
+    expect { subject[:foo, '0.0.1'] }.to raise_error Puppet::Error
   end
 
-  it "should be able to load all actions in all search paths"
-
-
   it_should_behave_like "things that declare options" do
     def add_options_to(&block)
       subject.new(:with_options, '0.0.1', &block)