Fixing #749 -- environment settings no longer accumulate. Significantly adding to the cron tests at the same time, such that hopefully we will no longer have these recurring bugs. I now do every combinatorial of multi-line cron jobs, including doing them all in one file. There are, unfortunately, still edge cases, but maybe we will have some peace in cron space for a while, anyway.

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

4 files changed, 56 insertions, 38 deletions

diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index 7ef0bdce9..b836887e9 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -337,7 +337,9 @@ class Puppet::Provider
 
     def initialize(resource = nil)
         if resource.is_a?(Hash)
-            @property_hash = resource.dup
+            # We don't use a duplicate here, because some providers (ParsedFile, at least)
+            # use the hash here for later events.
+            @property_hash = resource
         elsif resource
             @resource = resource if resource
             # LAK 2007-05-09: Keep the model stuff around for backward compatibility
diff --git a/lib/puppet/provider/cron/crontab.rb b/lib/puppet/provider/cron/crontab.rb
index 3296544f9..afdcc5f65 100755
--- a/lib/puppet/provider/cron/crontab.rb
+++ b/lib/puppet/provider/cron/crontab.rb
@@ -78,10 +78,10 @@ Puppet::Type.type(:cron).provide(:crontab,
     # 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.
-# 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}
+%{# HEADER: This file was autogenerated at #{Time.now} by puppet.
+# HEADER: While it can still be managed manually, it is definitely not recommended.
+# 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 record against an existing cron job.
@@ -134,31 +134,44 @@ Puppet::Type.type(:cron).provide(:crontab,
     # Collapse name and env records.
     def self.prefetch_hook(records)
         name = nil
-        envs = []
-        records.each { |record|
+        envs = nil
+        result = records.each { |record|
             case record[:record_type]
             when :comment:
                 if record[:name]
                     name = record[:name]
                     record[:skip] = true
+                    
+                    # Start collecting env values
+                    envs = []
                 end
             when :environment:
-                if name
+                # If we're collecting env values (meaning we're in a named cronjob),
+                # store the line and skip the record.
+                if envs
                     envs << record[:line]
                     record[:skip] = true
                 end
+            when :blank:
+                # nothing
             else
                 if name
                     record[:name] = name
                     name = nil
                 end
-                if envs.empty?
+                if envs.nil? or envs.empty?
                     record[:environment] = :absent
                 else
+                    # Collect all of the environment lines, and mark the records to be skipped,
+                    # since their data is included in our crontab record.
                     record[:environment] = envs
+
+                    # And turn off env collection again
+                    envs = nil
                 end
             end
         }.reject { |record| record[:skip] }
+        result
     end
 
     def self.to_file(records)
diff --git a/lib/puppet/provider/parsedfile.rb b/lib/puppet/provider/parsedfile.rb
index a26788843..6cac275cf 100755
--- a/lib/puppet/provider/parsedfile.rb
+++ b/lib/puppet/provider/parsedfile.rb
@@ -110,7 +110,7 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
     def self.instances
         prefetch()
         @records.find_all { |r| r[:record_type] == self.name }.collect { |r|
-            new(clean(r))
+            new(r)
         }
     end
 
@@ -134,27 +134,16 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
                 if @property_hash[attr] or self.class.valid_attr?(self.class.name, attr)
                     @property_hash[attr] || :absent
                 else
-                    @resource.should(attr)
+                    if defined? @resource
+                        @resource.should(attr)
+                    else
+                        nil
+                    end
                 end
             end
 
             define_method(attr.to_s + "=") do |val|
-                # Mark that this target was modified.
-                resourcetarget = @resource.should(:target) || self.class.default_target
-
-                # If they're the same, then just mark that one as modified
-                if @property_hash[:target] and @property_hash[:target] == resourcetarget
-                    self.class.modified(resourcetarget)
-                else
-                    # Always mark the resourcetarget as modified, and if there's
-                    # and old property_hash target, mark it as modified and replace
-                    # it.
-                    self.class.modified(resourcetarget)
-                    if @property_hash[:target]
-                        self.class.modified(@property_hash[:target])
-                    end
-                    @property_hash[:target] = resourcetarget
-                end
+                mark_target_modified
                 @property_hash[attr] = val
             end
         end
@@ -296,14 +285,13 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         header + text
     end
 
-
     def create
         @resource.class.validproperties.each do |property|
             if value = @resource.should(property)
                 @property_hash[property] = value
             end
         end
-        self.class.modified(@property_hash[:target] || self.class.default_target)
+        mark_target_modified()
         return (@resource.class.name.to_s + "_created").intern
     end
 
@@ -338,16 +326,15 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         #@property_hash = {}
     end
 
-    def initialize(resource)
+    def initialize(record)
         super
 
-        # See if there's already a matching property_hash in the records list;
-        # else, use a default value.
-        # We provide a default for 'ensure' here, because the provider will
-        # override it if the thing exists, but it won't touch it if it doesn't
-        # exist.
-        @property_hash = self.class.record?(resource[:name]) ||
-            {:record_type => self.class.name, :ensure => :absent}
+        # The 'record' could be a resource or a record, depending on how the provider
+        # is initialized.  If we got an empty property hash (probably because the resource
+        # is just being initialized), then we want to set up some defualts.
+        if @property_hash.empty?
+            @property_hash = self.class.record?(resource[:name]) || {:record_type => self.class.name, :ensure => :absent}
+        end
     end
 
     # Retrieve the current state from disk.
@@ -357,6 +344,22 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         end
         self.class.prefetch(@resource[:name] => @resource)
     end
+
+    def record_type
+        @property_hash[:record_type]
+    end
+
+    private
+
+    # Mark both the resource and provider target as modified.
+    def mark_target_modified
+        if defined? @resource and restarget = @resource.should(:target) and restarget != @property_hash[:target]
+            self.class.modified(restarget)
+        end
+        if @property_hash[:target] != :absent
+            self.class.modified(@property_hash[:target])
+        end
+    end
 end
 
 # $Id$
diff --git a/lib/puppet/util/fileparsing.rb b/lib/puppet/util/fileparsing.rb
index 8e69681cf..6a04dee5a 100644
--- a/lib/puppet/util/fileparsing.rb
+++ b/lib/puppet/util/fileparsing.rb
@@ -320,7 +320,7 @@ module Puppet::Util::FileParsing
     # Convert our parsed record into a text record.
     def to_line(details)
         unless record = record_type(details[:record_type])
-            raise ArgumentError, "Invalid record type %s" % details[:record_type]
+            raise ArgumentError, "Invalid record type %s" % details[:record_type].inspect
         end
 
         if record.respond_to?(:pre_gen)