redoing how arguments are handled in type.rb -- it is much cleaner now

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

3 files changed, 196 insertions, 128 deletions

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index ba64b184a..1b6be5838 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -57,6 +57,7 @@ class Type < Puppet::Element
     # the parameters that all instances will accept
     @@metaparams = [
         :onerror,
+        :noop,
         :schedule,
         :check,
         :require
@@ -392,6 +393,9 @@ class Type < Puppet::Element
             name = name.intern
         end
 
+        if name == :name
+            name = self.class.namevar
+        end
         if self.class.validstate?(name)
             if @states.include?(name)
                 return @states[name].is
@@ -414,43 +418,48 @@ class Type < Puppet::Element
     # abstract setting parameters and states, and normalize
     # access to always be symbols, not strings
     def []=(name,value)
-        mname = name
         if name.is_a?(String)
-            mname = name.intern
+            name = name.intern
         end
 
-        if Puppet::Type.metaparam?(mname)
+        if name == :name
+            name = self.class.namevar
+        end
+        if value.nil?
+            raise Puppet::Error.new("Got nil value for %s" % name)
+        end
+        if Puppet::Type.metaparam?(name)
             # call the metaparam method 
-            self.send(("meta" + mname.id2name + "="),value)
-        elsif stateklass = self.class.validstate?(mname) 
+            self.send(("meta" + name.id2name + "="),value)
+        elsif stateklass = self.class.validstate?(name) 
             if value.is_a?(Puppet::State)
-                Puppet.debug "'%s' got handed a state for '%s'" % [self,mname]
-                @states[mname] = value
+                Puppet.debug "'%s' got handed a state for '%s'" % [self,name]
+                @states[name] = value
             else
-                if @states.include?(mname)
-                    @states[mname].should = value
+                if @states.include?(name)
+                    @states[name].should = value
                 else
                     #Puppet.warning "Creating state %s for %s" %
                     #    [stateklass.name,self.name]
-                    @states[mname] = stateklass.new(
+                    @states[name] = stateklass.new(
                         :parent => self,
                         :should => value
                     )
-                    #debug "Adding parent to %s" % mname
-                    #@states[mname].parent = self
+                    #debug "Adding parent to %s" % name
+                    #@states[name].parent = self
                 end
             end
-        elsif self.class.validparameter?(mname)
+        elsif self.class.validparameter?(name)
             # if they've got a method to handle the parameter, then do it that way
-            method = "param" + mname.id2name + "="
+            method = "param" + name.id2name + "="
             if self.respond_to?(method)
                 self.send(method,value)
             else
                 # else just set it
-                @parameters[mname] = value
+                @parameters[name] = value
             end
         else
-            raise "Invalid parameter %s" % [mname]
+            raise "Invalid parameter %s" % [name]
         end
     end
     #---------------------------------------------------------------
@@ -614,27 +623,28 @@ class Type < Puppet::Element
         @syncedchanges = 0
         @failedchanges = 0
 
-        hash.each { |var,value|
-            unless var.is_a? Symbol
-                hash[var.intern] = value
-                hash.delete(var)
-            end
-        }
+        hash = self.argclean(hash)
 
-        if hash.include?(:noop)
-            @noop = hash[:noop]
-            hash.delete(:noop)
-        end
+        # now get all of the arguments, in a specific order
+        order = [self.class.namevar]
+        order << self.class.parameters
+        order << self.class.eachmetaparam { |param| param }
+        order << self.class.states.collect { |state| state.name }
 
-        self.nameclean(hash)
+        order.flatten!
 
-        # this should probably do all states and then all parameters, right?
-        hash.each { |param,value|
-            #debug("adding param '%s' with value '%s'" %
-            #    [param,value])
-            self[param] = value
+        order.each { |name|
+            if hash.include?(name)
+                self[name] = hash[name]
+                hash.delete name
+            end
         }
 
+        if hash.length > 0
+            raise Puppet::Error.new("Class %s does not accept argument(s) %s" %
+                [self.class.name, hash.keys.join(" ")])
+        end
+
         # add this object to the specific class's list of objects
         #puts caller
         self.class[self.name] = self
@@ -653,27 +663,35 @@ class Type < Puppet::Element
 
     #---------------------------------------------------------------
     # fix any namevar => param translations
-    def nameclean(hash)
+    def argclean(hash)
         # we have to set the name of our object before anything else,
         # because it might be used in creating the other states
+        hash = hash.dup
         namevar = self.class.namevar
 
+        hash.each { |var,value|
+            unless var.is_a? Symbol
+                hash[var.intern] = value
+                hash.delete(var)
+            end
+        }
+
         # if they're not using :name for the namevar but we got :name (probably
         # from the parser)
         if namevar != :name and hash.include?(:name) and ! hash[:name].nil?
-            self[namevar] = hash[:name]
+            #self[namevar] = hash[:name]
+            hash[namevar] = hash[:name]
             hash.delete(:name)
         # else if we got the namevar
         elsif hash.has_key?(namevar) and ! hash[namevar].nil?
-            self[namevar] = hash[namevar]
-            hash.delete(namevar)
+            #self[namevar] = hash[namevar]
+            #hash.delete(namevar)
         # else something's screwy
         else
-            raise TypeError.new("A name must be provided to %s at initialization time" %
-                self.class)
+            # they didn't specify anything related to names
         end
 
-        #return hash
+        return hash
     end
     #---------------------------------------------------------------
 
@@ -816,12 +834,12 @@ class Type < Puppet::Element
 
         states.each { |state|
             unless state.insync?
-                Puppet.debug("%s is not in sync" % state)
+                #Puppet.debug("%s is not in sync" % state)
                 insync = false
             end
         }
 
-        Puppet.debug("%s sync status is %s" % [self,insync])
+        #Puppet.debug("%s sync status is %s" % [self,insync])
         return insync
     end
     #---------------------------------------------------------------
@@ -940,6 +958,18 @@ class Type < Puppet::Element
     #---------------------------------------------------------------
 
     #---------------------------------------------------------------
+    def metanoop=(noop)
+        if noop == "true" or noop == true
+            @noop = true
+        elsif noop == "false" or noop == false
+            @noop = false
+        else
+            raise Puppet::Error.new("Invalid noop value '%s'" % noop)
+        end
+    end
+    #---------------------------------------------------------------
+
+    #---------------------------------------------------------------
     def metaonerror=(response)
         Puppet.debug("Would have called metaonerror")
         @onerror = response
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 335058399..89fae0ec4 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -40,7 +40,7 @@ module Puppet
                     @is = -1
                 end
 
-                Puppet.debug "'exists' state is %s" % self.is
+                #Puppet.debug "'exists' state is %s" % self.is
             end
 
 
@@ -154,7 +154,7 @@ module Puppet
 
                 self.is = sum
 
-                Puppet.debug "checksum state is %s" % self.is
+                #Puppet.debug "checksum state is %s" % self.is
             end
 
 
@@ -218,7 +218,8 @@ module Puppet
                 if state[self.parent.name].include?(@checktype)
                     unless defined? @should
                         raise Puppet::Error.new(
-                            "@should is not initialized for %s, even though we found a checksum" % self.parent[:path]
+                            ("@should is not initialized for %s, even though we " +
+                            "found a checksum") % self.parent[:path]
                         )
                     end
                     Puppet.debug "Replacing checksum %s with %s" %
@@ -336,8 +337,8 @@ module Puppet
                                         self.parent.name)
                                 raise error
                             end
-                            Puppet.debug "converting %s to integer '%d'" %
-                                [@should,user.uid]
+                            #Puppet.debug "converting %s to integer '%d'" %
+                            #    [@should,user.uid]
                             @should = user.uid
                         rescue => detail
                             error = Puppet::Error.new(
@@ -347,7 +348,7 @@ module Puppet
                     end
                 end
 
-                Puppet.debug "chown state is %d" % self.is
+                #Puppet.debug "chown state is %d" % self.is
             end
 
             def sync
@@ -399,7 +400,7 @@ module Puppet
                 stat = self.parent.stat(true)
                 self.is = stat.mode & 007777
 
-                Puppet.debug "chmod state is %o" % self.is
+                #Puppet.debug "chmod state is %o" % self.is
             end
 
             def sync
@@ -490,8 +491,8 @@ module Puppet
                                     "Could not retrieve gid for %s" % self.parent.name)
                                 raise error
                             end
-                            Puppet.debug "converting %s to integer %d" %
-                                [self.should,gid]
+                            #Puppet.debug "converting %s to integer %d" %
+                            #    [self.should,gid]
                             self.should = gid
                         rescue => detail
                             error = Puppet::Error.new(
@@ -500,7 +501,7 @@ module Puppet
                         end
                     end
                 end
-                Puppet.debug "chgrp state is %d" % self.is
+                #Puppet.debug "chgrp state is %d" % self.is
             end
 
             def sync
@@ -674,9 +675,9 @@ module Puppet
                 #:source,
             @parameters = [
                 :path,
+                :source,
                 :recurse,
                 :filebucket,
-                :source,
                 :backup
             ]
 
@@ -685,24 +686,11 @@ module Puppet
 
             @depthfirst = false
 
-            def self.newchild(hash)
-            end
-
             def initialize(hash)
-                if hash.include?("recurse")
-                    hash[:recurse] = hash["recurse"]
-                    hash.delete("recurse")
-                end
+                @arghash = self.argclean(hash)
+                @arghash.delete(self.class.namevar)
 
                 @stat = nil
-
-                @arghash = hash.dup
-                @arghash.each { |var,value|
-                    if var.is_a?(String)
-                        @arghash[var.intern] = value
-                        @arghash.delete(var)
-                    end
-                }
                 super
             end
 
@@ -729,6 +717,15 @@ module Puppet
                     )
                 end
 
+                # ...and that it's readable
+                unless FileTest.readable?(@source)
+                    Puppet.notice "Skipping unreadable %s" % @source
+                    #raise Puppet::Error.new(
+                    #    "Files must exist to be sources; %s does not" % @source
+                    #)
+                    return
+                end
+
                 # Check whether we'll be creating the file or whether it already
                 # exists.  The root of the destination tree will cause the
                 # recursive creation of all of the objects, and then all the
@@ -932,46 +929,55 @@ module Puppet
                     end
                 end
 
-                # unless we're at the end of the recursion
-                if recurse != 0
-                    @arghash.delete("recurse")
-                    if recurse.is_a?(Integer)
-                        recurse -= 1 # reduce the level of recursion
-                    end
+                # are we at the end of the recursion?
+                if recurse == 0
+                    return
+                end
 
-                    @arghash[:recurse] = recurse
+                @arghash.delete("recurse")
 
+                if recurse.is_a?(Integer)
+                    recurse -= 1 # reduce the level of recursion
+                end
 
-                    # make sure we don't have any remaining ':name' params
-                    self.nameclean(@arghash)
+                @arghash[:recurse] = recurse
 
-                    Dir.foreach(self.name) { |file|
-                        next if file =~ /^\.\.?/ # skip . and ..
 
-                        @arghash[:path] = File.join(self.name,file)
+                # make sure we don't have any remaining ':name' params
+                args = self.argclean(@arghash)
 
-                        child = nil
-                        # if the file already exists...
-                        if child = self.class[@arghash[:path]]
-                            @arghash.each { |var,value|
-                                next if var == :path
-                                child[var] = value
-                            }
-                        else # create it anew
-                            #notice "Creating new file with args %s" %
-                            #    @arghash.inspect
-                            begin
-                                child = self.class.new(@arghash)
-                            rescue => detail
-                                Puppet.notice(
-                                    "Cannot manage %s: %s" % [@arghash[:path],detail]
-                                )
-                                next
-                            end
-                        end
-                        self.push child
-                    }
+                unless FileTest.directory? self.name
+                    raise Puppet::Error.new("Uh, somehow trying to manage non-dir %s" % self.name)
                 end
+                Dir.foreach(self.name) { |file|
+                    next if file =~ /^\.\.?/ # skip . and ..
+                    args = @arghash.dup
+
+                    args[:name] = File.join(self.name,file)
+
+                    child = nil
+                    # if the file already exists...
+                    if child = self.class[args[:name]]
+                        args.each { |var,value|
+                            next if var == :path
+                            next if var == :name
+                            child[var] = value
+                        }
+                    else # create it anew
+                        #notice "Creating new file with args %s" %
+                        #    @arghash.inspect
+                        begin
+                            child = self.class.new(args)
+                        rescue => detail
+                            Puppet.notice(
+                                "Cannot manage %s(%s): %s" %
+                                    [args[:name],args.inspect,detail]
+                            )
+                            next
+                        end
+                    end
+                    self.push child
+                }
             end
 
             # I don't currently understand the problems of dependencies in this space
@@ -989,7 +995,7 @@ module Puppet
             # a wrapper method to make sure the file exists before doing anything
             def retrieve
                 unless stat = self.stat(true)
-                    Puppet.debug "File %s does not exist" % self[:path]
+                    Puppet.debug "File %s does not exist" % self.name
                     @states.each { |name,state|
                         state.is = -1
                     }
@@ -1001,10 +1007,10 @@ module Puppet
             def stat(refresh = false)
                 if @stat.nil? or refresh == true
                     begin
-                        @stat = File.stat(self[:path])
+                        @stat = File.stat(self.name)
                     rescue => error
                         Puppet.debug "Failed to stat %s: %s" %
-                            [self[:path],error]
+                            [self.name,error]
                         @stat = nil
                     end
                 end
diff --git a/test/types/tc_file.rb b/test/types/tc_file.rb
index 0eeab9bb9..ffb3b4f91 100644
--- a/test/types/tc_file.rb
+++ b/test/types/tc_file.rb
@@ -30,8 +30,10 @@ class TestFile < Test::Unit::TestCase
     def teardown
         Puppet::Type.allclear
         @@tmpfiles.each { |file|
-            system("chmod -R 755 %s" % file)
-            system("rm -rf %s" % file)
+            if FileTest.exists?(file)
+                system("chmod -R 755 %s" % file)
+                system("rm -rf %s" % file)
+            end
         }
         system("rm -f %s" % Puppet[:statefile])
     end
@@ -461,7 +463,32 @@ class TestFile < Test::Unit::TestCase
         }
     end
 
-    def test_complicatedlocalsource
+    def delete_random_files(dir)
+        checked = 0
+        list = nil
+        FileUtils.cd(dir) {
+            list = %x{find . 2>/dev/null}.chomp.split(/\n/)
+        }
+        list.reverse.each_with_index { |file,i|
+            path = File.join(dir,file)
+            stat = File.stat(dir)
+            if checked < 10 and i % 3 == 0
+                begin
+                    if stat.ftype == "directory"
+                    else
+                        File.unlink(path)
+                    end
+                rescue => detail
+                    # we probably won't be able to open our own secured files
+                    puts detail
+                    next
+                end
+                checked += 1
+            end
+        }
+    end
+
+    def test_xcomplicatedlocalsource
         path = "/tmp/complsourcetest"
         @@tmpfiles.push path
         system("mkdir -p #{path}")
@@ -473,33 +500,38 @@ class TestFile < Test::Unit::TestCase
             mkranddirsandfiles()
         }
 
-        todir = File.join(path,"todir")
-        tofile = nil
-        trans = nil
+        2.times {
+            initstorage
+            todir = File.join(path,"todir")
+            tofile = nil
+            trans = nil
 
-        assert_nothing_raised {
-            tofile = Puppet::Type::PFile.new(
-                :name => todir,
-                "recurse" => true,
-                "source" => fromdir
+            assert_nothing_raised {
+                tofile = Puppet::Type::PFile.new(
+                    :name => todir,
+                    "recurse" => true,
+                    "source" => fromdir
+                )
+            }
+            comp = Puppet::Component.new(
+                :name => "component"
             )
+            comp.push tofile
+            assert_nothing_raised {
+                trans = comp.evaluate
+            }
+            assert_nothing_raised {
+                trans.evaluate
+            }
+            assert_trees_equal(fromdir,todir)
+            clearstorage
+            Puppet::Type.allclear
+            delete_random_files(todir)
         }
-        comp = Puppet::Component.new(
-            :name => "component"
-        )
-        comp.push tofile
-        assert_nothing_raised {
-            trans = comp.evaluate
-        }
-        assert_nothing_raised {
-            trans.evaluate
-        }
-        assert_trees_equal(fromdir,todir)
-        clearstorage
-        Puppet::Type.allclear
+
     end
 
-    def test_xcopywithfailures
+    def test_copywithfailures
         path = "/tmp/failuresourcetest"
         @@tmpfiles.push path
         system("mkdir -p #{path}")