Another round of bug-fixes, prompted by test logs from David Schmitt

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

15 files changed, 137 insertions, 132 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 3f3d87fa4..09f74eb96 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,8 @@
 0.22.2 (grover)
+    All parameter instances are stored in a single @parameters instance variable
+    hash within resource type instances.  We used to use separate hashes for
+    each parameter type.
+
     Added the concept of provider features.  Eventually these should be able
     to express the full range of provider functionality, but for now they can
     test a provider to see what methods it has set and determine what features it
diff --git a/lib/puppet/metatype/attributes.rb b/lib/puppet/metatype/attributes.rb
index a96533964..b4cc5f687 100644
--- a/lib/puppet/metatype/attributes.rb
+++ b/lib/puppet/metatype/attributes.rb
@@ -102,26 +102,28 @@ class Puppet::Type
         end
     end
 
-    # A similar function but one that yields the name, type, and class.
+    # A similar function but one that yields the class and type.
     # This is mainly so that setdefaults doesn't call quite so many functions.
     def self.eachattr(*ary)
-        # now get all of the arguments, in a specific order
-        # Cache this, since it gets called so many times
-
         if ary.empty?
             ary = nil
         end
-        self.properties.each { |property|
-            yield(property, :property) if ary.nil? or ary.include?(property.name)
-        }
-
-        @parameters.each { |param|
-            yield(param, :param) if ary.nil? or ary.include?(param.name)
-        }
 
-        @@metaparams.each { |param|
-            yield(param, :meta) if ary.nil? or ary.include?(param.name)
-        }
+        # We have to do this in a specific order, so that defaults are
+        # created in that order (e.g., providers should be set up before
+        # anything else).
+        allattrs.each do |name|
+            next unless ary.nil? or ary.include?(name)
+            if obj = @properties.find { |p| p.name == name }
+                yield obj, :property
+            elsif obj = @parameters.find { |p| p.name == name }
+                yield obj, :param
+            elsif obj = @@metaparams.find { |p| p.name == name }
+                yield obj, :meta
+            else
+                raise Puppet::DevError, "Could not find parameter %s" % name
+            end
+        end
     end
 
     def self.eachmetaparam
@@ -627,6 +629,7 @@ class Puppet::Type
     # set, set them now.  This method can be handed a list of attributes,
     # and if so it will only set defaults for those attributes.
     def setdefaults(*ary)
+        #self.class.eachattr(*ary) { |klass, type|
         self.class.eachattr(*ary) { |klass, type|
             # not many attributes will have defaults defined, so we short-circuit
             # those away
diff --git a/lib/puppet/metatype/providers.rb b/lib/puppet/metatype/providers.rb
index c078ced24..98b5caf43 100644
--- a/lib/puppet/metatype/providers.rb
+++ b/lib/puppet/metatype/providers.rb
@@ -45,74 +45,6 @@ class Puppet::Type
         return @defaultprovider
     end
 
-    # Define one or more features.  Currently, features are just a list of
-    # methods; if all methods are defined as instance methods on the provider,
-    # then the provider has that feature, otherwise it does not.
-    def self.dis_features(hash)
-        @features ||= {}
-        hash.each do |name, methods|
-            name = symbolize(name)
-            methods = methods.collect { |m| symbolize(m) }
-            if @features.include?(name)
-                raise Puppet::DevError, "Feature %s is already defined" % name
-            end
-            @features[name] = methods
-        end
-    end
-
-    # Generate a module that sets up the boolean methods to test for given
-    # features.
-    def self.dis_feature_module
-        unless defined? @feature_module
-            @features ||= {}
-            @feature_module = ::Module.new
-            const_set("FeatureModule", @feature_module)
-            features = @features
-            @feature_module.send(:define_method, :feature?) do |name|
-                method = name.to_s + "?"
-                if respond_to?(method) and send(method)
-                    return true
-                else
-                    return false
-                end
-            end
-            @feature_module.send(:define_method, :features) do
-                return false unless defined?(features)
-                features.keys.find_all { |n| feature?(n) }.sort { |a,b| 
-                    a.to_s <=> b.to_s 
-                }
-            end
-            #if defined?(@features)
-                @features.each do |name, methods|
-                    method = name.to_s + "?"
-                    @feature_module.send(:define_method, method) do
-                        set = nil
-                        methods.each do |m|
-                            if is_a?(Class)
-                                unless public_method_defined?(m)
-                                    set = false
-                                    break
-                                end
-                            else
-                                unless respond_to?(m)
-                                    set = false
-                                    break
-                                end
-                            end
-                        end
-
-                        if set.nil?
-                            true
-                        else
-                            false
-                        end
-                    end
-                end
-            #end
-        end
-        @feature_module
-    end
-
     # Convert a hash, as provided by, um, a provider, into an instance of self.
     def self.hash2obj(hash)
         obj = nil
@@ -267,7 +199,9 @@ class Puppet::Type
                 }.join("\n")
             end
 
-            defaultto { @parent.class.defaultprovider.name }
+            defaultto {
+                @parent.class.defaultprovider.name
+            }
 
             validate do |value|
                 value = value[0] if value.is_a? Array
diff --git a/lib/puppet/network/client/master.rb b/lib/puppet/network/client/master.rb
index 6ae7d55b3..d568b8ae7 100644
--- a/lib/puppet/network/client/master.rb
+++ b/lib/puppet/network/client/master.rb
@@ -128,27 +128,27 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
         end
     end
 
-    # Have the facts changed since we last compiled?
-    def facts_changed?(facts)
-        oldfacts = Puppet::Util::Storage.cache(:configuration)[:facts]
-        newfacts = self.class.facts
-        if oldfacts == newfacts
-            return false
-        else
-            return true
-        end
-    end
-
     # Check whether our configuration is up to date
     def fresh?(facts)
-        return false if Puppet[:ignorecache]
-        return false unless self.compile_time
-        return false if self.facts_changed?(facts)
+        if Puppet[:ignorecache]
+            Puppet.notice "Ignoring cache"
+            return false
+        end
+        unless self.compile_time
+            Puppet.debug "No cached compile time"
+            return false
+        end
+        if facts_changed?(facts)
+            Puppet.info "Facts have changed; recompiling"
+            return false
+        end
 
         # We're willing to give a 2 second drift
-        if @driver.freshness - @compile_time.to_i < 1
+        newcompile = @driver.freshness
+        if newcompile - @compile_time.to_i < 1
             return true
         else
+            Puppet.debug "Server compile time is %s vs %s" % [newcompile, @compile_time]
             return false
         end
     end
@@ -174,12 +174,15 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
         if self.objects or FileTest.exists?(self.cachefile)
             if self.fresh?(facts)
                 Puppet.info "Config is up to date"
-                begin
-                    @objects = YAML.load(self.retrievecache).to_type
-                rescue => detail
-                    Puppet.warning "Could not load cached configuration: %s" % detail
+                unless self.objects
+                    oldtext = self.retrievecache
+                    begin
+                        @objects = YAML.load(oldtext).to_type
+                    rescue => detail
+                        Puppet.warning "Could not load cached configuration: %s" % detail
+                    end
+                    return
                 end
-                return
             end
         end
         Puppet.debug("getting config")
@@ -513,6 +516,26 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
     loadfacts()
     
     private
+
+    # Have the facts changed since we last compiled?
+    def facts_changed?(facts)
+        oldfacts = Puppet::Util::Storage.cache(:configuration)[:facts]
+        newfacts = facts
+        if oldfacts == newfacts
+            return false
+        else
+#            unless oldfacts
+#                puts "no old facts"
+#                return true
+#            end
+#            newfacts.keys.each do |k|
+#                unless newfacts[k] == oldfacts[k]
+#                    puts "%s: %s vs %s" % [k, newfacts[k], oldfacts[k]]
+#                end
+#            end
+            return true
+        end
+    end
     
     # Actually retrieve the configuration, either from the server or from a
     # local master.
diff --git a/lib/puppet/network/handler/filebucket.rb b/lib/puppet/network/handler/filebucket.rb
index 96b9a1e9a..f0d389aea 100755
--- a/lib/puppet/network/handler/filebucket.rb
+++ b/lib/puppet/network/handler/filebucket.rb
@@ -98,9 +98,11 @@ class Puppet::Network::Handler # :nodoc:
             self.info msg
 
             # ...then just create the file
-            File.open(bfile, File::WRONLY|File::CREAT, 0440) { |of|
-                of.print contents
-            }
+            Puppet::Util.withumask(0007) do
+                File.open(bfile, File::WRONLY|File::CREAT, 0440) { |of|
+                    of.print contents
+                }
+            end
 
             # Write the path to the paths file.
             add_path(path, pathpath)
@@ -132,6 +134,10 @@ class Puppet::Network::Handler # :nodoc:
             end
         end
 
+        def paths(md5)
+            self.class(@path, md5)
+        end
+
         def to_s
             self.name
         end
diff --git a/lib/puppet/provider/cron/crontab.rb b/lib/puppet/provider/cron/crontab.rb
index b7caa4a21..bd03c21e3 100755
--- a/lib/puppet/provider/cron/crontab.rb
+++ b/lib/puppet/provider/cron/crontab.rb
@@ -114,10 +114,11 @@ Puppet::Type.type(:cron).provide(:crontab,
                     break
                 end
 
-                # FIXME It'd be great if I could somehow reuse how the
-                # fields are turned into text, but....
+                # Yay differing definitions of absent.
                 next if (hash[field] == :absent and obj.value(field) == "*")
-                next if (hash[field].join(",") == obj.value(field))
+
+                # Everything should be in the form of arrays, not the normal text.
+                next if (hash[field] == obj.value(field))
                 Puppet.info "Did not match %s: %s vs %s" %
                     [field, obj.value(field).inspect, hash[field].inspect]
                 matched = false 
diff --git a/lib/puppet/provider/package/apt.rb b/lib/puppet/provider/package/apt.rb
index d19276cc1..034a406f1 100755
--- a/lib/puppet/provider/package/apt.rb
+++ b/lib/puppet/provider/package/apt.rb
@@ -69,7 +69,7 @@ Puppet::Type.type(:package).provide :apt, :parent => :dpkg do
             end
         end
 
-        cmd << 'install' << str
+        cmd << :install << str
         
         aptget(*cmd)
     end
diff --git a/lib/puppet/provider/package/aptitude.rb b/lib/puppet/provider/package/aptitude.rb
index 240c3fd82..c4f8023d2 100755
--- a/lib/puppet/provider/package/aptitude.rb
+++ b/lib/puppet/provider/package/aptitude.rb
@@ -15,7 +15,7 @@ Puppet::Type.type(:package).provide :aptitude, :parent => :apt do
         output = aptitude(*args)
 
         # Yay, stupid aptitude doesn't throw an error when the package is missing.
-        if args.include?(:install) and output =~ /0 newly installed/
+        if args.include?(:install) and output =~ /Couldn't find any package/
             raise Puppet::Error.new(
                 "Could not find package %s" % self.name
             )
@@ -24,7 +24,7 @@ Puppet::Type.type(:package).provide :aptitude, :parent => :apt do
 
     def purge
         aptitude '-y', 'purge', @model[:name]
-	 end
+	end
 end
 
 # $Id$
diff --git a/test/network/client/master.rb b/test/network/client/master.rb
index 221c2409f..c9f785580 100755
--- a/test/network/client/master.rb
+++ b/test/network/client/master.rb
@@ -595,8 +595,10 @@ end
         facts = master.class.facts
         assert_equal("two", Facter.value(:testfact), "fact did not change")
 
-        assert(master.facts_changed?(facts), "master does not think facts changed")
-        assert(! master.fresh?(facts), "master is considered fresh after facts changed")
+        assert(master.send(:facts_changed?, facts),
+            "master does not think facts changed")
+        assert(! master.fresh?(facts),
+            "master is considered fresh after facts changed")
 
         assert_nothing_raised("Could not recompile when facts changed") do
             master.getconfig
diff --git a/test/network/handler/master.rb b/test/network/handler/master.rb
index eb3db0959..7e1a32396 100755
--- a/test/network/handler/master.rb
+++ b/test/network/handler/master.rb
@@ -97,6 +97,10 @@ class TestMaster < Test::Unit::TestCase
         Puppet[:filetimeout] = 15
         manifest = mktestmanifest()
 
+        facts = Puppet::Network::Client.master.facts
+        # Store them, so we don't determine frshness based on facts.
+        Puppet::Util::Storage.cache(:configuration)[:facts] = facts
+
         file2 = @createdfile + "2"
         @@tmpfiles << file2
 
@@ -117,7 +121,8 @@ class TestMaster < Test::Unit::TestCase
 
         assert(client, "did not create master client")
         # The client doesn't have a config, so it can't be up to date
-        assert(! client.fresh?, "Client is incorrectly up to date")
+        assert(! client.fresh?(facts),
+            "Client is incorrectly up to date")
 
         Puppet.config.use(:puppet)
         assert_nothing_raised {
@@ -126,7 +131,7 @@ class TestMaster < Test::Unit::TestCase
         }
 
         # Now it should be up to date
-        assert(client.fresh?, "Client is not up to date")
+        assert(client.fresh?(facts), "Client is not up to date")
 
         # Cache this value for later
         parse1 = master.freshness
@@ -145,7 +150,7 @@ class TestMaster < Test::Unit::TestCase
         # Verify that the master doesn't immediately reparse the file; we
         # want to wait through the timeout
         assert_equal(parse1, master.freshness, "Master did not wait through timeout")
-        assert(client.fresh?, "Client is not up to date")
+        assert(client.fresh?(facts), "Client is not up to date")
 
         # Then eliminate it
         Puppet[:filetimeout] = 0
@@ -153,14 +158,14 @@ class TestMaster < Test::Unit::TestCase
         # Now make sure the master does reparse
         #Puppet.notice "%s vs %s" % [parse1, master.freshness]
         assert(parse1 != master.freshness, "Master did not reparse file")
-        assert(! client.fresh?, "Client is incorrectly up to date")
+        assert(! client.fresh?(facts), "Client is incorrectly up to date")
 
         # Retrieve and apply the new config
         assert_nothing_raised {
             client.getconfig
             client.apply
         }
-        assert(client.fresh?, "Client is not up to date")
+        assert(client.fresh?(facts), "Client is not up to date")
 
         assert(FileTest.exists?(file2), "Second file %s does not exist" % file2)
     end
diff --git a/test/network/handler/runner.rb b/test/network/handler/runner.rb
index 5c3acde2b..b6df9bab9 100755
--- a/test/network/handler/runner.rb
+++ b/test/network/handler/runner.rb
@@ -36,10 +36,13 @@ class TestHandlerRunner < Test::Unit::TestCase
         # Okay, make our manifest
         file = tempfile()
         created = tempfile()
+        # We specify the schedule here, because I was having problems with
+        # using default schedules.
         File.open(file, "w") do |f|
             f.puts %{
                 class yayness {
-                    file { "#{created}": ensure => file, schedule => weekly }
+                    schedule { "yayness": period => weekly }
+                    file { "#{created}": ensure => file, schedule => yayness }
                 }
 
                 include yayness
diff --git a/test/ral/manager/attributes.rb b/test/ral/manager/attributes.rb
index f27b0513a..43d64f367 100755
--- a/test/ral/manager/attributes.rb
+++ b/test/ral/manager/attributes.rb
@@ -11,14 +11,9 @@ class TestTypeAttributes < Test::Unit::TestCase
     include PuppetTest
 
     def mktype
-        Puppet::Type.newtype(:faketype) {}
-    end
-
-    def teardown
-        super
-        if Puppet::Type.type(:faketype)
-            Puppet::Type.rmtype(:faketype)
-        end
+        type = Puppet::Type.newtype(:faketype) {}
+        cleanup { Puppet::Type.rmtype(:faketype) }
+        type
     end
 
     def test_bracket_methods
@@ -227,6 +222,28 @@ class TestTypeAttributes < Test::Unit::TestCase
             assert_equal(val, inst.value(old), "Incorrect orig value for %s" % old)
         end
     end
+
+    # Make sure eachattr is called in the parameter order.
+    def test_eachattr
+        type = mktype
+        name = type.newparam(:name) {}
+        one = type.newparam(:one) {}
+        two = type.newproperty(:two) {}
+        type.provide(:testing) {}
+        provider = type.attrclass(:provider)
+        should = [[name, :param], [provider, :param], [two, :property], [one, :param]]
+
+        assert_nothing_raised do
+            result = nil
+            type.eachattr do |obj, name|
+                result = [obj, name]
+                shouldary = should.shift
+                assert_equal(shouldary, result, "Did not get correct parameter")
+                break if should.empty?
+            end
+        end
+        assert(should.empty?, "Did not get all of the parameters.")
+    end
 end
 
 # $Id$
diff --git a/test/ral/types/cron.rb b/test/ral/types/cron.rb
index ce9631d97..941f8d072 100755
--- a/test/ral/types/cron.rb
+++ b/test/ral/types/cron.rb
@@ -426,12 +426,16 @@ class TestCron < Test::Unit::TestCase
         crons = []
         users = ["root", nonrootuser.name]
         users.each do |user|
-            crons << Puppet::Type.type(:cron).create(
+            cron = Puppet::Type.type(:cron).create(
                 :name => "testcron-#{user}",
                 :user => user,
                 :command => "/bin/echo",
                 :minute => [0,30]
             )
+            crons << cron
+
+            assert_equal(cron.should(:user), cron.should(:target),
+                "Target was not set correctly for %s" % user)
         end
         provider = crons[0].provider.class
 
diff --git a/test/ral/types/file.rb b/test/ral/types/file.rb
index caf01fecd..9f7861787 100755
--- a/test/ral/types/file.rb
+++ b/test/ral/types/file.rb
@@ -1131,9 +1131,10 @@ class TestFile < Test::Unit::TestCase
         obj[:content] = "New content"
         assert_apply(obj)
 
-        bucketedpath = File.join(bpath, "18cc17fa3047fcc691fdf49c0a7f539a", "contents")
+        md5 = "18cc17fa3047fcc691fdf49c0a7f539a"
+        dir, file, pathfile = Puppet::Network::Handler.filebucket.paths(bpath, md5)
 
-        assert_equal(0440, filemode(bucketedpath))
+        assert_equal(0440, filemode(file))
     end
 
     def test_largefilechanges
diff --git a/test/ral/types/filebucket.rb b/test/ral/types/filebucket.rb
index 32b8a0bd5..5b0093425 100755
--- a/test/ral/types/filebucket.rb
+++ b/test/ral/types/filebucket.rb
@@ -82,7 +82,9 @@ class TestFileBucket < Test::Unit::TestCase
 
         assert(md5)
 
-        assert(FileTest.directory?(File.join(bucketpath, md5)),
+        dir, file, pathfile = Puppet::Network::Handler.filebucket.paths(bucketpath, md5)
+
+        assert(FileTest.directory?(dir),
             "MD5 directory does not exist")
 
         newmd5 = nil