Fixing #1054 - transaction reports are always sent

This refactors how reports, catalogs, configurers, and transactions
are all related - the Configurer class manages the report, both
creating and sending it, so the transaction is now just responsible
for adding data to it.  I'm still a bit uncomfortable of the coupling
between transactions, the report, and configurer, but it's better than
it was.

This also fixes #2944 and #2973.

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

3 files changed, 43 insertions, 64 deletions

diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index c2437153d..350e9c34f 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -62,6 +62,10 @@ class Puppet::Configurer
         @splayed = false
     end
 
+    def initialize_report
+        Puppet::Transaction::Report.new
+    end
+
     # Prepare for catalog retrieval.  Downloads everything necessary, etc.
     def prepare
         dostorage()
@@ -135,6 +139,9 @@ class Puppet::Configurer
             Puppet.err "Failed to prepare catalog: %s" % detail
         end
 
+        report = initialize_report()
+        Puppet::Util::Log.newdestination(report)
+
         if catalog = options[:catalog]
             options.delete(:catalog)
         elsif ! catalog = retrieve_catalog
@@ -142,11 +149,11 @@ class Puppet::Configurer
             return
         end
 
+        transaction = nil
+
         begin
             benchmark(:notice, "Finished catalog run") do
                 transaction = catalog.apply(options)
-                transaction.generate_report
-                report = transaction.report
             end
             report
         rescue => detail
@@ -158,6 +165,19 @@ class Puppet::Configurer
         # Now close all of our existing http connections, since there's no
         # reason to leave them lying open.
         Puppet::Network::HttpPool.clear_http_instances
+
+        Puppet::Util::Log.close(report)
+
+        send_report(report, transaction)
+    end
+
+    def send_report(report, trans = nil)
+        trans.add_metrics_to_report(report) if trans
+        puts report.summary if Puppet[:summarize]
+        report.save() if Puppet[:report]
+    rescue => detail
+        puts detail.backtrace if Puppet[:trace]
+        Puppet.err "Could not send report: #{detail}"
     end
 
     private
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index f21c820a0..cb6128517 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -135,7 +135,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
         transaction.tags = options[:tags] if options[:tags]
         transaction.ignoreschedules = true if options[:ignoreschedules]
 
-        transaction.addtimes :config_retrieval => self.retrieval_duration
+        transaction.addtimes :config_retrieval => self.retrieval_duration || 0
 
 
         begin
@@ -154,8 +154,6 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
 
         yield transaction if block_given?
 
-        transaction.send_report if host_config and (Puppet[:report] or Puppet[:summarize])
-
         return transaction
     ensure
         @applying = false
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 21aa7a36d..725b86dc5 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -12,9 +12,6 @@ class Transaction
     attr_accessor :component, :catalog, :ignoreschedules
     attr_accessor :sorted_resources, :configurator
 
-    # The report, once generated.
-    attr_reader :report
-
     # The list of events generated in this transaction.
     attr_reader :events
 
@@ -278,33 +275,25 @@ class Transaction
     def evaluate
         @count = 0
 
-        # Start logging.
-        Puppet::Util::Log.newdestination(@report)
-
         prepare()
 
         Puppet.info "Applying configuration version '%s'" % catalog.version if catalog.version
 
-        begin
-            allevents = @sorted_resources.collect { |resource|
-                if resource.is_a?(Puppet::Type::Component)
-                    Puppet.warning "Somehow left a component in the relationship graph"
-                    next
-                end
-                ret = nil
-                seconds = thinmark do
-                    ret = eval_resource(resource)
-                end
+        allevents = @sorted_resources.collect { |resource|
+            if resource.is_a?(Puppet::Type::Component)
+                Puppet.warning "Somehow left a component in the relationship graph"
+                next
+            end
+            ret = nil
+            seconds = thinmark do
+                ret = eval_resource(resource)
+            end
 
-                if Puppet[:evaltrace] and @catalog.host_config?
-                    resource.info "Evaluated in %0.2f seconds" % seconds
-                end
-                ret
-            }.flatten.reject { |e| e.nil? }
-        ensure
-            # And then close the transaction log.
-            Puppet::Util::Log.close(@report)
-        end
+            if Puppet[:evaltrace] and @catalog.host_config?
+                resource.info "Evaluated in %0.2f seconds" % seconds
+            end
+            ret
+        }.flatten.reject { |e| e.nil? }
 
         Puppet.debug "Finishing transaction %s with %s changes" %
             [self.object_id, @count]
@@ -382,8 +371,7 @@ class Transaction
         end
     end
 
-    # Generate a transaction report.
-    def generate_report
+    def add_metrics_to_report(report)
         @resourcemetrics[:failed] = @failures.find_all do |name, num|
             num > 0
         end.length
@@ -395,19 +383,15 @@ class Transaction
         end
 
         # Add all of the metrics related to resource count and status
-        @report.newmetric(:resources, @resourcemetrics)
+        report.newmetric(:resources, @resourcemetrics)
 
         # Record the relative time spent in each resource.
-        @report.newmetric(:time, @timemetrics)
+        report.newmetric(:time, @timemetrics)
 
         # Then all of the change-related metrics
-        @report.newmetric(:changes,
-            :total => @changes.length
-        )
-
-        @report.time = Time.now
+        report.newmetric(:changes, :total => @changes.length)
 
-        return @report
+        report.time = Time.now
     end
 
     # Should we ignore tags?
@@ -418,7 +402,7 @@ class Transaction
     # this should only be called by a Puppet::Type::Component resource now
     # and it should only receive an array
     def initialize(catalog)
-        @catalog = resources
+        @catalog = catalog
 
         @resourcemetrics = {
             :total => @catalog.vertices.length,
@@ -452,7 +436,6 @@ class Transaction
             h[key] = 0
         end
 
-        @report = Report.new
         @count = 0
     end
 
@@ -498,28 +481,6 @@ class 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
-
     # Roll all completed changes back.
     def rollback
         @targets.clear