Merge branch 'tickets/master/6770'

10 files changed, 247 insertions, 38 deletions

diff --git a/README.markdown b/README.markdown
index 5f720db62..29ff41430 100644
--- a/README.markdown
+++ b/README.markdown
@@ -86,7 +86,7 @@ Or use IRB to do the same thing:
     => true
     >> interface = Puppet::Interface[:facts, '1.0.0']
     => #<Puppet::Interface::Facts:0x1024a1390 @format=:yaml>
-    >> facts = interface.find("myhost"); nil
+    >> facts = interface.find("myhost")
 
 Like I said, a prototype, but I'd love it if people would play it with some and make some recommendations.
 
@@ -111,3 +111,5 @@ Like most parts of Puppet, these are easy to extend.  Just drop a new action int
     $
 
 Notice that this gets loaded automatically when you try to use it.  So, if you have a simple command you've written, such as for cleaning up nodes or diffing catalogs, you an port it to this framework and it should fit cleanly.
+
+Also note that interfaces are versioned.  These version numbers are interpreted according to Semantic Versioning (http://semver.org).
diff --git a/lib/puppet/application/interface_base.rb b/lib/puppet/application/interface_base.rb
index c1c02040a..841f3ca12 100644
--- a/lib/puppet/application/interface_base.rb
+++ b/lib/puppet/application/interface_base.rb
@@ -72,10 +72,10 @@ class Puppet::Application::InterfaceBase < Puppet::Application
     @type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
 
     # TODO: These should be configurable versions.
-    unless Puppet::Interface.interface?(@type, '0.0.1')
-      raise "Could not find version #{1} of interface '#{@type}'"
+    unless Puppet::Interface.interface?(@type, :latest)
+      raise "Could not find any version of interface '#{@type}'"
     end
-    @interface = Puppet::Interface[@type, '0.0.1']
+    @interface = Puppet::Interface[@type, :latest]
     @format ||= @interface.default_format
 
     # We copy all of the app options to the interface.
diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 64f1bfe49..a667c6b75 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -30,15 +30,17 @@ class Puppet::Interface
       Puppet::Interface::InterfaceCollection.register(instance)
     end
 
-    def define(name, version, &blk)
+    def define(name, version, &block)
       if interface?(name, version)
         interface = Puppet::Interface::InterfaceCollection[name, version]
-        interface.instance_eval(&blk) if blk
       else
-        interface = self.new(name, :version => version, &blk)
+        interface = self.new(name, version)
         Puppet::Interface::InterfaceCollection.register(interface)
         interface.load_actions
       end
+
+      interface.instance_eval(&block) if block_given?
+
       return interface
     end
 
@@ -54,22 +56,21 @@ class Puppet::Interface
   attr_accessor :type, :verb, :version, :arguments, :options
   attr_reader :name
 
-  def initialize(name, options = {}, &block)
-    unless options[:version]
-      raise ArgumentError, "Interface #{name} declared without version!"
+  def initialize(name, version, &block)
+    unless Puppet::Interface::InterfaceCollection.validate_version(version)
+      raise ArgumentError, "Cannot create interface with invalid version number '#{version}'!"
     end
 
     @name = Puppet::Interface::InterfaceCollection.underscorize(name)
-
+    @version = version
     @default_format = :pson
-    options.each { |opt, val| send(opt.to_s + "=", val) }
 
-    instance_eval(&block) if block
+    instance_eval(&block) if block_given?
   end
 
   # Try to find actions defined in other files.
   def load_actions
-    path = "puppet/interface/#{name}"
+    path = "puppet/interface/v#{version}/#{name}"
 
     loaded = []
     Puppet::Interface.autoloader.search_directories.each do |dir|
@@ -92,6 +93,6 @@ class Puppet::Interface
   end
 
   def to_s
-    "Puppet::Interface(#{name}, :version => #{version.inspect})"
+    "Puppet::Interface[#{name.inspect}, #{version.inspect}]"
   end
 end
diff --git a/lib/puppet/interface/interface_collection.rb b/lib/puppet/interface/interface_collection.rb
index d626c4f72..92e2933fe 100644
--- a/lib/puppet/interface/interface_collection.rb
+++ b/lib/puppet/interface/interface_collection.rb
@@ -1,6 +1,8 @@
 require 'puppet/interface'
 
 module Puppet::Interface::InterfaceCollection
+  SEMVER_VERSION = /^(\d+)\.(\d+)\.(\d+)([A-Za-z][0-9A-Za-z-]*|)$/
+
   @interfaces = Hash.new { |hash, key| hash[key] = {} }
 
   def self.interfaces
@@ -9,7 +11,7 @@ module Puppet::Interface::InterfaceCollection
       $LOAD_PATH.each do |dir|
         next unless FileTest.directory?(dir)
         Dir.chdir(dir) do
-          Dir.glob("puppet/interface/*.rb").collect { |f| f.sub(/\.rb/, '') }.each do |file|
+          Dir.glob("puppet/interface/v*/*.rb").collect { |f| f.sub(/\.rb/, '') }.each do |file|
             iname = file.sub(/\.rb/, '')
             begin
               require iname
@@ -24,12 +26,56 @@ module Puppet::Interface::InterfaceCollection
     return @interfaces.keys
   end
 
+  def self.versions(name)
+    versions = []
+    $LOAD_PATH.each do |dir|
+      next unless FileTest.directory?(dir)
+      v_dir = File.join dir, %w[puppet interface v*]
+      Dir.glob(File.join v_dir, "#{name}{.rb,/*.rb}").each do |f|
+        v = f.sub(%r[.*/v([^/]+?)/#{name}(?:(?:/[^/]+)?.rb)$], '\1')
+        if validate_version(v)
+          versions << v
+        else
+          warn "'#{v}' (#{f}) is not a valid version string; skipping"
+        end
+      end
+    end
+    return versions.uniq.sort { |a, b| compare_versions(a, b)  }
+  end
+
+  def self.validate_version(version)
+    !!(SEMVER_VERSION =~ version.to_s)
+  end
+
+  def self.compare_versions(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
+
+    cmp = a[0..2] <=> b[0..2]
+    if cmp == 0
+      cmp = a[3] <=> b[3]
+      cmp = +1 if a[3].empty? && !b[3].empty?
+      cmp = -1 if b[3].empty? && !a[3].empty?
+    end
+    cmp
+  end
+
   def self.[](name, version)
-    @interfaces[underscorize(name)][version] if interface?(name, version)
+    version = versions(name).last if version == :latest
+    unless version.nil?
+      @interfaces[underscorize(name)][version] if interface?(name, version)
+    end
   end
 
   def self.interface?(name, version)
+    version = versions(name).last if version == :latest
+    return false if version.nil?
+
     name = underscorize(name)
+
     unless @interfaces.has_key?(name) && @interfaces[name].has_key?(version)
       require "puppet/interface/v#{version}/#{name}"
     end
diff --git a/spec/unit/application/interface_base_spec.rb b/spec/unit/application/interface_base_spec.rb
index 15d465559..d82325bfd 100644
--- a/spec/unit/application/interface_base_spec.rb
+++ b/spec/unit/application/interface_base_spec.rb
@@ -4,7 +4,19 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
 require 'puppet/application/interface_base'
 
 describe Puppet::Application::InterfaceBase do
-  base_interface = Puppet::Interface[:basetest, '0.0.1']
+  before :all do
+    @dir = Dir.mktmpdir
+    $LOAD_PATH.push(@dir)
+    FileUtils.mkdir_p(File.join @dir, 'puppet', 'interface', 'v0.0.1')
+    FileUtils.touch(File.join @dir, 'puppet', 'interface', 'v0.0.1', 'basetest.rb')
+  end
+
+  after :all do
+    FileUtils.remove_entry_secure @dir
+    $LOAD_PATH.pop
+  end
+
+  base_interface = Puppet::Interface.define(:basetest, '0.0.1')
   class Puppet::Application::InterfaceBase::Basetest < Puppet::Application::InterfaceBase
   end
 
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index 2c2f3b14c..27e817fe9 100644
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
@@ -13,7 +13,7 @@ describe Puppet::Interface::ActionBuilder do
     end
 
     it "should define a method on the interface which invokes the action" do
-      interface = Puppet::Interface.new(:action_builder_test_interface, :version => '0.0.1')
+      interface = Puppet::Interface.new(:action_builder_test_interface, '0.0.1')
       action = Puppet::Interface::ActionBuilder.build(interface, :foo) do
         invoke do
           "invoked the method"
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 246ae9622..292caabb9 100644
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -24,7 +24,7 @@ describe Puppet::Interface::Action do
 
   describe "when invoking" do
     it "should be able to call other actions on the same object" do
-      interface = Puppet::Interface.new(:my_interface, :version => '0.0.1') do
+      interface = Puppet::Interface.new(:my_interface, '0.0.1') do
         action(:foo) do
           invoke { 25 }
         end
@@ -56,7 +56,7 @@ describe Puppet::Interface::Action do
         end
       end
 
-      interface = Puppet::Interface::MyInterfaceBaseClass.new(:my_inherited_interface, :version => '0.0.1') do
+      interface = Puppet::Interface::MyInterfaceBaseClass.new(:my_inherited_interface, '0.0.1') do
         action(:baz) do
           invoke { "the value of foo in baz is '#{foo}'" }
         end
diff --git a/spec/unit/interface/indirector_spec.rb b/spec/unit/interface/indirector_spec.rb
index b14058eca..4b2beaefc 100644
--- a/spec/unit/interface/indirector_spec.rb
+++ b/spec/unit/interface/indirector_spec.rb
@@ -5,7 +5,7 @@ require 'puppet/interface/indirector'
 
 describe Puppet::Interface::Indirector do
   before do
-    @instance = Puppet::Interface::Indirector.new(:test, :version => '0.0.1')
+    @instance = Puppet::Interface::Indirector.new(:test, '0.0.1')
 
     @indirection = stub 'indirection', :name => :stub_indirection
 
@@ -24,7 +24,7 @@ describe Puppet::Interface::Indirector do
     it "should be able to determine its indirection" do
       # Loading actions here an get, um, complicated
       Puppet::Interface.stubs(:load_actions)
-      Puppet::Interface::Indirector.new(:catalog, :version => '0.0.1').indirection.should equal(Puppet::Resource::Catalog.indirection)
+      Puppet::Interface::Indirector.new(:catalog, '0.0.1').indirection.should equal(Puppet::Resource::Catalog.indirection)
     end
   end
 
diff --git a/spec/unit/interface/interface_collection_spec.rb b/spec/unit/interface/interface_collection_spec.rb
index 193d31b1e..3e4b9d624 100644
--- a/spec/unit/interface/interface_collection_spec.rb
+++ b/spec/unit/interface/interface_collection_spec.rb
@@ -1,6 +1,7 @@
 #!/usr/bin/env ruby
 
 require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
+require 'tmpdir'
 
 describe Puppet::Interface::InterfaceCollection do
   before :all do
@@ -18,11 +19,149 @@ describe Puppet::Interface::InterfaceCollection do
   describe "::interfaces" do
   end
 
+  describe "::versions" do
+    before :each do
+      @dir = Dir.mktmpdir
+      @lib = FileUtils.mkdir_p(File.join @dir, 'puppet', 'interface')
+      $LOAD_PATH.push(@dir)
+    end
+
+    after :each do
+      FileUtils.remove_entry_secure @dir
+      $LOAD_PATH.pop
+    end
+
+    it "should return an empty array when no versions are loadable" do
+      subject.versions(:fozzie).should == []
+    end
+
+    it "should return versions loadable as puppet/interface/v{version}/{name}" do
+      FileUtils.mkdir_p(File.join @lib, 'v1.0.0')
+      FileUtils.touch(File.join @lib, 'v1.0.0', 'fozzie.rb')
+      subject.versions(:fozzie).should == ['1.0.0']
+    end
+
+    it "should an ordered list of all versions loadable as puppet/interface/v{version}/{name}" do
+      %w[ 1.2.1rc2 1.2.1beta1 1.2.1rc1 1.2.1 1.2.2 ].each do |version|
+        FileUtils.mkdir_p(File.join @lib, "v#{version}")
+        FileUtils.touch(File.join @lib, "v#{version}", 'fozzie.rb')
+      end
+      subject.versions(:fozzie).should == %w[ 1.2.1beta1 1.2.1rc1 1.2.1rc2 1.2.1 1.2.2 ]
+    end
+
+    it "should not return a version for an empty puppet/interface/v{version}/{name}" do
+      FileUtils.mkdir_p(File.join @lib, 'v1.0.0', 'fozzie')
+      subject.versions(:fozzie).should == []
+    end
+
+    it "should an ordered list of all versions loadable as puppet/interface/v{version}/{name}/*.rb" do
+      %w[ 1.2.1rc2 1.2.1beta1 1.2.1rc1 1.2.1 1.2.2 ].each do |version|
+        FileUtils.mkdir_p(File.join @lib, "v#{version}", "fozzie")
+        FileUtils.touch(File.join @lib, "v#{version}", 'fozzie', 'action.rb')
+      end
+      subject.versions(:fozzie).should == %w[ 1.2.1beta1 1.2.1rc1 1.2.1rc2 1.2.1 1.2.2 ]
+    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
+    end
+
+    it 'should not permit versions with text in any position but at the end' do
+      subject.validate_version('v1.1.1').should == false
+    end
+  end
+
+  describe "::compare_versions" do
+    # (a <=> b) should be:
+    #   -1 if a < b
+    #   0  if a == b
+    #   1  if a > b
+    it 'should sort major version numbers numerically' do
+      subject.compare_versions('1.0.0', '2.0.0').should == -1
+      subject.compare_versions('2.0.0', '1.1.1').should == 1
+      subject.compare_versions('2.0.0', '10.0.0').should == -1
+    end
+
+    it 'should sort minor version numbers numerically' do
+      subject.compare_versions('0.1.0', '0.2.0').should == -1
+      subject.compare_versions('0.2.0', '0.1.1').should == 1
+      subject.compare_versions('0.2.0', '0.10.0').should == -1
+    end
+
+    it 'should sort tiny version numbers numerically' do
+      subject.compare_versions('0.0.1', '0.0.2').should == -1
+      subject.compare_versions('0.0.2', '0.0.1').should == 1
+      subject.compare_versions('0.0.2', '0.0.10').should == -1
+    end
+
+    it 'should sort major version before minor version' do
+      subject.compare_versions('1.1.0', '1.2.0').should == -1
+      subject.compare_versions('1.2.0', '1.1.1').should == 1
+      subject.compare_versions('1.2.0', '1.10.0').should == -1
+
+      subject.compare_versions('1.1.0', '2.2.0').should == -1
+      subject.compare_versions('2.2.0', '1.1.1').should == 1
+      subject.compare_versions('2.2.0', '1.10.0').should == 1
+    end
+
+    it 'should sort minor version before tiny version' do
+      subject.compare_versions('0.1.1', '0.1.2').should == -1
+      subject.compare_versions('0.1.2', '0.1.1').should == 1
+      subject.compare_versions('0.1.2', '0.1.10').should == -1
+
+      subject.compare_versions('0.1.1', '0.2.2').should == -1
+      subject.compare_versions('0.2.2', '0.1.1').should == 1
+      subject.compare_versions('0.2.2', '0.1.10').should == 1
+    end
+
+    it 'should sort appended strings asciibetically' do
+      subject.compare_versions('0.0.0a', '0.0.0b').should == -1
+      subject.compare_versions('0.0.0beta1', '0.0.0beta2').should == -1
+      subject.compare_versions('0.0.0beta1', '0.0.0rc1').should == -1
+      subject.compare_versions('0.0.0beta1', '0.0.0alpha1').should == 1
+      subject.compare_versions('0.0.0beta1', '0.0.0beta1').should == 0
+    end
+
+    it "should sort appended strings before 'whole' versions" do
+      subject.compare_versions('0.0.1a', '0.0.1').should == -1
+      subject.compare_versions('0.0.1', '0.0.1beta').should == 1
+    end
+  end
+
   describe "::[]" do
     before :each do
       subject.instance_variable_get("@interfaces")[:foo]['0.0.1'] = 10
     end
 
+    before :each do
+      @dir = Dir.mktmpdir
+      @lib = FileUtils.mkdir_p(File.join @dir, 'puppet', 'interface')
+      $LOAD_PATH.push(@dir)
+    end
+
+    after :each do
+      FileUtils.remove_entry_secure @dir
+      $LOAD_PATH.pop
+    end
+
     it "should return the interface with the given name" do
       subject["foo", '0.0.1'].should == 10
     end
@@ -31,6 +170,15 @@ describe Puppet::Interface::InterfaceCollection do
       subject.expects(:require).with('puppet/interface/v0.0.1/bar')
       subject["bar", '0.0.1']
     end
+
+    it "should attempt to load the interface with the greatest version for specified version :latest" do
+      %w[ 1.2.1 1.2.2 ].each do |version|
+        FileUtils.mkdir_p(File.join @lib, "v#{version}")
+        FileUtils.touch(File.join @lib, "v#{version}", 'fozzie.rb')
+      end
+      subject.expects(:require).with('puppet/interface/v1.2.2/fozzie')
+      subject['fozzie', :latest]
+    end
   end
 
   describe "::interface?" do
@@ -67,7 +215,7 @@ describe Puppet::Interface::InterfaceCollection do
 
   describe "::register" do
     it "should store the interface by name" do
-      interface = Puppet::Interface.new(:my_interface, :version => '0.0.1')
+      interface = Puppet::Interface.new(:my_interface, '0.0.1')
       subject.register(interface)
       subject.instance_variable_get("@interfaces").should == {:my_interface => {'0.0.1' => interface}}
     end
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 520060cba..cf7d209da 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -15,29 +15,33 @@ describe Puppet::Interface do
     Puppet::Interface::InterfaceCollection.instance_variable_set("@interfaces", @interfaces)
   end
 
-  describe "#interface" do
+  describe "#define" do
     it "should register the interface" do
-      interface = Puppet::Interface[:interface_test_register, '0.0.1']
+      interface = Puppet::Interface.define(:interface_test_register, '0.0.1')
       interface.should == Puppet::Interface[:interface_test_register, '0.0.1']
     end
 
     it "should load actions" do
       Puppet::Interface.any_instance.expects(:load_actions)
-      Puppet::Interface[:interface_test_load_actions, '0.0.1']
+      Puppet::Interface.define(:interface_test_load_actions, '0.0.1')
     end
 
     it "should require a version number" do
-      proc { Puppet::Interface[:no_version] }.should raise_error(ArgumentError)
+      proc { Puppet::Interface.define(:no_version) }.should raise_error(ArgumentError)
     end
   end
 
   describe "#initialize" do
     it "should require a version number" do
-      proc { Puppet::Interface.new(:no_version) }.should raise_error(/declared without version/)
+      proc { Puppet::Interface.new(:no_version) }.should raise_error(ArgumentError)
+    end
+
+    it "should require a valid version number" do
+      proc { Puppet::Interface.new(:bad_version, 'Rasins') }.should raise_error(ArgumentError)
     end
 
     it "should instance-eval any provided block" do
-      face = Puppet::Interface.new(:interface_test_block, :version => '0.0.1') do
+      face = Puppet::Interface.new(:interface_test_block,'0.0.1') do
         action(:something) do
           invoke { "foo" }
         end
@@ -48,21 +52,21 @@ describe Puppet::Interface do
   end
 
   it "should have a name" do
-    Puppet::Interface.new(:me, :version => '0.0.1').name.should == :me
+    Puppet::Interface.new(:me,'0.0.1').name.should == :me
   end
 
   it "should stringify with its own name" do
-    Puppet::Interface.new(:me, :version => '0.0.1').to_s.should =~ /\bme\b/
+    Puppet::Interface.new(:me,'0.0.1').to_s.should =~ /\bme\b/
   end
 
   it "should allow overriding of the default format" do
-    face = Puppet::Interface.new(:me, :version => '0.0.1')
+    face = Puppet::Interface.new(:me,'0.0.1')
     face.set_default_format :foo
     face.default_format.should == :foo
   end
 
   it "should default to :pson for its format" do
-    Puppet::Interface.new(:me, :version => '0.0.1').default_format.should == :pson
+    Puppet::Interface.new(:me, '0.0.1').default_format.should == :pson
   end
 
   # Why?
@@ -70,10 +74,6 @@ describe Puppet::Interface do
     Puppet::Interface.autoloader.should be_instance_of(Puppet::Util::Autoload)
   end
 
-  it "should set any provided options" do
-    Puppet::Interface.new(:me, :version => 1, :verb => "foo").verb.should == "foo"
-  end
-
   it "should try to require interfaces that are not known" do
     Puppet::Interface::InterfaceCollection.expects(:require).with "puppet/interface/v0.0.1/foo"
     Puppet::Interface[:foo, '0.0.1']