Merge branch 'ticket/next/5427' into next

* ticket/next/5427:
  (#5427) Using Propery::OrderedList for host_alias
  (#5427) Remove redundant testunit tests

5 files changed, 80 insertions, 247 deletions

diff --git a/lib/puppet/provider/host/parsed.rb b/lib/puppet/provider/host/parsed.rb
index a303c4bcf..2ba01a41c 100644
--- a/lib/puppet/provider/host/parsed.rb
+++ b/lib/puppet/provider/host/parsed.rb
@@ -22,9 +22,7 @@ Puppet::Type.type(:host).provide(:parsed,:parent => Puppet::Provider::ParsedFile
       # An absent comment should match "comment => ''"
       hash[:comment] = '' if hash[:comment].nil? or hash[:comment] == :absent
       unless hash[:host_aliases].nil? or hash[:host_aliases] == :absent
-        hash[:host_aliases] = hash[:host_aliases].split(/\s+/)
-      else
-        hash[:host_aliases] = []
+        hash[:host_aliases].gsub!(/\s+/,' ') # Change delimiter
       end
     },
     :to_line  => proc { |hash|
@@ -32,8 +30,8 @@ Puppet::Type.type(:host).provide(:parsed,:parent => Puppet::Provider::ParsedFile
         raise ArgumentError, "#{n} is a required attribute for hosts" unless hash[n] and hash[n] != :absent
       end
       str = "#{hash[:ip]}\t#{hash[:name]}"
-      if hash.include? :host_aliases and !hash[:host_aliases].empty?
-        str += "\t#{hash[:host_aliases].join("\t")}"
+      if hash.include? :host_aliases and !hash[:host_aliases].nil? and hash[:host_aliases] != :absent
+        str += "\t#{hash[:host_aliases]}"
       end
       if hash.include? :comment and !hash[:comment].empty?
         str += "\t# #{hash[:comment]}"
diff --git a/lib/puppet/type/host.rb b/lib/puppet/type/host.rb
index 1af74d886..867ef2ab3 100755
--- a/lib/puppet/type/host.rb
+++ b/lib/puppet/type/host.rb
@@ -1,3 +1,5 @@
+require 'puppet/property/ordered_list'
+
 module Puppet
   newtype(:host) do
     ensurable
@@ -13,41 +15,24 @@ module Puppet
 
     end
 
-    newproperty(:host_aliases) do
+    # for now we use OrderedList to indicate that the order does matter.
+    newproperty(:host_aliases, :parent => Puppet::Property::OrderedList) do
       desc "Any aliases the host might have.  Multiple values must be
         specified as an array."
 
-      def insync?(is)
-        is == @should
+      def delimiter
+        " "
       end
 
-      def is_to_s(currentvalue = @is)
-        currentvalue = [currentvalue] unless currentvalue.is_a? Array
-        currentvalue.join(" ")
-      end
-
-      # We actually want to return the whole array here, not just the first
-      # value.
-      def should
-        if defined?(@should)
-          if @should == [:absent]
-            return :absent
-          else
-            return @should
-          end
-        else
-          return nil
-        end
-      end
-
-      def should_to_s(newvalue = @should)
-        newvalue.join(" ")
+      def inclusive?
+        true
       end
 
       validate do |value|
         raise Puppet::Error, "Host aliases cannot include whitespace" if value =~ /\s/
         raise Puppet::Error, "Host alias cannot be an empty string. Use an empty array to delete all host_aliases " if value =~ /^\s*$/
       end
+
     end
 
     newproperty(:comment) do
diff --git a/spec/unit/provider/host/parsed_spec.rb b/spec/unit/provider/host/parsed_spec.rb
index 3e00a2190..5704304ba 100644
--- a/spec/unit/provider/host/parsed_spec.rb
+++ b/spec/unit/provider/host/parsed_spec.rb
@@ -28,9 +28,13 @@ describe provider_class do
     hostresource = Puppet::Type::Host.new(:name => args[:name])
     hostresource.stubs(:should).with(:target).returns @hostfile
 
-    # Using setters of provider
+    # Using setters of provider to build our testobject
+    # Note: We already proved, that in case of host_aliases
+    # the provider setter "host_aliases=(value)" will be
+    # called with the joined array, so we just simulate that
     host = @provider.new(hostresource)
     args.each do |property,value|
+      value = value.join(" ") if property == :host_aliases and value.is_a?(Array)
       host.send("#{property}=", value)
     end
     host
@@ -63,6 +67,10 @@ describe provider_class do
       @provider.parse_line("::1     localhost")[:comment].should == ""
     end
 
+    it "should set host_aliases to :absent" do
+      @provider.parse_line("::1     localhost")[:host_aliases].should == :absent
+    end
+
   end
 
   describe "when parsing a line with ip, hostname and comment" do
@@ -87,13 +95,13 @@ describe provider_class do
   describe "when parsing a line with ip, hostname and aliases" do
 
     it "should parse alias from the third field" do
-      @provider.parse_line("127.0.0.1   localhost   localhost.localdomain")[:host_aliases].should == ["localhost.localdomain"]
+      @provider.parse_line("127.0.0.1   localhost   localhost.localdomain")[:host_aliases].should == "localhost.localdomain"
     end
 
     it "should parse multiple aliases" do
-      @provider.parse_line("127.0.0.1 host alias1 alias2")[:host_aliases].should == ['alias1', 'alias2']
-      @provider.parse_line("127.0.0.1 host alias1\talias2")[:host_aliases].should == ['alias1', 'alias2']
-      @provider.parse_line("127.0.0.1 host alias1\talias2   alias3")[:host_aliases].should == ['alias1', 'alias2', 'alias3']
+      @provider.parse_line("127.0.0.1 host alias1 alias2")[:host_aliases].should == 'alias1 alias2'
+      @provider.parse_line("127.0.0.1 host alias1\talias2")[:host_aliases].should == 'alias1 alias2'
+      @provider.parse_line("127.0.0.1 host alias1\talias2   alias3")[:host_aliases].should == 'alias1 alias2 alias3'
     end
 
   end
@@ -114,7 +122,7 @@ describe provider_class do
     end
 
     it "should parse all host_aliases from the third field" do
-      @provider.parse_line(@testline)[:host_aliases].should == ['alias1' ,'alias2', 'alias3']
+      @provider.parse_line(@testline)[:host_aliases].should == 'alias1 alias2 alias3'
     end
 
     it "should parse the comment after the first '#' character" do
@@ -143,7 +151,7 @@ describe provider_class do
       host = mkhost(
         :name   => 'localhost.localdomain',
         :ip     => '127.0.0.1',
-        :host_aliases => ['localhost'],
+        :host_aliases => 'localhost',
         :ensure => :present
       )
       genhost(host).should == "127.0.0.1\tlocalhost.localdomain\tlocalhost\n"
@@ -156,7 +164,7 @@ describe provider_class do
         :host_aliases => [ 'a1','a2','a3','a4' ],
         :ensure     => :present
       )
-      genhost(host).should == "192.0.0.1\thost\ta1\ta2\ta3\ta4\n"
+      genhost(host).should == "192.0.0.1\thost\ta1 a2 a3 a4\n"
     end
 
     it "should be able to generate a simple hostfile entry with comments" do
@@ -173,7 +181,7 @@ describe provider_class do
       host = mkhost(
         :name   => 'localhost.localdomain',
         :ip     => '127.0.0.1',
-        :host_aliases => ['localhost'],
+        :host_aliases => 'localhost',
         :comment => 'Bazinga!',
         :ensure => :present
       )
@@ -188,7 +196,7 @@ describe provider_class do
         :comment      => 'Bazinga!',
         :ensure       => :present
       )
-      genhost(host).should == "192.0.0.1\thost\ta1\ta2\ta3\ta4\t# Bazinga!\n"
+      genhost(host).should == "192.0.0.1\thost\ta1 a2 a3 a4\t# Bazinga!\n"
     end
 
   end
diff --git a/spec/unit/type/host_spec.rb b/spec/unit/type/host_spec.rb
index 36ef460c1..60ce73c6d 100755
--- a/spec/unit/type/host_spec.rb
+++ b/spec/unit/type/host_spec.rb
@@ -2,12 +2,14 @@
 
 require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper')
 
-ssh_authorized_key = Puppet::Type.type(:ssh_authorized_key)
+host = Puppet::Type.type(:host)
 
-describe Puppet::Type.type(:host) do
+describe host do
   before do
-    @class = Puppet::Type.type(:host)
+    @class = host
     @catalog = Puppet::Resource::Catalog.new
+    @provider = stub 'provider'
+    @resource = stub 'resource', :resource => nil, :provider => @provider
   end
 
   it "should have :name be its namevar" do
@@ -26,6 +28,11 @@ describe Puppet::Type.type(:host) do
         @class.attrtype(property).should == :property
       end
     end
+
+    it "should have a list host_aliases" do
+      @class.attrclass(:host_aliases).ancestors.should be_include(Puppet::Property::OrderedList)
+    end
+
   end
 
   describe "when validating values" do
@@ -80,4 +87,44 @@ describe Puppet::Type.type(:host) do
       proc { @class.new(:name => "foo", :host_aliases => ['alias1','']) }.should raise_error
     end
   end
+
+  describe "when syncing" do
+
+    it "should send the first value to the provider for ip property" do
+      @ip = @class.attrclass(:ip).new(:resource => @resource, :should => %w{192.168.0.1 192.168.0.2})
+      @provider.expects(:ip=).with '192.168.0.1'
+      @ip.sync
+    end
+
+    it "should send the first value to the provider for comment property" do
+      @comment = @class.attrclass(:comment).new(:resource => @resource, :should => %w{Bazinga Notme})
+      @provider.expects(:comment=).with 'Bazinga'
+      @comment.sync
+    end
+
+    it "should send the joined array to the provider for host_alias" do
+      @host_aliases = @class.attrclass(:host_aliases).new(:resource => @resource, :should => %w{foo bar})
+      @provider.expects(:host_aliases=).with 'foo bar'
+      @host_aliases.sync
+    end
+
+    it "should also use the specified delimiter for joining" do
+      @host_aliases = @class.attrclass(:host_aliases).new(:resource => @resource, :should => %w{foo bar})
+      @host_aliases.stubs(:delimiter).returns "\t"
+      @provider.expects(:host_aliases=).with "foo\tbar"
+      @host_aliases.sync
+    end
+
+    it "should care about the order of host_aliases" do
+      @host_aliases = @class.attrclass(:host_aliases).new(:resource => @resource, :should => %w{foo bar})
+      @host_aliases.insync?(%w{foo bar}).should == true
+      @host_aliases.insync?(%w{bar foo}).should == false
+    end
+
+    it "should not consider aliases to be in sync if should is a subset of current" do
+      @host_aliases = @class.attrclass(:host_aliases).new(:resource => @resource, :should => %w{foo bar})
+      @host_aliases.insync?(%w{foo bar anotherone}).should == false
+    end
+
+  end
 end
diff --git a/test/ral/providers/host/parsed.rb b/test/ral/providers/host/parsed.rb
index 1e847f8e7..521654d53 100755
--- a/test/ral/providers/host/parsed.rb
+++ b/test/ral/providers/host/parsed.rb
@@ -2,7 +2,6 @@
 
 require File.expand_path(File.dirname(__FILE__) + '/../../../lib/puppettest')
 
-require 'etc'
 require 'puppettest'
 require 'puppettest/fileparsing'
 require 'test/unit'
@@ -25,213 +24,9 @@ class TestParsedHostProvider < Test::Unit::TestCase
     super
   end
 
-  def test_provider_existence
-    assert(@provider, "Could not retrieve provider")
-  end
-
-  # Here we just create a fake host type that answers to all of the methods
-  # but does not modify our actual system.
-  def mkfaketype
-    @provider.filetype = Puppet::Util::FileType.filetype(:ram)
-  end
-
-  def mkhosthash
-    if defined?(@hcount)
-      @hcount += 1
-    else
-      @hcount = 1
-    end
-
-    return {
-      :name => "fakehost#{@hcount}",
-      :ip => "192.168.27.#{@hcount}",
-      :host_aliases => ["alias#{@hcount}"],
-      :ensure => :present
-    }
-  end
-
-  def mkhost
-    hash = mkhosthash
-
-    fakeresource = fakeresource(:host, hash[:name])
-
-    host = @provider.new(fakeresource)
-
-    assert(host, "Could not create provider host")
-    hash.each do |name, val|
-      host.send(name.to_s + "=", val)
-    end
-
-    host
-  end
-
-  # Make sure we convert both directlys correctly using a simple host.
-  def test_basic_isomorphism
-    hash = {:record_type => :parsed, :name => "myhost", :ip => "192.168.43.56", :host_aliases => %w{another host},
-            :comment => ''}
-
-    str = nil
-    assert_nothing_raised do
-      str = @provider.to_line(hash)
-    end
-
-    assert_equal("192.168.43.56\tmyhost\tanother\thost", str)
-
-    newhash = nil
-    assert_nothing_raised do
-      newhash = @provider.parse(str).shift
-    end
-
-    assert_equal(hash, newhash)
-  end
-
-  # Make sure parsing gets comments, blanks, and hosts
-  def test_blanks_and_comments
-    mkfaketype
-    text = %{# comment one
-
-192.168.43.56\tmyhost\tanother\thost
-    
-# another comment
-192.168.43.57\tanotherhost
-}
-
-    instances = nil
-    assert_nothing_raised do
-      instances = @provider.parse(text)
-    end
-
-
-          assert_equal(
-        [
-      {:record_type => :comment, :line => "# comment one"},
-      {:record_type => :blank, :line => ""},
-      {:record_type => :parsed, :name => "myhost", :ip => "192.168.43.56", :host_aliases => %w{another host},
-       :comment => ''},
-      {:record_type => :blank, :line => "    "},
-      {:record_type => :comment, :line => "# another comment"},
-        
-      {:record_type => :parsed, :name => "anotherhost", :ip => "192.168.43.57", :host_aliases => [],
-       :comment => ''}
-    ], instances)
-
-    newtext = nil
-    assert_nothing_raised do
-      newtext = @provider.to_file(instances).gsub(/^# HEADER.+\n/, '')
-    end
-
-    assert_equal(text, newtext)
-  end
-
-  def test_simplehost
-    mkfaketype
-    @provider.default_target = :yayness
-    file = @provider.target_object(:yayness)
-
-    # Start out with no content.
-    assert_nothing_raised {
-      assert_equal([], @provider.parse(file.read))
-    }
-
-    # Now create a provider
-    host = nil
-    assert_nothing_raised {
-      host = mkhost
-    }
-
-    # Make sure we're still empty
-    assert_nothing_raised {
-      assert_equal([], @provider.parse(file.read))
-    }
-
-    # Try storing it
-    assert_nothing_raised do
-      host.flush
-    end
-
-    # Make sure we get the host back
-    assert_nothing_raised {
-
-            assert(
-        file.read.include?(host.name),
-        
-        "Did not flush host to disk")
-    }
-
-    # Remove a single field and make sure it gets tossed
-    name = host.host_aliases
-    host.host_aliases = [:absent]
-
-    assert_nothing_raised {
-      host.flush
-
-            assert(
-        ! file.read.include?(name[0]),
-        
-        "Did not remove host_aliases from disk")
-    }
-
-    # Make sure it throws up if we remove a required field
-    host.ip = :absent
-
-    assert_raise(ArgumentError) {
-      host.flush
-    }
-
-    # Now remove the whole object
-    host.ensure = :absent
-    assert_nothing_raised {
-      host.flush
-      assert_equal([], @provider.parse(file.read))
-    }
-  end
-
   # Parse our sample data and make sure we regenerate it correctly.
   def test_hostsparse
     fakedata("data/types/hosts").each do |file| fakedataparse(file) end
   end
-
-  # Make sure we can modify the file elsewhere and those modifications will
-  # get taken into account.
-  def test_modifyingfile
-    hostfile = tempfile
-    @provider.default_target = hostfile
-
-    file = @provider.target_object(hostfile)
-
-    hosts = []
-    3.times {
-      h = mkhost
-      hosts << h
-    }
-
-    hosts.each do |host|
-      host.flush
-    end
-
-    newhost = mkhost
-    hosts << newhost
-
-    # Now store our new host
-    newhost.flush
-
-    # Verify we can retrieve that info
-    assert_nothing_raised("Could not retrieve after second write") {
-      @provider.prefetch
-    }
-
-    text = file.read
-
-    instances = @provider.parse(text)
-
-    # And verify that we have data for everything
-    hosts.each { |host|
-      name = host.resource[:name]
-      assert(text.include?(name), "Host #{name} is not in file")
-      hash = host.property_hash
-      assert(! hash.empty?, "Could not find host #{name}")
-      assert(hash[:ip], "Could not find ip for host #{name}")
-    }
-  end
 end