Fixing cron support (I hope). It now uses providers, and seems to work, at least on my os x box.

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

9 files changed, 518 insertions, 446 deletions

diff --git a/lib/puppet/metatype/attributes.rb b/lib/puppet/metatype/attributes.rb
index ad964b7ed..a96533964 100644
--- a/lib/puppet/metatype/attributes.rb
+++ b/lib/puppet/metatype/attributes.rb
@@ -22,8 +22,11 @@ class Puppet::Type
         namevar = self.namevar
 
         order = [namevar]
+        if self.parameters.include?(:provider)
+            order << :provider
+        end
         order << [self.properties.collect { |property| property.name },
-            self.parameters,
+            self.parameters - [:provider],
             self.metaparams].flatten.reject { |param|
                 # we don't want our namevar in there multiple times
                 param == namevar
@@ -34,6 +37,19 @@ class Puppet::Type
         return order
     end
 
+    # Retrieve an attribute alias, if there is one.
+    def self.attr_alias(param)
+        @attr_aliases[symbolize(param)]
+    end
+
+    # Create an alias to an existing attribute.  This will cause the aliased
+    # attribute to be valid when setting and retrieving values on the instance.
+    def self.set_attr_alias(hash)
+        hash.each do |new, old|
+            @attr_aliases[symbolize(new)] = symbolize(old)
+        end
+    end
+
     # Find the class associated with any given attribute.
     def self.attrclass(name)
         @attrclasses ||= {}
@@ -409,6 +425,16 @@ class Puppet::Type
 
         return hash
     end
+
+    # Return either the attribute alias or the attribute.
+    def attr_alias(name)
+        name = symbolize(name)
+        if synonym = self.class.attr_alias(name)
+            return synonym
+        else
+            return name
+        end
+    end
     
     # Are we deleting this resource?
     def deleting?
@@ -420,9 +446,7 @@ class Puppet::Type
     # Most classes won't use this.
     def is=(ary)
         param, value = ary
-        if param.is_a?(String)
-            param = param.intern
-        end
+        param = attr_alias(param)
         if self.class.validproperty?(param)
             unless prop = @parameters[param]
                 prop = self.newattr(param)
@@ -439,18 +463,16 @@ class Puppet::Type
     # value, but you can also specifically return 'is' and 'should'
     # values using 'object.is(:property)' or 'object.should(:property)'.
     def [](name)
-        if name.is_a?(String)
-            name = name.intern
+        name = attr_alias(name)
+
+        unless self.class.validattr?(name)
+            raise TypeError.new("Invalid parameter %s(%s)" % [name, name.inspect])
         end
 
         if name == :name
             name = self.class.namevar
         end
 
-        unless self.class.validattr?(name)
-            raise TypeError.new("Invalid parameter %s(%s)" % [name, name.inspect])
-        end
-
         if obj = @parameters[name]
             if obj.is_a?(Puppet::Type::Property)
                 return obj.is
@@ -466,10 +488,11 @@ class Puppet::Type
     # access to always be symbols, not strings.  This sets the 'should'
     # value on properties, and otherwise just sets the appropriate parameter.
     def []=(name,value)
+        name = attr_alias(name)
+
         unless self.class.validattr?(name)
             raise TypeError.new("Invalid parameter %s" % [name])
         end
-        name = symbolize(name)
 
         if name == :name
             name = self.class.namevar
@@ -516,7 +539,8 @@ class Puppet::Type
 
     # retrieve the 'is' value for a specified property
     def is(name)
-        if prop = @parameters[symbolize(name)] and prop.is_a?(Puppet::Type::Property)
+        name = attr_alias(name)
+        if prop = @parameters[name] and prop.is_a?(Puppet::Type::Property)
             return prop.is
         else
             return nil
@@ -525,7 +549,8 @@ class Puppet::Type
 
     # retrieve the 'should' value for a specified property
     def should(name)
-        if prop = @parameters[symbolize(name)] and prop.is_a?(Puppet::Type::Property)
+        name = attr_alias(name)
+        if prop = @parameters[name] and prop.is_a?(Puppet::Type::Property)
             return prop.should
         else
             return nil
@@ -634,7 +659,8 @@ class Puppet::Type
 
     # Return a specific value for an attribute.
     def value(name)
-        name = symbolize(name)
+        name = attr_alias(name)
+
         if obj = @parameters[name] and obj.respond_to?(:value)
             return obj.value
         else
diff --git a/lib/puppet/provider/cron/crontab.rb b/lib/puppet/provider/cron/crontab.rb
index c50bc2680..b7caa4a21 100755
--- a/lib/puppet/provider/cron/crontab.rb
+++ b/lib/puppet/provider/cron/crontab.rb
@@ -6,49 +6,130 @@ tab = case Facter.value(:operatingsystem)
         :crontab
     end
 
+
 Puppet::Type.type(:cron).provide(:crontab,
     :parent => Puppet::Provider::ParsedFile,
+    :default_target => ENV["USER"],
     :filetype => tab
 ) do
     commands :crontab => "crontab"
 
-    attr_accessor :target
-
     text_line :comment, :match => %r{^#}, :post_parse => proc { |hash|
         if hash[:line] =~ /Puppet Name: (.+)\s*$/
             hash[:name] = $1
         end
     }
 
-    text_line :blank, :match => /^\s+/
+    text_line :blank, :match => %r{^\s+}
+
+    text_line :environment, :match => %r{^\w+=}
 
-    text_line :environment, :match => %r{/^\w+=/}
+    record_line :freebsd_special, :fields => %w{special command},
+        :match => %r{^@(\w+)\s+(.+)$}, :pre_gen => proc { |hash|
+            hash[:special] = "@" + hash[:special]
+        }
 
-    record_line :crontab, :fields => %w{minute hour weekday month monthday command},
+    crontab = record_line :crontab, :fields => %w{minute hour monthday month weekday command},
         :optional => %w{minute hour weekday month monthday}, :absent => "*"
 
-    #record_line :freebsd_special, :fields => %w{special command},
-    #    :match => %r{^@(\w+)\s+(.+)}
-    # Apparently Freebsd will "helpfully" add a new TZ line to every
-    # single cron line, but not in all cases (e.g., it doesn't do it
-    # on my machine.  This is my attempt to fix it so the TZ lines don't
-    # multiply.
-    #if tab =~ /^TZ=.+$/
-    #    return tab.sub(/\n/, "\n" + self.header)
-    #else
-    #    return self.header() + tab
-    #end
+    class << crontab
+        def numeric_fields
+            fields - [:command]
+        end
+        # Do some post-processing of the parsed record.  Basically just
+        # split the numeric fields on ','.
+        def post_parse(details)
+            numeric_fields.each do |field|
+                if val = details[field] and val != :absent
+                    details[field] = details[field].split(",")
+                end
+            end
+        end
+
+        # Join the fields back up based on ','.
+        def pre_gen(details)
+            numeric_fields.each do |field|
+                if vals = details[field] and vals.is_a?(Array)
+                    details[field] = vals.join(",")
+                end
+            end
+        end
+
+
+        # Add name and environments as necessary.
+        def to_line(details)
+            str = ""
+            if details[:name]
+                str = "# Puppet Name: %s\n" % details[:name]
+            end
+            if details[:environment] and details[:environment] != :absent
+                details[:environment].each do |env|
+                    str += env + "\n"
+                end
+            end
+
+            str += join(details)
+            str
+        end
+    end
 
 
     # Return the header placed at the top of each generated file, warning
     # users that modifying this file manually is probably a bad idea.
     def self.header
-%{# HEADER This file was autogenerated at #{Time.now} by puppet.  While it
-# HEADER can still be managed manually, it is definitely not recommended.
+%{# HEADER This file was autogenerated at #{Time.now} by puppet.
+# HEADER While it can still be managed manually, it is definitely notrecommended.
 # HEADER Note particularly that the comments starting with 'Puppet Name' should
 # HEADER not be deleted, as doing so could cause duplicate cron jobs.\n}
     end
 
+    # See if we can match the hash against an existing cron job.
+    def self.match(hash)
+        model.find_all { |obj|
+            obj.value(:user) == hash[:user] and obj.value(:command) == hash[:command]
+        }.each do |obj|
+            # we now have a cron job whose command exactly matches
+            # let's see if the other fields match
+
+            # First check the @special stuff
+            if hash[:special]
+                next unless obj.value(:special) == hash[:special]
+            end
+
+            # Then the normal fields.
+            matched = true
+            record_type(hash[:record_type]).fields().each do |field|
+                next if field == :command
+                if hash[field] and ! obj.value(field)
+                    Puppet.info "Cron is missing %s: %s and %s" %
+                        [field, hash[field].inspect, obj.value(field).inspect]
+                    matched = false
+                    break
+                end
+
+                if ! hash[field] and obj.value(field)
+                    Puppet.info "Hash is missing %s: %s and %s" %
+                        [field, obj.value(field).inspect, hash[field].inspect]
+                    matched = false
+                    break
+                end
+
+                # FIXME It'd be great if I could somehow reuse how the
+                # fields are turned into text, but....
+                next if (hash[field] == :absent and obj.value(field) == "*")
+                next if (hash[field].join(",") == obj.value(field))
+                Puppet.info "Did not match %s: %s vs %s" %
+                    [field, obj.value(field).inspect, hash[field].inspect]
+                matched = false 
+                break
+            end
+            next unless matched
+            return obj
+        end
+
+        return false
+    end
+
     # Collapse name and env records.
     def self.prefetch_hook(records)
         name = nil
@@ -78,12 +159,27 @@ Puppet::Type.type(:cron).provide(:crontab,
         }.reject { |record| record[:skip] }
     end
 
+    def self.to_file(records)
+        text = super
+        # Apparently Freebsd will "helpfully" add a new TZ line to every
+        # single cron line, but not in all cases (e.g., it doesn't do it
+        # on my machine).  This is my attempt to fix it so the TZ lines don't
+        # multiply.
+        if text =~ /(^TZ=.+\n)/
+            tz = $1 
+            text.sub!(tz, '')
+            text = tz + text
+        end
+        return text
+    end
+
     def user=(user)
-        self.target = target
+        @property_hash[:user] = user
+        @property_hash[:target] = user
     end
 
     def user
-        self.target
+        @property_hash[:user] || @property_hash[:target]
     end
 end
 
diff --git a/lib/puppet/provider/parsedfile.rb b/lib/puppet/provider/parsedfile.rb
index 56474e273..bb6546095 100755
--- a/lib/puppet/provider/parsedfile.rb
+++ b/lib/puppet/provider/parsedfile.rb
@@ -80,9 +80,10 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
 
     # Flush all of the records relating to a specific target.
     def self.flush_target(target)
-        target_object(target).write(to_file(target_records(target).reject { |r|
+        records = target_records(target).reject { |r|
             r[:ensure] == :absent
-        }))
+        }
+        target_object(target).write(to_file(records))
     end
 
     # Return the header placed at the top of each generated file, warning
@@ -101,7 +102,7 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         @target = nil
 
         # Default to flat files
-        @filetype = Puppet::Util::FileType.filetype(:flat)
+        @filetype ||= Puppet::Util::FileType.filetype(:flat)
         super
     end
 
@@ -191,7 +192,7 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         end
 
         if respond_to?(:prefetch_hook)
-            prefetch_hook(target_records)
+            target_records = prefetch_hook(target_records)
         end
 
         @records += target_records
@@ -202,8 +203,8 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
             if instance = self.model[record[:name]]
                 next unless instance.provider.is_a?(self)
                 instance.provider.property_hash = record
-            elsif self.respond_to?(:match)
-                if instance = self.match(record)
+            elsif respond_to?(:match)
+                if instance = match(record)
                     record[:name] = instance[:name]
                     instance.provider.property_hash = record
                 end
@@ -269,6 +270,12 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         targets.uniq.reject { |t| t.nil? }
     end
 
+    def self.to_file(records)
+        text = super
+        header + text
+    end
+
+
     def create
         @model.class.validproperties.each do |property|
             if value = @model.should(property)
diff --git a/lib/puppet/provider/service/smf.rb b/lib/puppet/provider/service/smf.rb
index 08f2bd731..e2e5c989a 100755
--- a/lib/puppet/provider/service/smf.rb
+++ b/lib/puppet/provider/service/smf.rb
@@ -54,6 +54,7 @@ Puppet::Type.type(:service).provide :smf, :parent => :base do
                 value = $2
             else
                 Puppet.err "Could not match %s" % line.inspect
+                next
             end
             case var
             when "state":
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 023f85ab3..bf2060018 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -73,6 +73,8 @@ class Type < Puppet::Element
         @parameters = []
         @paramhash = {}
 
+        @attr_aliases = {}
+
         @paramdoc = Hash.new { |hash,key|
           if key.is_a?(String)
             key = key.intern
diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index d71654586..dd9ed3ab5 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -4,453 +4,378 @@ require 'puppet/type/property'
 require 'puppet/util/filetype'
 require 'puppet/type/parsedtype'
 
-module Puppet
-    # Model the actual cron jobs.  Supports all of the normal cron job fields
-    # as parameters, with the 'command' as the single property.  Also requires a
-    # completely symbolic 'name' paremeter, which gets written to the file
-    # and is used to manage the job.
-    newtype(:cron) do
-
-        # A base class for all of the Cron parameters, since they all have
-        # similar argument checking going on.  We're stealing the base class
-        # from parsedtype, and we should probably subclass Cron from there,
-        # but it was just too annoying to do.
-        class CronParam < Puppet::Property
-            class << self
-                attr_accessor :boundaries, :default
-            end
-
-            # We have to override the parent method, because we consume the entire
-            # "should" array
-            def insync?
-                if defined? @should and @should
-                    self.is_to_s == self.should_to_s
-                else
-                    true
-                end
-            end
+Puppet::Type.newtype(:cron) do
+    @doc = "Installs and manages cron jobs.  All fields except the command 
+        and the user are optional, although specifying no periodic
+        fields would result in the command being executed every
+        minute.  While the name of the cron job is not part of the actual
+        job, it is used by Puppet to store and retrieve it.
+        
+        If you specify a cron job that matches an existing job in every way
+        except name, then the jobs will be considered equivalent and the
+        new name will be permanently associated with that job.  Once this
+        association is made and synced to disk, you can then manage the job
+        normally (e.g., change the schedule of the job).
+        
+        Example:
+            
+            cron { logrotate:
+                command => \"/usr/sbin/logrotate\",
+                user => root,
+                hour => 2,
+                minute => 0
+            }
+        "
+    ensurable
+
+    # A base class for all of the Cron parameters, since they all have
+    # similar argument checking going on.  We're stealing the base class
+    # from parsedtype, and we should probably subclass Cron from there,
+    # but it was just too annoying to do.
+    class CronParam < Puppet::Property
+        class << self
+            attr_accessor :boundaries, :default
+        end
 
-            # A method used to do parameter input handling.  Converts integers
-            # in string form to actual integers, and returns the value if it's
-            # an integer or false if it's just a normal string.
-            def numfix(num)
-                if num =~ /^\d+$/
-                    return num.to_i
-                elsif num.is_a?(Integer)
-                    return num
-                else
-                    return false
-                end
+        # We have to override the parent method, because we consume the entire
+        # "should" array
+        def insync?
+            if defined? @should and @should
+                self.is_to_s == self.should_to_s
+            else
+                true
             end
+        end
 
-            # Verify that a number is within the specified limits.  Return the
-            # number if it is, or false if it is not.
-            def limitcheck(num, lower, upper)
-                if num >= lower and num <= upper
-                    return num
-                else
-                    return false
-                end
+        # A method used to do parameter input handling.  Converts integers
+        # in string form to actual integers, and returns the value if it's
+        # an integer or false if it's just a normal string.
+        def numfix(num)
+            if num =~ /^\d+$/
+                return num.to_i
+            elsif num.is_a?(Integer)
+                return num
+            else
+                return false
             end
+        end
 
-            # Verify that a value falls within the specified array.  Does case
-            # insensitive matching, and supports matching either the entire word
-            # or the first three letters of the word.
-            def alphacheck(value, ary)
-                tmp = value.downcase
-                
-                # If they specified a shortened version of the name, then see
-                # if we can lengthen it (e.g., mon => monday).
-                if tmp.length == 3
-                    ary.each_with_index { |name, index|
-                        if name =~ /#{tmp}/i
-                            return index
-                        end
-                    }
-                else
-                    if ary.include?(tmp)
-                        return ary.index(tmp)
-                    end
-                end
-
+        # Verify that a number is within the specified limits.  Return the
+        # number if it is, or false if it is not.
+        def limitcheck(num, lower, upper)
+            if num >= lower and num <= upper
+                return num
+            else
                 return false
             end
+        end
 
-            def should_to_s
-                if @should
-                    if self.name == :command or @should[0].is_a? Symbol
-                        @should[0]
-                    else
-                        @should.join(",")
+        # Verify that a value falls within the specified array.  Does case
+        # insensitive matching, and supports matching either the entire word
+        # or the first three letters of the word.
+        def alphacheck(value, ary)
+            tmp = value.downcase
+            
+            # If they specified a shortened version of the name, then see
+            # if we can lengthen it (e.g., mon => monday).
+            if tmp.length == 3
+                ary.each_with_index { |name, index|
+                    if name =~ /#{tmp}/i
+                        return index
                     end
-                else
-                    nil
+                }
+            else
+                if ary.include?(tmp)
+                    return ary.index(tmp)
                 end
             end
 
-            def is_to_s
-                if @is
-                    unless @is.is_a?(Array)
-                        return @is
-                    end
+            return false
+        end
 
-                    if self.name == :command or @is[0].is_a? Symbol
-                        @is[0]
-                    else
-                        @is.join(",")
-                    end
+        def should_to_s
+            if @should
+                if self.name == :command or @should[0].is_a? Symbol
+                    @should[0]
                 else
-                    nil
+                    @should.join(",")
                 end
+            else
+                nil
             end
+        end
 
-            # The method that does all of the actual parameter value
-            # checking; called by all of the +param<name>=+ methods.
-            # Requires the value, type, and bounds, and optionally supports
-            # a boolean of whether to do alpha checking, and if so requires
-            # the ary against which to do the checking.
-            munge do |value|
-                # Support 'absent' as a value, so that they can remove
-                # a value
-                if value == "absent" or value == :absent
-                    return :absent
-                end
-
-                # Allow the */2 syntax
-                if value =~ /^\*\/[0-9]+$/
-                    return value
-                end
-
-                # Allow ranges
-                if value =~ /^[0-9]+-[0-9]+$/
-                    return value
-                end
-
-                if value == "*"
-                    return value
-                end
-
-                return value unless self.class.boundaries
-                lower, upper = self.class.boundaries
-                retval = nil
-                if num = numfix(value)
-                    retval = limitcheck(num, lower, upper)
-                elsif respond_to?(:alpha)
-                    # If it has an alpha method defined, then we check
-                    # to see if our value is in that list and if so we turn
-                    # it into a number
-                    retval = alphacheck(value, alpha())
+        def is_to_s
+            if @is
+                unless @is.is_a?(Array)
+                    return @is
                 end
 
-                if retval
-                    return retval.to_s
+                if self.name == :command or @is[0].is_a? Symbol
+                    @is[0]
                 else
-                    self.fail "%s is not a valid %s" %
-                        [value, self.class.name]
+                    @is.join(",")
                 end
+            else
+                nil
             end
         end
 
-        # Somewhat uniquely, this property does not actually change anything -- it
-        # just calls +@parent.sync+, which writes out the whole cron tab for
-        # the user in question.  There is no real way to change individual cron
-        # jobs without rewriting the entire cron file.
-        #
-        # Note that this means that managing many cron jobs for a given user
-        # could currently result in multiple write sessions for that user.
-        newproperty(:command, :parent => CronParam) do
-            desc "The command to execute in the cron job.  The environment
-                provided to the command varies by local system rules, and it is
-                best to always provide a fully qualified command.  The user's
-                profile is not sourced when the command is run, so if the
-                user's environment is desired it should be sourced manually.
-                
-                All cron parameters support ``absent`` as a value; this will
-                remove any existing values for that field."
-
-            def should
-                if @should
-                    if @should.is_a? Array
-                        @should[0]
-                    else
-                        devfail "command is not an array"
-                    end
-                else
-                    nil
-                end
-            end
+        def should
+            @should
         end
 
-        newproperty(:special) do
-            desc "Special schedules only supported on FreeBSD."
-
-            def specials
-                %w{reboot yearly annually monthly weekly daily midnight hourly}
+        # The method that does all of the actual parameter value
+        # checking; called by all of the +param<name>=+ methods.
+        # Requires the value, type, and bounds, and optionally supports
+        # a boolean of whether to do alpha checking, and if so requires
+        # the ary against which to do the checking.
+        munge do |value|
+            # Support 'absent' as a value, so that they can remove
+            # a value
+            if value == "absent" or value == :absent
+                return :absent
             end
 
-            validate do |value|
-                unless specials().include?(value)
-                    raise ArgumentError, "Invalid special schedule %s" %
-                        value.inspect
-                end
+            # Allow the */2 syntax
+            if value =~ /^\*\/[0-9]+$/
+                return value
             end
-        end
 
-        newproperty(:minute, :parent => CronParam) do
-            self.boundaries = [0, 59]
-            desc "The minute at which to run the cron job.
-                Optional; if specified, must be between 0 and 59, inclusive."
-        end
-
-        newproperty(:hour, :parent => CronParam) do
-            self.boundaries = [0, 23]
-            desc "The hour at which to run the cron job. Optional;
-                if specified, must be between 0 and 23, inclusive."
-        end
+            # Allow ranges
+            if value =~ /^[0-9]+-[0-9]+$/
+                return value
+            end
 
-        newproperty(:weekday, :parent => CronParam) do
-            def alpha
-                %w{sunday monday tuesday wednesday thursday friday saturday}
+            if value == "*"
+                return value
             end
-            self.boundaries = [0, 6]
-            desc "The weekday on which to run the command.
-                Optional; if specified, must be between 0 and 6, inclusive, with
-                0 being Sunday, or must be the name of the day (e.g., Tuesday)."
-        end
 
-        newproperty(:month, :parent => CronParam) do
-            def alpha
-                %w{january february march april may june july
-                    august september october november december}
+            return value unless self.class.boundaries
+            lower, upper = self.class.boundaries
+            retval = nil
+            if num = numfix(value)
+                retval = limitcheck(num, lower, upper)
+            elsif respond_to?(:alpha)
+                # If it has an alpha method defined, then we check
+                # to see if our value is in that list and if so we turn
+                # it into a number
+                retval = alphacheck(value, alpha())
             end
-            self.boundaries = [1, 12]
-            desc "The month of the year.  Optional; if specified
-                must be between 1 and 12 or the month name (e.g., December)."
-        end
 
-        newproperty(:monthday, :parent => CronParam) do
-            self.boundaries = [1, 31]
-            desc "The day of the month on which to run the
-                command.  Optional; if specified, must be between 1 and 31."
+            if retval
+                return retval.to_s
+            else
+                self.fail "%s is not a valid %s" %
+                    [value, self.class.name]
+            end
         end
+    end
 
-        newproperty(:environment) do
-            desc "Any environment settings associated with this cron job.  They
-                will be stored between the header and the job in the crontab.  There
-                can be no guarantees that other, earlier settings will not also
-                affect a given cron job.
-
-                Also, Puppet cannot automatically determine whether an existing,
-                unmanaged environment setting is associated with a given cron
-                job.  If you already have cron jobs with environment settings,
-                then Puppet will keep those settings in the same place in the file,
-                but will not associate them with a specific job.
-                
-                Settings should be specified exactly as they should appear in
-                the crontab, e.g., 'PATH=/bin:/usr/bin:/usr/sbin'.  Multiple
-                settings should be specified as an array."
-
-            validate do |value|
-                unless value =~ /^\s*(\w+)\s*=\s*(.+)\s*$/
-                    raise ArgumentError, "Invalid environment setting %s" %
-                        value.inspect
-                end
-            end
+    # Somewhat uniquely, this property does not actually change anything -- it
+    # just calls +@parent.sync+, which writes out the whole cron tab for
+    # the user in question.  There is no real way to change individual cron
+    # jobs without rewriting the entire cron file.
+    #
+    # Note that this means that managing many cron jobs for a given user
+    # could currently result in multiple write sessions for that user.
+    newproperty(:command, :parent => CronParam) do
+        desc "The command to execute in the cron job.  The environment
+            provided to the command varies by local system rules, and it is
+            best to always provide a fully qualified command.  The user's
+            profile is not sourced when the command is run, so if the
+            user's environment is desired it should be sourced manually.
+            
+            All cron parameters support ``absent`` as a value; this will
+            remove any existing values for that field."
 
-            def insync?
-                if @is.is_a? Array
-                    return @is.sort == @should.sort
+        def should
+            if @should
+                if @should.is_a? Array
+                    @should[0]
                 else
-                    return @is == @should
+                    devfail "command is not an array"
                 end
+            else
+                nil
             end
+        end
+    end
 
-            def should
-                @should
-            end
+    newproperty(:special) do
+        desc "Special schedules only supported on FreeBSD."
+
+        def specials
+            %w{reboot yearly annually monthly weekly daily midnight hourly}
         end
 
-        newparam(:name) do
-            desc "The symbolic name of the cron job.  This name
-                is used for human reference only and is generated automatically
-                for cron jobs found on the system.  This generally won't
-                matter, as Puppet will do its best to match existing cron jobs
-                against specified jobs (and Puppet adds a comment to cron jobs it
-                adds), but it is at least possible that converting from
-                unmanaged jobs to managed jobs might require manual
-                intervention.
-                
-                The names can only have alphanumeric characters plus the '-'
-                character."
-
-            isnamevar
-
-            validate do |value|
-                unless value =~ /^[-\w]+$/
-                    raise ArgumentError, "Invalid name format '%s'" % value
-                end
+        validate do |value|
+            unless specials().include?(value)
+                raise ArgumentError, "Invalid special schedule %s" %
+                    value.inspect
             end
         end
+    end
 
-        newproperty(:user) do
-            desc "The user to run the command as.  This user must
-                be allowed to run cron jobs, which is not currently checked by
-                Puppet.
-                
-                The user defaults to whomever Puppet is running as."
+    newproperty(:minute, :parent => CronParam) do
+        self.boundaries = [0, 59]
+        desc "The minute at which to run the cron job.
+            Optional; if specified, must be between 0 and 59, inclusive."
+    end
 
-            defaultto { ENV["USER"] }
+    newproperty(:hour, :parent => CronParam) do
+        self.boundaries = [0, 23]
+        desc "The hour at which to run the cron job. Optional;
+            if specified, must be between 0 and 23, inclusive."
+    end
+
+    newproperty(:weekday, :parent => CronParam) do
+        def alpha
+            %w{sunday monday tuesday wednesday thursday friday saturday}
         end
+        self.boundaries = [0, 6]
+        desc "The weekday on which to run the command.
+            Optional; if specified, must be between 0 and 6, inclusive, with
+            0 being Sunday, or must be the name of the day (e.g., Tuesday)."
+    end
 
-        @doc = "Installs and manages cron jobs.  All fields except the command 
-            and the user are optional, although specifying no periodic
-            fields would result in the command being executed every
-            minute.  While the name of the cron job is not part of the actual
-            job, it is used by Puppet to store and retrieve it.
-            
-            If you specify a cron job that matches an existing job in every way
-            except name, then the jobs will be considered equivalent and the
-            new name will be permanently associated with that job.  Once this
-            association is made and synced to disk, you can then manage the job
-            normally (e.g., change the schedule of the job).
-            
-            Example:
-                
-                cron { logrotate:
-                    command => \"/usr/sbin/logrotate\",
-                    user => root,
-                    hour => 2,
-                    minute => 0
-                }
-            "
+    newproperty(:month, :parent => CronParam) do
+        def alpha
+            %w{january february march april may june july
+                august september october november december}
+        end
+        self.boundaries = [1, 12]
+        desc "The month of the year.  Optional; if specified
+            must be between 1 and 12 or the month name (e.g., December)."
+    end
 
-        attr_accessor :uid
+    newproperty(:monthday, :parent => CronParam) do
+        self.boundaries = [1, 31]
+        desc "The day of the month on which to run the
+            command.  Optional; if specified, must be between 1 and 31."
+    end
 
-        # Convert our hash to an object
-        def self.hash2obj(hash)
-            obj = nil
-            namevar = self.namevar
-            unless hash.include?(namevar) and hash[namevar]
-                Puppet.info "Autogenerating name for %s" % hash[:command]
-                hash[:name] = "autocron-%s" % hash.object_id
+    newproperty(:environment) do
+        desc "Any environment settings associated with this cron job.  They
+            will be stored between the header and the job in the crontab.  There
+            can be no guarantees that other, earlier settings will not also
+            affect a given cron job.
+
+            Also, Puppet cannot automatically determine whether an existing,
+            unmanaged environment setting is associated with a given cron
+            job.  If you already have cron jobs with environment settings,
+            then Puppet will keep those settings in the same place in the file,
+            but will not associate them with a specific job.
+            
+            Settings should be specified exactly as they should appear in
+            the crontab, e.g., 'PATH=/bin:/usr/bin:/usr/sbin'.  Multiple
+            settings should be specified as an array."
+
+        validate do |value|
+            unless value =~ /^\s*(\w+)\s*=\s*(.+)\s*$/
+                raise ArgumentError, "Invalid environment setting %s" %
+                    value.inspect
             end
+        end
 
-            unless hash.include?(:command)
-                raise Puppet::DevError, "No command for %s" % name
-            end
-            # if the cron already exists with that name...
-            if obj = (self[hash[:name]] || match(hash))
-                # Mark the cron job as present
-                obj.is = [:ensure, :present]
-
-                # Mark all of the values appropriately
-                hash.each { |param, value|
-                    if property = obj.property(param)
-                        property.is = value
-                    elsif val = obj[param]
-                        obj[param] = val
-                    else    
-                        # There is a value on disk, but it should go away
-                        obj.is = [param, value]
-                        obj[param] = :absent
-                    end
-                }
+        def insync?
+            if @is.is_a? Array
+                return @is.sort == @should.sort
             else
-                # create a new cron job, since no existing one
-                # seems to match
-                obj = self.create(
-                    :name => hash[namevar]
-                )
+                return @is == @should
+            end
+        end
 
-                obj.is = [:ensure, :present]
+        def should
+            @should
+        end
+    end
 
-                obj.notice "created"
+    newparam(:name) do
+        desc "The symbolic name of the cron job.  This name
+            is used for human reference only and is generated automatically
+            for cron jobs found on the system.  This generally won't
+            matter, as Puppet will do its best to match existing cron jobs
+            against specified jobs (and Puppet adds a comment to cron jobs it
+            adds), but it is at least possible that converting from
+            unmanaged jobs to managed jobs might require manual
+            intervention.
+            
+            The names can only have alphanumeric characters plus the '-'
+            character."
 
-                hash.delete(namevar)
-                hash.each { |param, value|
-                    obj.is = [param, value]
-                }
-            end
+        isnamevar
 
-            instance(obj)
+        validate do |value|
+            unless value =~ /^[-\w]+$/
+                raise ArgumentError, "Invalid name format '%s'" % value
+            end
         end
+    end
 
-        # See if we can match the hash against an existing cron job.
-        def self.match(hash)
-            self.find_all { |obj|
-                obj[:user] == hash[:user] and obj.value(:command) == hash[:command][0]
-            }.each do |obj|
-                # we now have a cron job whose command exactly matches
-                # let's see if the other fields match
-
-                # First check the @special stuff
-                if hash[:special]
-                    next unless obj.value(:special) == hash[:special]
-                end
+    newproperty(:user) do
+        desc "The user to run the command as.  This user must
+            be allowed to run cron jobs, which is not currently checked by
+            Puppet.
+            
+            The user defaults to whomever Puppet is running as."
 
-                # Then the normal fields.
-                matched = true
-                fields().each do |field|
-                    next if field == :command
-                    if hash[field] and ! obj.value(field)
-                        #Puppet.info "Cron is missing %s: %s and %s" %
-                        #    [field, hash[field].inspect, obj.value(field).inspect]
-                        matched = false
-                        break
-                    end
+        defaultto { ENV["USER"] }
+    end
 
-                    if ! hash[field] and obj.value(field)
-                        #Puppet.info "Hash is missing %s: %s and %s" %
-                        #    [field, obj.value(field).inspect, hash[field].inspect]
-                        matched = false
-                        break
-                    end
+    newproperty(:target) do
+        desc "Where the cron job should be stored.  For crontab-style
+            entries this is the same as the user and defaults that way.
+            Other providers default accordingly."
 
-                    # FIXME It'd be great if I could somehow reuse how the
-                    # fields are turned into text, but....
-                    next if (hash[field] == [:absent] and obj.value(field) == "*")
-                    next if (hash[field].join(",") == obj.value(field))
-                    #Puppet.info "Did not match %s: %s vs %s" %
-                    #    [field, obj.value(field).inspect, hash[field].inspect]
-                    matched = false 
-                    break
+        defaultto {
+            if provider.is_a?(@parent.class.provider(:crontab))
+                if val = @parent.should(:user)
+                    val
+                else
+                    raise ArgumentError,
+                        "You must provide a user with crontab entries"
                 end
-                next unless matched
-                return obj
+            elsif provider.class.ancestors.include?(Puppet::Provider::ParsedFile)
+                provider.class.default_target
+            else
+                nil
             end
+        }
+    end
 
-            return false
-        end
+    # We have to reorder things so that :provide is before :target
 
-        def value(name)
-            name = symbolize(name)
-            ret = nil
-            if obj = @parameters[name]
-                ret = obj.should_to_s
+    attr_accessor :uid
 
-                if ret.nil?
-                    ret = obj.is_to_s
-                end
+    def value(name)
+        name = symbolize(name)
+        ret = nil
+        if obj = @parameters[name]
+            ret = obj.should_to_s
 
-                if ret == :absent
-                    ret = nil
-                end
+            if ret.nil?
+                ret = obj.is_to_s
             end
 
-            unless ret
-                case name
-                when :command
-                    devfail "No command, somehow"
-                when :special
-                    # nothing
-                else
-                    #ret = (self.class.validproperty?(name).default || "*").to_s
-                    ret = "*"
-                end
+            if ret == :absent
+                ret = nil
             end
+        end
 
-            ret
+        unless ret
+            case name
+            when :command
+                devfail "No command, somehow"
+            when :special
+                # nothing
+            else
+                #ret = (self.class.validproperty?(name).default || "*").to_s
+                ret = "*"
+            end
         end
+
+        ret
     end
 end
 
diff --git a/lib/puppet/type/property.rb b/lib/puppet/type/property.rb
index b4dcdfae0..2cc6beb03 100644
--- a/lib/puppet/type/property.rb
+++ b/lib/puppet/type/property.rb
@@ -201,11 +201,15 @@ class Property < Puppet::Parameter
                     return event
                 end
             else
-                event = case self.should
-                when :present: (@parent.class.name.to_s + "_created").intern
-                when :absent: (@parent.class.name.to_s + "_removed").intern
+                if self.class.name == :ensure
+                    event = case self.should
+                    when :present: (@parent.class.name.to_s + "_created").intern
+                    when :absent: (@parent.class.name.to_s + "_removed").intern
+                    else
+                        (@parent.class.name.to_s + "_changed").intern
+                    end
                 else
-                    (@parent.class.name.to_s + "_changed").intern
+                    event = (@parent.class.name.to_s + "_changed").intern
                 end
             end
         end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 3bd7d277c..fa4d0fae4 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -324,9 +324,9 @@ module Util
                         Process.uid = uid unless @@os == "Darwin"
                     end
                     if command.is_a?(Array)
-                        system(*command)
+                        Kernel.exec(*command)
                     else
-                        system(command)
+                        Kernel.exec(command)
                     end
                     exit!($?.exitstatus)
                 rescue => detail
diff --git a/lib/puppet/util/fileparsing.rb b/lib/puppet/util/fileparsing.rb
index 08f1c79d9..ed92c398d 100644
--- a/lib/puppet/util/fileparsing.rb
+++ b/lib/puppet/util/fileparsing.rb
@@ -75,6 +75,25 @@ module Puppet::Util::FileParsing
             end
         end
 
+        # Convert a record into a line by joining the fields together appropriately.
+        # This is pulled into a separate method so it can be called by the hooks.
+        def join(details)
+            joinchar = self.joiner
+
+            fields.collect { |field|
+                # If the field is marked absent, use the appropriate replacement
+                if details[field] == :absent or details[field].nil?
+                    if self.optional.include?(field)
+                        self.absent
+                    else
+                        raise ArgumentError, "Field %s is required" % field
+                    end
+                else
+                    details[field].to_s
+                end
+            }.reject { |c| c.nil?}.join(joinchar)
+        end
+
         # Customize this so we can do a bit of validation.
         def optional=(optional)
             @optional = optional.collect do |field|
@@ -91,6 +110,10 @@ module Puppet::Util::FileParsing
         def pre_gen=(block)
             meta_def(:pre_gen, &block)
         end
+
+        def to_line=(block)
+            meta_def(:to_line, &block)
+        end
     end
 
     # Clear all existing record definitions.  Only used for testing.
@@ -118,28 +141,25 @@ module Puppet::Util::FileParsing
 
     # Try to match a record.
     def handle_record_line(line, record)
+        ret = nil
         if record.respond_to?(:process)
             if ret = record.send(:process, line.dup)
                 unless ret.is_a?(Hash)
                     raise Puppet::DevError,
                         "Process record type %s returned non-hash" % record.name
                 end
-                ret[:record_type] = record.name
-                return ret
             else
                 return nil
             end
         elsif regex = record.match
-            raise "Cannot use matches to handle records yet"
             # In this case, we try to match the whole line and then use the
             # match captures to get our fields.
             if match = regex.match(line)
                 fields = []
-                ignore = record.ignore || []
-                match.captures.each_with_index do |value, i|
-                    fields << value unless ignore.include? i
+                ret = {}
+                record.fields.zip(match.captures).each do |f, v|
+                    ret[f] = v
                 end
-                nil
             else
                 Puppet.info "Did not match %s" % line
                 nil
@@ -168,8 +188,13 @@ module Puppet::Util::FileParsing
                 val = ([ret[last_field]] + line_fields).join(record.joiner)
                 ret[last_field] = val
             end
+        end
+
+        if ret
             ret[:record_type] = record.name
             return ret
+        else
+            return nil
         end
     end
 
@@ -200,13 +225,7 @@ module Puppet::Util::FileParsing
             raise Puppet::DevError, "No record types defined; cannot parse lines"
         end
 
-        @record_order.each do |name|
-            record = record_type(name)
-            unless record
-                raise Puppet::DevError, "Did not get record type for %s: %s" %
-                    [name, @record_types.inspect]
-            end
-
+        @record_order.each do |record|
             # These are basically either text or record lines.
             method = "handle_%s_line" % record.type
             if respond_to?(method)
@@ -290,20 +309,11 @@ module Puppet::Util::FileParsing
         case record.type
         when :text: return details[:line]
         else
-            joinchar = record.joiner
+            if record.respond_to?(:to_line)
+                return record.to_line(details)
+            end
 
-            line = record.fields.collect { |field|
-                # If the field is marked absent, use the appropriate replacement
-                if details[field] == :absent or details[field].nil?
-                    if record.optional.include?(field)
-                        record.absent
-                    else
-                        raise ArgumentError, "Field %s is required" % field
-                    end
-                else
-                    details[field].to_s
-                end
-            }.reject { |c| c.nil?}.join(joinchar)
+            line = record.join(details)
 
             if regex = record.rts
                 # If they say true, then use whitespace; else, use their regex.
@@ -340,6 +350,7 @@ module Puppet::Util::FileParsing
     end
 
     private
+
     # Define a new type of record.
     def new_line_type(record)
         @record_types ||= {}
@@ -350,7 +361,7 @@ module Puppet::Util::FileParsing
         end
 
         @record_types[record.name] = record
-        @record_order << record.name
+        @record_order << record
 
         return record
     end