Fixed merging of state values, but I have not yet solved merging of parameter or metaparam values, since they are quite different.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@735 980ebf18-57e1-0310-9a29-db15c13687c0

6 files changed, 152 insertions, 102 deletions

diff --git a/lib/puppet/log.rb b/lib/puppet/log.rb
index 62fe98fdf..7c30e88b5 100644
--- a/lib/puppet/log.rb
+++ b/lib/puppet/log.rb
@@ -167,10 +167,10 @@ module Puppet # :nodoc:
                     # cannot log a message with a '%' in it.  So, we get rid
                     # of them.
                     if msg.source == "Puppet"
-                        dest.send(msg.level, msg.to_s.gsub("%", ''))
+                        dest.send(msg.level, msg.to_s.gsub("%", '%%'))
                     else
                         dest.send(msg.level, "(%s) %s" %
-                            [msg.source.to_s.gsub("%", ""), msg.to_s.gsub("%", '')]
+                            [msg.source.to_s.gsub("%", ""), msg.to_s.gsub("%", '%%')]
                         )
                     end
                 when File:
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 8a08152f8..9d728c07c 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -808,36 +808,46 @@ class Type < Puppet::Element
         self.class[self.name] = self
     end
 
-    # merge new information with an existing object, checking for conflicts
-    # and such
+    # Merge new information with an existing object, checking for conflicts
+    # and such.  This allows for two specifications of the same object and
+    # the same values, but it's pretty limited right now.  The result of merging
+    # states is very different from the result of merging parameters or metaparams.
     def merge(hash)
         hash.each { |param, value|
             if param.is_a?(String)
                 param = param.intern
             end
+            
+            # Of course names are the same, duh.
             next if param == :name or param == self.class.namevar
 
             unless value.is_a?(Array)
                 value = [value]
             end
 
-            # This needs some way to retrieve the original values specified to
-            # 'should'.
-            if oldvals = self.should(param)
-                if oldvals.is_a?(Array)
-                    # take the intersection
-                    newvals = oldvals & value
-                    if newvals.empty?
-                        raise Puppet::Error, "No common values for %s on %s(%s)" %
-                            [param, self.class.name, self.name]
-                    else
-                        self.debug "Reduced old values %s and new values %s to %s" %
-                            [oldvals.inspect, value.inspect, newvals.inspect]
-                        self.should = newvals
-                    end
-                else
-                    raise Puppet::Error, "Cannot override %s on %s(%s)" %
+            if oldvals = @states[param].shouldorig
+                unless oldvals.is_a?(Array)
+                    oldvals = [oldvals]
+                end
+                # If the values are exactly the same, order and everything,
+                # then it's okay.
+                if oldvals == value
+                    return true
+                end
+                # take the intersection
+                newvals = oldvals & value
+                if newvals.empty?
+                    raise Puppet::Error, "No common values for %s on %s(%s)" %
+                        [param, self.class.name, self.name]
+                elsif newvals.length > 1
+                    raise Puppet::Error, "Too many values for %s on %s(%s)" %
                         [param, self.class.name, self.name]
+                else
+                    self.debug "Reduced old values %s and new values %s to %s" %
+                        [oldvals.inspect, value.inspect, newvals.inspect]
+                    @states[param].should = newvals
+                    #self.should = newvals
+                    return true
                 end
             else
                 self[param] = value
diff --git a/lib/puppet/type/pfile/uid.rb b/lib/puppet/type/pfile/uid.rb
index b61fa8af7..299d92756 100755
--- a/lib/puppet/type/pfile/uid.rb
+++ b/lib/puppet/type/pfile/uid.rb
@@ -20,6 +20,40 @@ module Puppet
                 end
             end
 
+            def name2id(value)
+                begin
+                    user = Etc.getpwnam(value)
+                    if user.uid == ""
+                        return nil
+                    end
+                    return user.uid
+                rescue ArgumentError => detail
+                    return nil
+                end
+            end
+
+            # Determine if the user is valid, and if so, return the UID
+            def validuser?(value)
+                if value =~ /^\d+$/
+                    value = value.to_i
+                end
+
+                if value.is_a?(Integer)
+                    # verify the user is a valid user
+                    if tmp = id2name(value)
+                        return value
+                    else
+                        return false
+                    end
+                else
+                    if tmp = name2id(value)
+                        return tmp
+                    else
+                        return false
+                    end
+                end
+            end
+
             # We want to print names, not numbers
             def is_to_s
                 id2name(@is) || @is
@@ -38,49 +72,17 @@ module Puppet
                 self.is = stat.uid
             end
 
-            # If we're not root, we can check the values but we cannot change them
+            # If we're not root, we can check the values but we cannot change them.
+            # We can't really do any processing here, because users might
+            # not exist yet.  FIXME There's still a bit of a problem here if
+            # the user's UID changes at run time, but we're just going to have
+            # to be okay with that for now, unfortunately.
             def shouldprocess(value)
-                if value.is_a?(Integer)
-                    # verify the user is a valid user
-                    begin
-                        user = Etc.getpwuid(value)
-                        if user.uid == ""
-                            error = Puppet::Error.new(
-                                "Could not retrieve uid for '%s'" %
-                                    @parent.name)
-                            raise error
-                        end
-                    rescue ArgumentError => detail
-                        raise Puppet::Error.new("User ID %s does not exist" %
-                            value
-                        )
-                    rescue => detail
-                        raise Puppet::Error.new(
-                            "Could not find user '%s': %s" % [value, detail])
-                        raise error
-                    end
+                if tmp = self.validuser?(value)
+                    return tmp
                 else
-                    begin
-                        user = Etc.getpwnam(value)
-                        if user.uid == ""
-                            error = Puppet::Error.new(
-                                "Could not retrieve uid for '%s'" %
-                                    @parent.name)
-                            raise error
-                        end
-                        value = user.uid
-                    rescue ArgumentError => detail
-                        raise Puppet::Error.new("User %s does not exist" %
-                            value
-                        )
-                    rescue => detail
-                        error = Puppet::Error.new(
-                            "Could not find user '%s': %s" % [value, detail])
-                        raise error
-                    end
+                    return value
                 end
-
-                return value
             end
 
             def sync
@@ -90,13 +92,25 @@ module Puppet
                         #@parent.delete(self.name)
                         @@notifieduid = true
                     end
-                    # there's a possibility that we never got retrieve() called
-                    # e.g., if the file didn't exist
-                    # thus, just delete ourselves now and don't do any work
-                    #@parent.delete(self.name)
                     return nil
                 end
 
+                user = nil
+                unless user = self.validuser?(self.should)
+                    tmp = self.should
+                    unless defined? @@usermissing
+                        @@usermissing = {}
+                    end
+
+                    if @@usermissing.include?(tmp)
+                        @@usermissing[tmp] += 1
+                    else
+                        self.notice "user %s does not exist" % tmp
+                        @@usermissing[tmp] = 1
+                        return nil
+                    end
+                end
+
                 if @is == :notfound
                     @parent.stat(true)
                     self.retrieve
@@ -111,10 +125,10 @@ module Puppet
                 end
 
                 begin
-                    File.chown(self.should,nil,@parent[:path])
+                    File.chown(user, nil, @parent[:path])
                 rescue => detail
                     raise Puppet::Error, "Failed to set owner to '%s': %s" %
-                        [self.should,detail]
+                        [user, detail]
                 end
 
                 return :inode_changed
diff --git a/test/other/log.rb b/test/other/log.rb
index f25b01149..0fd72887a 100644
--- a/test/other/log.rb
+++ b/test/other/log.rb
@@ -141,4 +141,16 @@ class TestLog < Test::Unit::TestCase
         assert_equal(log.tags, file.tags, "Tags were not equal")
         assert_equal(log.source, file.path, "Source was not set correctly")
     end
+
+    # Verify that we can pass strings that match printf args
+    def test_percentlogs
+        Puppet[:logdest] = :syslog
+
+        assert_nothing_raised {
+            Puppet::Log.new(
+                :level => :info,
+                :message => "A message with %s in it"
+            )
+        }
+    end
 end
diff --git a/test/types/file.rb b/test/types/file.rb
index 4ff0705c0..b160ae1da 100644
--- a/test/types/file.rb
+++ b/test/types/file.rb
@@ -32,12 +32,12 @@ class TestFile < Test::Unit::TestCase
     end
 
     def setup
+        super
         begin
             initstorage
         rescue
             system("rm -rf %s" % Puppet[:checksumfile])
         end
-        super
     end
 
     def teardown
@@ -192,14 +192,15 @@ class TestFile < Test::Unit::TestCase
                 assert(file.insync?())
             }
 
-            fake.each { |uid, name|
-                assert_raise(Puppet::Error) {
-                    file[:owner] = name
-                }
-                assert_raise(Puppet::Error) {
-                    file[:owner] = uid
-                }
-            }
+            # We no longer raise an error here, because we check at run time
+            #fake.each { |uid, name|
+            #    assert_raise(Puppet::Error) {
+            #        file[:owner] = name
+            #    }
+            #    assert_raise(Puppet::Error) {
+            #        file[:owner] = uid
+            #    }
+            #}
         end
 
         def test_groupasroot
@@ -518,30 +519,6 @@ class TestFile < Test::Unit::TestCase
             file.sync
         }
     end
-
-    if Process.uid == 0
-    def test_zfilewithpercentsign
-        file = nil
-        dir = tmpdir()
-        path = File.join(dir, "file%sname")
-        assert_nothing_raised {
-            file = Puppet::Type::PFile.create(
-                :path => path,
-                :create => true,
-                :owner => "nosuchuser",
-                :group => "root",
-                :mode => "755"
-            )
-        }
-
-        comp = newcomp("percent", file)
-        events = nil
-        assert_nothing_raised {
-            trans = comp.evaluate
-            events = trans.evaluate
-        }
-    end
-    end
 end
 
 # $Id$
diff --git a/test/types/type.rb b/test/types/type.rb
index 4347ea0b0..55b809372 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -1,11 +1,9 @@
 if __FILE__ == $0
     $:.unshift '..'
     $:.unshift '../../lib'
-    $puppetbase = "../../../../language/trunk"
+    $puppetbase = "../.."
 end
 
-# $Id$
-
 require 'puppet/type'
 require 'puppettest'
 require 'test/unit'
@@ -111,4 +109,43 @@ class TestType < Test::Unit::TestCase
 
         assert_equal("testing", group.name, "Could not retrieve name")
     end
+
+    # Verify that values get merged correctly
+    def test_mergestatevalues
+        file = tempfile()
+
+        # Create the first version
+        assert_nothing_raised {
+            Puppet::Type::PFile.create(
+                :path => file,
+                :owner => ["root", "bin"]
+            )
+        }
+
+        # Make an identical statement
+        assert_nothing_raised {
+            Puppet::Type::PFile.create(
+                :path => file,
+                :owner => ["root", "bin"]
+            )
+        }
+
+        # Create a conflicting statement
+        assert_raise(Puppet::Error) {
+            Puppet::Type::PFile.create(
+                :path => file,
+                :owner => ["root", "bin", "adm"]
+            )
+        }
+
+        # And then make an intersection with one valid value
+        assert_nothing_raised {
+            Puppet::Type::PFile.create(
+                :path => file,
+                :owner => "root"
+            )
+        }
+    end
 end
+
+# $Id$