Fixing users so that they can use a group created by Puppet, and they also now autorequire that group. To do so, I modified Puppet::Util.gid, which required that I fix Puppet::Type#merge to support merging managed and umanaged objects, which required fixing a bug in Puppet::Type#managed?, and I also changed the ensure state to only default to a value, when the object is managed, which required that I change the defaults system to support default procs that do not return a value. In other words, lots of fixes for a smallish problem, but we are much better off now.

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

9 files changed, 186 insertions, 84 deletions

diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb
index a3a935b00..25789e9af 100644
--- a/lib/puppet/parameter.rb
+++ b/lib/puppet/parameter.rb
@@ -16,9 +16,9 @@ module Puppet
             end
 
             def nodefault
-                undef_method :default
-                #if defined_method? :default
-                #end
+                if public_method_defined? :default
+                    undef_method :default
+                end
             end
 
             # Store documentation for this parameter.
@@ -42,6 +42,9 @@ module Puppet
                         Puppet.debug "Reraising %s" % detail
                         raise
                     rescue => detail
+                        if Puppet[:debug]
+                            puts detail.backtrace
+                        end
                         raise Puppet::DevError, "Munging failed for class %s: %s" %
                             [self.name, detail]
                     end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 0c0fc2d98..c90185fc1 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -900,13 +900,17 @@ class Type < Puppet::Element
     # in order resolve other questions, such as finding a package
     # in a list
     def managed?
-        if defined? @managed
+        # Once an object is managed, it always stays managed; but an object
+        # that is listed as unmanaged might become managed later in the process,
+        # so we have to check that every time
+        if defined? @managed and @managed
             return @managed
         else
             @managed = false
             states.each { |state|
                 if state.should and ! state.class.unmanaged
                     @managed = true
+                    break
                 end
             }
             return @managed
@@ -1107,10 +1111,16 @@ class Type < Puppet::Element
 
                 return retobj
             else
+                # If only one of the objects is being managed, then merge them
+                if retobj.managed?
+                    raise Puppet::Error, "%s '%s' is already being managed" %
+                        [self.name, name]
+                else
+                    retobj.merge(hash)
+                    return retobj
+                end
                 # We will probably want to support merging of some kind in
                 # the future, but for now, just throw an error.
-                raise Puppet::Error, "%s '%s' is already being managed" %
-                    [self.name, name]
                 #retobj.merge(hash)
 
                 #return retobj
@@ -1341,6 +1351,13 @@ class Type < Puppet::Element
         if self.respond_to?(:validate)
             self.validate
         end
+
+        # Ensure defaults to present for managed objects, but not otherwise.
+        # Because of this complication, we can't use normal defaulting mechanisms
+#        if ! @states.include?(:ensure) and self.managed? and
+#                    self.class.validstate?(:ensure)
+#            self[:ensure] = :present
+#        end
     end
 
     # Figure out of there are any objects we can automatically add as
@@ -1358,12 +1375,19 @@ class Type < Puppet::Element
 
             # Collect the current prereqs
             list.each { |dep|
-                # Skip autorequires that we aren't managing
-                next unless obj = typeobj[dep]
+                obj = nil
+                # Support them passing objects directly, to save some effort.
+                if dep.is_a? Puppet::Type
+                    type = dep.class.name
+                    obj = dep
+                else
+                    # Skip autorequires that we aren't managing
+                    next unless obj = typeobj[dep]
+                end
 
                 # Skip autorequires that we already require
                 next if self.requires?(obj)
-                self.info "Auto-requiring %s" % obj.name
+                #self.info "Auto-requiring %s %s" % [obj.class.name, obj.name]
 
                 self[:require] = [type, dep]
             }
@@ -1473,8 +1497,13 @@ class Type < Puppet::Element
             next if self.attrset?(type, klass.name)
 
             obj = self.newattr(type, klass)
-            #self.debug "defaulting %s to %s" % [obj.name, obj.default]
-            obj.value = obj.default
+            if value = obj.default
+                #self.debug "defaulting %s to %s" % [obj.name, obj.default]
+                obj.value = value
+            else
+                #self.debug "No default for %s" % obj.name
+                self.delete(obj.name)
+            end
         }
 
     end
@@ -1497,7 +1526,7 @@ class Type < Puppet::Element
                 value = [value]
             end
 
-            if oldvals = @states[param].shouldorig
+            if @states.include?(param) and oldvals = @states[param].shouldorig
                 unless oldvals.is_a?(Array)
                     oldvals = [oldvals]
                 end
@@ -1525,6 +1554,9 @@ class Type < Puppet::Element
                 self[param] = value
             end
         }
+
+        # Set the defaults again, just in case.
+        self.setdefaults
     end
 
     # derive the instance name based on class.namevar
diff --git a/lib/puppet/type/nameservice.rb b/lib/puppet/type/nameservice.rb
index 9d6c2ba62..2f812ac61 100755
--- a/lib/puppet/type/nameservice.rb
+++ b/lib/puppet/type/nameservice.rb
@@ -52,7 +52,7 @@ class State
             # things like useradd, in which creation can be done with all
             # information in one swell foop.
             def allatonce?
-                Puppet.info "Returning allatonce %s" % @allatonce
+                #Puppet.info "Returning allatonce %s" % @allatonce
                 if defined? @allatonce
                     return @allatonce
                 else
diff --git a/lib/puppet/type/pfile/ensure.rb b/lib/puppet/type/pfile/ensure.rb
index 4659b44fe..ed5d624f1 100755
--- a/lib/puppet/type/pfile/ensure.rb
+++ b/lib/puppet/type/pfile/ensure.rb
@@ -12,11 +12,6 @@ module Puppet
         # Most 'ensure' states have a default, but with files we, um, don't.
         nodefault
 
-        #newvalue(:false) do
-        #    # If they say "false" here, we just don't do anything at all; either
-        #    # the file is there or it's not.
-        #end
-
         newvalue(:absent) do
             File.unlink(@parent.name)
         end
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index f090f38ec..38cf7fc9e 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -340,7 +340,14 @@ class State < Puppet::Parameter
 
         # If they're talking about the thing at all, they generally want to
         # say it should exist.
-        defaultto :present
+        #defaultto :present
+        defaultto do
+            if @parent.managed?
+                :present
+            else
+                nil
+            end
+        end
     end
 end
 end
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 9ae07e679..b789ad61d 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -82,19 +82,30 @@ module Puppet
                     return gid
                 end
 
-                # FIXME this should really check to see if we already have a
-                # group ready to be managed; if so, then we should just mark it
-                # as a prereq
-                begin
-                    ginfo = Etc.send(method, gid)
-                rescue ArgumentError => detail
-                    self.fail "Could not find group %s: %s" %
-                        [gid, detail]
+                if group = Puppet::Util.gid(gid)
+                    @found = true
+                    return group
+                else
+                    @found = false
+                    return gid
                 end
+            end
 
-                self.notice "setting gid to %s" % ginfo.gid.inspect
-                return ginfo.gid
+            # *shudder*  Make sure that we've looked up the group and gotten
+            # an ID for it.  Yuck-o.
+            def should
+                unless defined? @should
+                    return super
+                end
+                unless defined? @found and @found
+                    @should = @should.each { |val|
+                        next unless val
+                        Puppet::Util.gid(val)
+                    }
+                end
+                super
             end
+
         end
 
         newstate(:comment, @parentstate) do
@@ -167,6 +178,47 @@ module Puppet
 
         @netinfodir = "users"
 
+        # Autorequire the group, if it's around
+        autorequire(:group) do
+            #return nil unless @states.include?(:gid)
+            #return nil unless groups = @states[:gid].shouldorig
+            autos = []
+
+            if @states.include?(:gid) and groups = @states[:gid].shouldorig
+                groups = groups.collect { |group|
+                    if group =~ /^\d+$/
+                        Integer(group)
+                    else
+                        group
+                    end
+                }
+                groups.each { |group|
+                    case group
+                    when Integer:
+                        if obj = Puppet.type(:group).find { |gobj|
+                            gobj.should(:gid) == group
+                        }
+                            autos << obj
+                            
+                        end
+                    else
+                        autos << group
+                    end
+                }
+            end
+
+            autos
+        end
+
+        autorequire(:file) do
+            dir = self.should(:home) or self.is(:home)
+            if dir =~ /^#{File::SEPARATOR}/
+                dir
+            else
+                nil
+            end
+        end
+
         def exists?
             self.class.parentmodule.exists?(self)
         end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 44b3bae44..0d2d73900 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -126,21 +126,29 @@ module Util
         if group =~ /^\d+$/
             group = Integer(group)
         end
+        unless group
+            raise Puppet::DevError, "Invalid group %s" % group.inspect
+        end
         gid = nil
+        obj = nil
+
+        # We want to look the group up either way
         if group.is_a?(Integer)
-            gid = group
+            obj = Puppet.type(:group).find { |gobj|
+                gobj.should(:gid) == group ||
+                    gobj.is(:gid) == group
+            }
         else
             unless obj = Puppet.type(:group)[group]
                 obj = Puppet.type(:group).create(
                     :name => group,
                     :check => [:gid]
                 )
+                obj.retrieve
             end
-            obj.retrieve
-            gid = obj.is(:gid)
-            unless gid.is_a?(Integer)
-                raise Puppet::Error, "Could not find group %s" % group
-            end
+        end
+        if obj
+            gid = obj.should(:gid) || obj.is(:gid)
         end
 
         return gid
@@ -171,53 +179,6 @@ module Util
         return uid
     end
 
-    # Create a lock file while something is happening
-    def self.disabledlock(*opts)
-        lock = opts[0] + ".lock"
-        while File.exists?(lock)
-            stamp = File.stat(lock).mtime.to_i 
-            if Time.now.to_i - stamp > 5
-                Puppet.notice "Lock file %s is %s seconds old; removing" %
-                    [lock, Time.now.to_i - stamp]
-                File.delete(lock)
-                break
-            else
-                sleep 0.1
-            end
-            #Puppet.debug "%s is locked" % opts[0]
-        end
-        File.open(lock, "w") { |f| f.print " "; f.flush }
-        writing = false
-        if opts[1] == "w"
-            writing = true
-            tmp = opts[0] + ".tmp"
-            orig = opts[0]
-            opts[0] = tmp
-        end
-        begin
-            File.open(*opts) { |file| yield file }
-            begin
-                if writing
-                    Puppet.warning "opts were %s" % opts.inspect
-                    system("ls -l %s 2>/dev/null" % tmp)
-                    system("ls -l %s 2>/dev/null" % orig)
-                    File.rename(tmp, orig)
-                end
-            rescue => detail
-                Puppet.err "Could not replace %s: %s" % [orig, detail]
-                File.unlink(tmp)
-            end
-        rescue => detail
-            Puppet.err "Storage error: %s" % detail
-            raise
-        ensure
-            # I don't really understand how the lock file could disappear,
-            # but just in case...
-            if FileTest.exists?(lock)
-                File.delete(lock)
-            end
-        end
-    end
 
     # Create instance methods for each of the log levels.  This allows
     # the messages to be a little richer.  Most classes will be calling this
diff --git a/test/types/type.rb b/test/types/type.rb
index e7808228a..abb3178c6 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -209,7 +209,38 @@ class TestType < Test::Unit::TestCase
         assert_apply(file)
 
         assert(Puppet.type(:file)[name], "Could not look up object by name")
-        #assert(Puppet.type(:file)[path], "Could not look up object by path")
+    end
+
+    def test_ensuredefault
+        user = nil
+        assert_nothing_raised {
+            user = Puppet.type(:user).create(
+                :name => "pptestAA",
+                :check => [:uid]
+            )
+        }
+
+        # make sure we don't get :ensure for unmanaged files
+        assert(! user.state(:ensure), "User got an ensure state")
+
+        assert_nothing_raised {
+            user = Puppet.type(:user).create(
+                :name => "pptestAA",
+                :comment => "Testingness"
+            )
+        }
+        # but make sure it gets added once we manage them
+        assert(user.state(:ensure), "User did not add ensure state")
+
+        assert_nothing_raised {
+            user = Puppet.type(:user).create(
+                :name => "pptestBB",
+                :comment => "A fake user"
+            )
+        }
+
+        # and make sure managed objects start with them
+        assert(user.state(:ensure), "User did not get an ensure state")
     end
 end
 
diff --git a/test/types/user.rb b/test/types/user.rb
index a822747d1..6a7767054 100755
--- a/test/types/user.rb
+++ b/test/types/user.rb
@@ -250,7 +250,6 @@ class TestUser < Test::Unit::TestCase
 
         newuid = old
         while true
-            Puppet.warning newuid.inspect
             newuid += 1
 
             if newuid - old > 1000
@@ -367,6 +366,28 @@ class TestUser < Test::Unit::TestCase
                 end
             }
         end
+
+        def test_autorequire
+            file = tempfile()
+            user = Puppet.type(:user).create(
+                :name => "pptestu",
+                :home => file,
+                :gid => "pptestg"
+            )
+            home = Puppet.type(:file).create(
+                :path => file,
+                :ensure => "directory"
+            )
+            group = Puppet.type(:group).create(
+                :name => "pptestg"
+            )
+            Puppet::Type.finalize
+            comp = newcomp(user, group)
+            comp.retrieve
+
+            assert(user.requires?(group), "User did not require group")
+            assert(user.requires?(home), "User did not require home dir")
+        end
     else
         $stderr.puts "Not root; skipping user creation/modification tests"
     end