Changing Transaction to use the new ResourceHarness

This is a much messier commit than I would like,
mostly because of how 'file' works.  I had to
fix multiple special cases, and I had to move others.

The whole system appears to now work, though, and we're
ready to change reports to receive resource status
instances rather than events.

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

4 files changed, 114 insertions, 186 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index dd28b5588..9da761a8c 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -8,6 +8,8 @@ class Puppet::Transaction
     require 'puppet/transaction/change'
     require 'puppet/transaction/event'
     require 'puppet/transaction/event_manager'
+    require 'puppet/transaction/resource_harness'
+    require 'puppet/resource/status'
 
     attr_accessor :component, :catalog, :ignoreschedules
     attr_accessor :sorted_resources, :configurator
@@ -21,6 +23,9 @@ class Puppet::Transaction
     # Routes and stores any events and subscriptions.
     attr_reader :event_manager
 
+    # Handles most of the actual interacting with resources
+    attr_reader :resource_harness
+
     include Puppet::Util
     include Puppet::Util::Tagging
 
@@ -60,38 +65,13 @@ class Puppet::Transaction
 
     # Apply all changes for a resource
     def apply(resource)
-        begin
-            changes = resource.evaluate
-        rescue => detail
-            puts detail.backtrace if Puppet[:trace]
-
-            resource.err "Failed to retrieve current state of resource: %s" % detail
-
-            # Mark that it failed
-            @failures[resource] += 1
-
-            # And then return
-            return
-        end
-
-        changes = [changes] unless changes.is_a?(Array)
-
-        @resourcemetrics[:out_of_sync] += 1 if changes.length > 0
-
-        return if changes.empty? or ! allow_processing?(resource, changes)
-
-        apply_changes(resource, changes)
-
-        # If there were changes and the resource isn't in noop mode...
-        unless changes.empty? or resource.noop
-            # Record when we last synced
-            resource.cache(:synced, Time.now)
-
-            # Flush, if appropriate
-            if resource.respond_to?(:flush)
-                resource.flush
-            end
+        status = resource_harness.evaluate(resource)
+        add_resource_status(status)
+        status.events.each do |event|
+            event_manager.queue_event(resource, event)
         end
+    rescue => detail
+        resource.err "Could not evaluate: #{detail}"
     end
 
     # Apply each change in turn.
@@ -138,13 +118,6 @@ class Puppet::Transaction
         end
     end
 
-    # Are we deleting this resource?
-    def deleting?(changes)
-        changes.detect { |change|
-            change.property.name == :ensure and change.should == :absent
-        }
-    end
-
     # See if the resource generates new resources at evaluation time.
     def eval_generate(resource)
         generate_additional_resources(resource, :eval_generate)
@@ -237,13 +210,8 @@ class Puppet::Transaction
         event_manager.events
     end
 
-    # Determine whether a given resource has failed.
-    def failed?(obj)
-        if @failures[obj] > 0
-            return @failures[obj]
-        else
-            return false
-        end
+    def failed?(resource)
+        s = resource_status(resource) and s.failed?
     end
 
     # Does this resource have any failed dependencies?
@@ -252,17 +220,14 @@ class Puppet::Transaction
         # we check for failures in any of the vertexes above us.  It's not
         # enough to check the immediate dependencies, which is why we use
         # a tree from the reversed graph.
-        skip = false
-        deps = relationship_graph.dependencies(resource)
-        deps.each do |dep|
-            if fails = failed?(dep)
-                resource.notice "Dependency %s[%s] has %s failures" %
-                    [dep.class.name, dep.name, @failures[dep]]
-                skip = true
-            end
+        found_failed = false
+        relationship_graph.dependencies(resource).each do |dep|
+            next unless failed?(dep)
+            resource.notice "Dependency #{dep} has failures: #{resource_status(dep).failed}"
+            found_failed = true
         end
 
-        return skip
+        return found_failed
     end
 
     # A general method for recursively generating new resources from a
@@ -363,6 +328,10 @@ class Puppet::Transaction
         @report = Report.new
 
         @event_manager = Puppet::Transaction::EventManager.new(self)
+
+        @resource_harness = Puppet::Transaction::ResourceHarness.new(self)
+
+        @resource_status = {}
     end
 
     # Prefetch any providers that support it.  We don't support prefetching
@@ -407,6 +376,36 @@ class Puppet::Transaction
         catalog.relationship_graph
     end
 
+    # Send off the transaction report.
+    def send_report
+        begin
+            report = generate_report()
+        rescue => detail
+            Puppet.err "Could not generate report: %s" % detail
+            return
+        end
+
+        if Puppet[:summarize]
+            puts report.summary
+        end
+
+        if Puppet[:report]
+            begin
+                report.save()
+            rescue => detail
+                Puppet.err "Reporting failed: %s" % detail
+            end
+        end
+    end
+
+    def add_resource_status(status)
+        @resource_status[status.resource] = status
+    end
+
+    def resource_status(resource)
+        @resource_status[resource.to_s]
+    end
+
     # Roll all completed changes back.
     def rollback
         @changes.reverse.collect do |change|
@@ -480,21 +479,6 @@ class Puppet::Transaction
     def appropriately_tagged?(resource)
         self.ignore_tags? or tags.empty? or resource.tagged?(*tags)
     end
-
-    private
-
-    def apply_change(resource, change)
-        @changes << change
-
-        event = change.forward
-
-        if event.status == "success"
-            @resourcemetrics[:applied] += 1
-        else
-            @failures[resource] += 1
-        end
-        event_manager.queue_event(resource, event)
-    end
 end
 
 require 'puppet/transaction/report'
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index da859910c..669b0ae98 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -26,14 +26,16 @@ class Puppet::Transaction::ResourceHarness
     def changes_to_perform(status, resource)
         current = resource.retrieve
 
+        resource.cache :checked, Time.now
+
         if param = resource.parameter(:ensure)
-            insync = param.insync?(current[:ensure])
-            return [Puppet::Transaction::Change.new(param, current[:ensure])] unless insync
-            return [] if param.should == :absent
+            return [] if absent_and_not_being_created?(current, param)
+            return [Puppet::Transaction::Change.new(param, current[:ensure])] unless ensure_is_insync?(current, param)
+            return [] if ensure_should_be_absent?(current, param)
         end
 
         resource.properties.reject { |p| p.name == :ensure }.find_all do |param|
-            ! param.insync?(current[param.name])
+            param_is_not_insync?(current, param)
         end.collect do |param|
             Puppet::Transaction::Change.new(param, current[param.name])
         end
@@ -51,6 +53,7 @@ class Puppet::Transaction::ResourceHarness
         return status
     rescue => detail
         resource.fail "Could not create resource status: #{detail}" unless status
+        puts detail.backtrace if Puppet[:trace]
         resource.err "Could not evaluate: #{detail}"
         status.failed = true
         return status
@@ -59,4 +62,22 @@ class Puppet::Transaction::ResourceHarness
     def initialize(transaction)
         @transaction = transaction
     end
+
+    private
+
+    def absent_and_not_being_created?(current, param)
+        current[:ensure] == :absent and param.should.nil?
+    end
+
+    def ensure_is_insync?(current, param)
+        param.insync?(current[:ensure])
+    end
+
+    def ensure_should_be_absent?(current, param)
+        param.should == :absent
+    end
+
+    def param_is_not_insync?(current, param)
+        ! param.insync?(current[param.name] || :absent)
+    end
 end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index d17a56d67..1df84f2df 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -620,31 +620,12 @@ class Type
         catalog.version
     end
 
-    # Meta-parameter methods:  These methods deal with the results
-    # of specifying metaparameters
-
-    private
-
     # Return all of the property objects, in the order specified in the
     # class.
     def properties
-        #debug "%s has %s properties" % [self,@parameters.length]
-        props = self.class.properties.collect { |prop|
-            @parameters[prop.name]
-        }.find_all { |p|
-            ! p.nil?
-        }.each do |prop|
-            unless prop.is_a?(Puppet::Property)
-                raise Puppet::DevError, "got a non-property %s(%s)" %
-                    [prop.class, prop.class.name]
-            end
-        end
-
-        props
+        self.class.properties.collect { |prop| @parameters[prop.name] }.compact
     end
 
-    public
-
     # Is this type's name isomorphic with the object?  That is, if the
     # name conflicts, does it necessarily mean that the objects conflict?
     # Defaults to true.
@@ -722,43 +703,6 @@ class Type
     ###############################
     # Code related to evaluating the resources.
 
-    # This method is responsible for collecting property changes we always
-    # descend into the children before we evaluate our current properties.
-    # This returns any changes resulting from testing, thus 'collect' rather
-    # than 'each'.
-    def evaluate
-        if self.provider.is_a?(Puppet::Provider)
-            unless provider.class.suitable?
-                raise Puppet::Error, "Provider %s is not functional on this platform" % provider.class.name
-            end
-        end
-
-        # this only operates on properties, not properties + children
-        # it's important that we call retrieve() on the type instance,
-        # not directly on the property, because it allows the type to override
-        # the method, like pfile does
-        currentvalues = self.retrieve
-
-        changes = propertychanges(currentvalues).flatten
-
-        # now record how many changes we've resulted in
-        if changes.length > 0
-            self.debug "%s change(s)" %
-                [changes.length]
-        end
-
-        # If we're in noop mode, we don't want to store the checked time,
-        # because it will result in the resource not getting scheduled if
-        # someone were to apply the catalog in non-noop mode.
-        # We're going to go ahead and record that we checked if there were
-        # no changes, since it's unlikely it will affect the scheduling.
-        noop = noop?
-        if ! noop or (noop && changes.length == 0)
-            self.cache(:checked, Time.now)
-        end
-        return changes.flatten
-    end
-
     # Flush the provider, if it supports it.  This is called by the
     # transaction.
     def flush
@@ -808,7 +752,31 @@ class Type
 
     # retrieve the current value of all contained properties
     def retrieve
-        return currentpropvalues
+        if self.provider.is_a?(Puppet::Provider) and ! provider.class.suitable?
+            fail "Provider #{provider.class.name} is not functional on this host"
+        end
+
+        result = Puppet::Resource.new(type, title)
+
+        # Provide the name, so we know we'll always refer to a real thing
+        result[:name] = self[:name] unless self[:name] == title
+
+        if ensure_prop = property(:ensure) or (self.class.validattr?(:ensure) and ensure_prop = newattr(:ensure))
+            result[:ensure] = ensure_state = ensure_prop.retrieve
+        else
+            ensure_state = nil
+        end
+
+        properties.each do |property|
+            next if property.name == :ensure
+            if ensure_state == :absent
+                result[property] = :absent
+            else
+                result[property] = property.retrieve
+            end
+        end
+
+        result
     end
 
     # Get a hash of the current properties.  Returns a hash with
@@ -848,42 +816,6 @@ class Type
         noop?
     end
 
-    # Retrieve the changes associated with all of the properties.
-    def propertychanges(currentvalues)
-        # If we are changing the existence of the object, then none of
-        # the other properties matter.
-        changes = []
-        ensureparam = @parameters[:ensure]
-
-        # This allows resource types to have 'ensure' be a parameter, which allows them to
-        # just pass the parameter on to other generated resources.
-        ensureparam = nil unless ensureparam.is_a?(Puppet::Property)
-        if ensureparam && !currentvalues.include?(ensureparam)
-            raise Puppet::DevError, "Parameter ensure defined but missing from current values"
-        end
-
-        if ensureparam and ! ensureparam.insync?(currentvalues[ensureparam])
-            changes << Puppet::Transaction::Change.new(ensureparam, currentvalues[ensureparam])
-        # Else, if the 'ensure' property is correctly absent, then do
-        # nothing
-        elsif ensureparam and currentvalues[ensureparam] == :absent
-            return []
-        else
-            changes = properties().find_all { |property|
-                currentvalues[property] ||= :absent
-                ! property.insync?(currentvalues[property])
-            }.collect { |property|
-                Puppet::Transaction::Change.new(property, currentvalues[property])
-            }
-        end
-
-        if Puppet[:debug] and changes.length > 0
-            self.debug("Changing " + changes.collect { |ch| ch.property.name }.join(","))
-        end
-
-        changes
-    end
-
     ###############################
     # Code related to managing resource instances.
     require 'puppet/transportable'
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index d995add0d..895c0b207 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -404,6 +404,16 @@ Puppet::Type.newtype(:file) do
 
         super
 
+        # If they've specified a source, we get our 'should' values
+        # from it.
+        unless self[:ensure]
+            if self[:target]
+                self[:ensure] = :symlink
+            elsif self[:content]
+                self[:ensure] = :file
+            end
+        end
+
         @title = self.class.canonicalize_ref(@title)
         @stat = nil
     end
@@ -630,7 +640,6 @@ Puppet::Type.newtype(:file) do
         expire
     end
 
-    # a wrapper method to make sure the file exists before doing anything
     def retrieve
         if source = parameter(:source)
             source.copy_source_values
@@ -784,24 +793,6 @@ Puppet::Type.newtype(:file) do
         self.fail "File written to disk did not match checksum; discarding changes (%s vs %s)" % [checksum, newsum]
     end
 
-    # Override the parent method, because we don't want to generate changes
-    # when the file is missing and there is no 'ensure' state.
-    def propertychanges(currentvalues)
-        unless self.stat
-            found = false
-            ([:ensure] + CREATORS).each do |prop|
-                if @parameters.include?(prop)
-                    found = true
-                    break
-                end
-            end
-            unless found
-                return []
-            end
-        end
-        super
-    end
-
     # There are some cases where all of the work does not get done on
     # file creation/modification, so we have to do some extra checking.
     def property_fix