Finishing the work to use Puppet::Resource instead of TransObject

This was a complicated project because TransObject had made its
way into too many classes.  The usage by Util::Settings
was particularly nefarious.  Refactoring and fixing this
exposed some other issues.

The main complication, though, was the extent to which the
Puppet::Type class depended on TransObject.

Signed-off-by: Luke Kanies <luke@madstop.com>

7 files changed, 90 insertions, 105 deletions

diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 1a47b05b0..be9433d50 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -364,9 +364,9 @@ module Puppet
             :desc => "Where FileBucket files are stored."
         },
         :ca => [true, "Wether the master should function as a certificate authority."],
-        :modulepath => [ "$confdir/modules:/usr/share/puppet/modules",
-           "The search path for modules as a colon-separated list of
-            directories." ],
+        :modulepath => {:default => "$confdir/modules:/usr/share/puppet/modules",
+           :desc => "The search path for modules as a colon-separated list of
+            directories.", :type => :element }, # We don't want this to be considered a file, since it's multiple files.
         :ssl_client_header => ["HTTP_X_CLIENT_DN", "The header containing an authenticated
             client's SSL DN.  Only used with Mongrel.  This header must be set by the proxy
             to the authenticated client's SSL DN (e.g., ``/CN=puppet.reductivelabs.com``).
@@ -501,11 +501,12 @@ module Puppet
 
     # Plugin information.
     self.setdefaults(:main,
-        :pluginpath => ["$vardir/plugins",
-            "Where Puppet should look for plugins.  Multiple directories should
+        :pluginpath => {:default => "$vardir/plugins/",
+            :desc => "Where Puppet should look for plugins.  Multiple directories should
             be colon-separated, like normal PATH variables.  As of 0.23.1, this
             option is deprecated; download your custom libraries to the $libdir
-            instead."],
+            instead.",
+            :type => :element}, # Don't consider this a file, since it's a colon-separated list.
         :plugindest => ["$libdir",
             "Where Puppet should store plugins that it pulls down from the central
             server."],
@@ -521,15 +522,16 @@ module Puppet
 
     # Central fact information.
     self.setdefaults(:main,
-        :factpath => {:default => "$vardir/facts",
+        :factpath => {:default => "$vardir/facts/",
             :desc => "Where Puppet should look for facts.  Multiple directories should
                 be colon-separated, like normal PATH variables.",
             :call_on_define => true, # Call our hook with the default value, so we always get the value added to facter.
+            :type => :element, # Don't consider it a file, because it could be multiple colon-separated files
             :hook => proc { |value| Facter.search(value) if Facter.respond_to?(:search) }},
-        :factdest => ["$vardir/facts",
+        :factdest => ["$vardir/facts/",
             "Where Puppet should store facts that it pulls down from the central
             server."],
-        :factsource => ["puppet://$server/facts",
+        :factsource => ["puppet://$server/facts/",
             "From where to retrieve facts.  The standard Puppet ``file`` type
              is used for retrieval, so anything that is a valid file source can
              be used here."],
diff --git a/lib/puppet/module.rb b/lib/puppet/module.rb
index 7bf35ac18..ab1bc75bd 100644
--- a/lib/puppet/module.rb
+++ b/lib/puppet/module.rb
@@ -12,9 +12,14 @@ class Puppet::Module
         dirs = Puppet.settings.value(:modulepath, environment).split(":")
         if ENV["PUPPETLIB"]
             dirs = ENV["PUPPETLIB"].split(":") + dirs
-        else
         end
-        dirs.select do |p|
+        dirs.collect do |dir|
+            if dir !~ /^#{File::SEPARATOR}/
+                File.join(Dir.getwd, dir)
+            else
+                dir
+            end
+        end.select do |p|
             p =~ /^#{File::SEPARATOR}/ && File::directory?(p)
         end
     end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 5739b369e..56788a6cf 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -96,10 +96,6 @@ class Type
                 when @validproperties.include?(attr): :property
                 when @paramhash.include?(attr): :param
                 when @@metaparamhash.include?(attr): :meta
-                else
-                    raise Puppet::DevError,
-                        "Invalid attribute '%s' for class '%s'" %
-                        [attr, self.name]
                 end
         end
 
@@ -123,30 +119,6 @@ class Type
         end
     end
 
-    # 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)
-        if ary.empty?
-            ary = nil
-        end
-
-        # 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
         @@metaparams.each { |p| yield p.name }
     end
@@ -501,13 +473,6 @@ class Type
         }
     end
 
-    # If we've got a catalog, then use it to expire our data;
-    # otherwise, null-op.
-    def expire
-        return nil unless expirer
-        super
-    end
-
     # Let the catalog determine whether a given cached value is
     # still valid or has expired.
     def expirer
@@ -589,24 +554,18 @@ class Type
     # For any parameters or properties that have defaults and have not yet been
     # 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|
-            # not many attributes will have defaults defined, so we short-circuit
-            # those away
-            next unless klass.method_defined?(:default)
-            next if @parameters[klass.name]
-
-            next unless obj = self.newattr(klass)
-
-            # We have to check for nil values, not "truth", so we allow defaults
-            # to false.
-            value = obj.default and ! value.nil?
-            if ! value.nil?
-                obj.value = value
-            else
-                @parameters.delete(obj.name)
-            end
-        }
+    def set_default(attr)
+        return unless klass = self.class.attrclass(attr)
+        return unless klass.method_defined?(:default)
+        return if @parameters.include?(klass.name)
+
+        return unless parameter = newattr(klass.name)
+
+        if value = parameter.default and ! value.nil?
+            parameter.value = value
+        else
+            @parameters.delete(parameter.name)
+        end
     end
 
     # Convert our object to a hash.  This just includes properties.
@@ -658,7 +617,7 @@ class Type
 
     ###############################
     # Code related to the closure-like behaviour of the resource classes.
-    attr_writer :implicit
+    attr_accessor :implicit
 
     # Is this type's name isomorphic with the object?  That is, if the
     # name conflicts, does it necessarily mean that the objects conflict?
@@ -1075,14 +1034,14 @@ class Type
     #   This should only be used directly from Ruby -- it's not used when going through
     # normal Puppet usage.
     def self.hash2resource(hash)
-        hash = hash.inject({}) { |result, ary| result[ary[0].to_sym] = ary[1]; hash }
+        hash = hash.inject({}) { |result, ary| result[ary[0].to_sym] = ary[1]; result }
 
         if title = hash[:title]
             hash.delete(:title)
         else
             if self.namevar != :name
                 if hash.include?(:name) and hash.include?(self.namevar)
-                    raise ArgumentError, "Cannot provide both name and %s to resources of type %s" % [self.namevar, self.name]
+                    raise Puppet::Error, "Cannot provide both name and %s to resources of type %s" % [self.namevar, self.name]
                 end
                 if title = hash[self.namevar]
                     hash.delete(self.namevar)
@@ -1094,13 +1053,16 @@ class Type
             end
         end
 
-        if catalog = hash[:catalog]
-            hash.delete(:catalog)
-        end
 
         # Now create our resource.
         resource = Puppet::Resource.new(self.name, title)
-        resource.catalog = catalog if catalog
+        [:catalog, :implicit].each do |attribute|
+            if value = hash[attribute]
+                hash.delete(attribute)
+                resource.send(attribute.to_s + "=", value)
+            end
+        end
+
         hash.each do |param, value|
             resource[param] = value
         end
@@ -1950,15 +1912,12 @@ class Type
 
     public
 
+    attr_reader :original_parameters
+
     # initialize the type instance
     def initialize(resource)
-        if resource.is_a?(Puppet::TransObject)
-            raise Puppet::DevError, "Got TransObject instead of Resource or hash"
-        end
-
-        unless resource.is_a?(Puppet::Resource)
-            resource = self.class.hash2resource(resource)
-        end
+        raise Puppet::DevError, "Got TransObject instead of Resource or hash" if resource.is_a?(Puppet::TransObject)
+        resource = self.class.hash2resource(resource) unless resource.is_a?(Puppet::Resource)
 
         # The list of parameter/property instances.
         @parameters = {}
@@ -1971,7 +1930,7 @@ class Type
             self.title = resource.ref
         end
 
-        [:file, :line, :catalog].each do |getter|
+        [:file, :line, :catalog, :implicit].each do |getter|
             setter = getter.to_s + "="
             if val = resource.send(getter)
                 self.send(setter, val)
@@ -1980,22 +1939,32 @@ class Type
 
         @tags = resource.tags
 
-        # If they've provided a title but no name, then set the name now.
-        unless name = resource[:name] || resource[self.class.namevar]
-            self[:name] = resource.title
-        end
+        @original_parameters = resource.to_hash
 
-        found = []
-        (self.class.allattrs + resource.keys).uniq.each do |attr|
+        set_default(:provider)
 
+        set_parameters(@original_parameters)
+
+        self.validate if self.respond_to?(:validate)
+    end
+
+    private
+
+    # Set all of the parameters from a hash, in the appropriate order.
+    def set_parameters(hash)
+        # Use the order provided by allattrs, but add in any
+        # extra attributes from the resource so we get failures
+        # on invalid attributes.
+        no_values = []
+        (self.class.allattrs + hash.keys).uniq.each do |attr|
             begin
                 # Set any defaults immediately.  This is mostly done so
                 # that the default provider is available for any other
                 # property validation.
-                if resource.has_key?(attr)
-                    self[attr] = resource[attr]
+                if hash.has_key?(attr)
+                    self[attr] = hash[attr]
                 else
-                    setdefaults(attr)
+                    no_values << attr
                 end
             rescue ArgumentError, Puppet::Error, TypeError
                 raise
@@ -2005,10 +1974,13 @@ class Type
                 raise error
             end
         end
-
-        self.validate if self.respond_to?(:validate)
+        no_values.each do |attr|
+            set_default(attr)
+        end
     end
 
+    public
+
     # Set up all of our autorequires.
     def finish
         # Scheduling has to be done when the whole config is instantiated, so
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 7c9afb4ff..169dc577a 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -448,9 +448,6 @@ module Puppet
         end
         
         def initialize(hash)
-            # Store a copy of the arguments for later.
-            @original_arguments = hash.to_hash
-
             # Used for caching clients
             @clients = {}
 
@@ -478,7 +475,7 @@ module Puppet
             # or so.  Unfortunately, we don't have a straightforward way to manage
             # the different lifetimes of this data, so we kludge it like this.
             # The right-side hash wins in the merge.
-            options = @original_arguments.merge(:path => full_path, :implicit => true).reject { |param, value| value.nil? }
+            options = @original_parameters.merge(:path => full_path, :implicit => true).reject { |param, value| value.nil? }
 
             # These should never be passed to our children.
             [:parent, :ensure, :recurse, :target].each do |param|
@@ -683,6 +680,7 @@ module Puppet
             else
                 self.fail "Could not back up files of type %s" % s.ftype
             end
+            expire
         end
 
         # a wrapper method to make sure the file exists before doing anything
diff --git a/lib/puppet/type/file/ensure.rb b/lib/puppet/type/file/ensure.rb
index 026d13fae..ae7137264 100755
--- a/lib/puppet/type/file/ensure.rb
+++ b/lib/puppet/type/file/ensure.rb
@@ -163,6 +163,7 @@ module Puppet
         end
 
         def sync
+            expire
             @resource.remove_existing(self.should)
             if self.should == :absent
                 return :file_removed
diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb
index f0425d2bb..9280e4a2b 100755
--- a/lib/puppet/type/tidy.rb
+++ b/lib/puppet/type/tidy.rb
@@ -255,9 +255,9 @@ Puppet::Type.newtype(:tidy) do
             dir = File.dirname(path)
             next unless resource = files_by_name[dir]
             if resource[:require] 
-                resource[:require] << [:file, path]
+                resource[:require] << Puppet::Resource::Reference.new(:file, path)
             else
-                resource[:require] = [[:file, path]]
+                resource[:require] = [Puppet::Resource::Reference.new(:file, path)]
             end
         end
 
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index 184530cc7..9cc05d891 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -378,15 +378,22 @@ class Puppet::Util::Settings
         if hash[:section]
             hash[:section] = symbolize(hash[:section])
         end
-        case hash[:default]
-        when true, false, "true", "false":
-            klass = CBoolean
-        when /^\$\w+\//, /^\//:
-            klass = CFile
-        when String, Integer, Float: # nothing
-            klass = CElement
+        if type = hash[:type]
+            unless klass = {:element => CElement, :file => CFile, :boolean => CBoolean}[type]
+                raise ArgumentError, "Invalid setting type '%s'" % type
+            end
+            hash.delete(:type)
         else
-            raise Puppet::Error, "Invalid value '%s' for %s" % [value.inspect, hash[:name]]
+            case hash[:default]
+            when true, false, "true", "false":
+                klass = CBoolean
+            when /^\$\w+\//, /^\//:
+                klass = CFile
+            when String, Integer, Float: # nothing
+                klass = CElement
+            else
+                raise Puppet::Error, "Invalid value '%s' for %s" % [value.inspect, hash[:name]]
+            end
         end
         hash[:settings] = self
         element = klass.new(hash)
@@ -512,8 +519,8 @@ class Puppet::Util::Settings
 
         @config.values.find_all { |value| value.is_a?(CFile) }.each do |file|
             next unless (sections.nil? or sections.include?(file.section))
-            next if catalog.resource(:file, value(file.name))
             next unless resource = file.to_resource
+            next if catalog.resource(resource.ref)
 
             catalog.add_resource(resource)
         end