Merge branch 'ticket/2.6.next/5715' into 2.6.next

* ticket/2.6.next/5715:
  (#5715) Added attributes resource_type and title to Puppet::Resource::Status.
  (#5715) Removed attribute source_description from the YAML representation of Puppet::Resource::Status.
  (#5715) Removed unnecessary attributes from YAML of Puppet::Transaction::Event.
  (#5715) Make certain report attributes always present.
  (#5715) Changed the type of metric names to always be strings.
  (#5715) Add status attribute to reports.
  (#5715) Made the report "calculate" methods strictly functional.
  (#5715) Made the changes/total and events/total metrics always present
  (#5715) Refactor in preparation for adding a status attribute to reports.
  (#5715) Added total time to inspect reports and made inspect metrics more consistent.
  (#5715) Removed Puppet::Transaction::Report#external_times from YAML output.
  (#5715) Added total time metric to apply reports.
  (#5715) Removed redundant attribute Transaction::Event#version
  (#5715) Removed redundant attribute Resource::Status#version
  (#5715) Removed Puppet::Util::Log#version.
  (#5715) Removed the unused attribute Puppet::Transaction::Event#node
  (#5715) Removed Resource::Status#skipped_reason.  It was never used.
  (#5715) Prep work: Fixed add_statuses in report_spec.

11 files changed, 70 insertions, 64 deletions

diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb
index caa32a7c2..2f068a271 100644
--- a/lib/puppet/application/inspect.rb
+++ b/lib/puppet/application/inspect.rb
@@ -51,10 +51,8 @@ class Puppet::Application::Inspect < Puppet::Application
 
     @report.configuration_version = catalog.version
 
-    retrieval_time =  Time.now - retrieval_starttime
-    @report.add_times("config_retrieval", retrieval_time)
-
-    starttime = Time.now
+    inspect_starttime = Time.now
+    @report.add_times("config_retrieval", inspect_starttime - retrieval_starttime)
 
     catalog.to_ral.resources.each do |ral_resource|
       audited_attributes = ral_resource[:audit]
@@ -70,7 +68,9 @@ class Puppet::Application::Inspect < Puppet::Application
       @report.add_resource_status(status)
     end
 
-    @report.add_metric(:time, {"config_retrieval" => retrieval_time, "inspect" => Time.now - starttime})
+    finishtime = Time.now
+    @report.add_times("inspect", finishtime - inspect_starttime)
+    @report.finalize_report
 
     begin
       @report.save
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 070176554..d3c902576 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -170,8 +170,8 @@ class Puppet::Configurer
     send_report(report, transaction)
   end
 
-  def send_report(report, trans = nil)
-    trans.generate_report if trans
+  def send_report(report, trans)
+    report.finalize_report if trans
     puts report.summary if Puppet[:summarize]
     report.save if Puppet[:report]
   rescue => detail
diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb
index f34edc469..ee83004bb 100644
--- a/lib/puppet/resource/status.rb
+++ b/lib/puppet/resource/status.rb
@@ -4,13 +4,15 @@ module Puppet
       include Puppet::Util::Tagging
       include Puppet::Util::Logging
 
-      attr_accessor :resource, :node, :version, :file, :line, :current_values, :skipped_reason, :status, :evaluation_time
+      attr_accessor :resource, :node, :file, :line, :current_values, :status, :evaluation_time
 
       STATES = [:skipped, :failed, :failed_to_restart, :restarted, :changed, :out_of_sync, :scheduled]
       attr_accessor *STATES
 
       attr_reader :source_description, :default_log_level, :time, :resource
-      attr_reader :change_count, :out_of_sync_count
+      attr_reader :change_count, :out_of_sync_count, :resource_type, :title
+
+      YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title}
 
       # Provide a boolean method for each of the states.
       STATES.each do |attr|
@@ -47,14 +49,22 @@ module Puppet
         @resource = resource.to_s
         @change_count = 0
         @out_of_sync_count = 0
+        @changed = false
+        @out_of_sync = false
 
-        [:file, :line, :version].each do |attr|
+        [:file, :line].each do |attr|
           send(attr.to_s + "=", resource.send(attr))
         end
 
         tag(*resource.tags)
         @time = Time.now
         @events = []
+        @resource_type = resource.type.to_s.capitalize
+        @title = resource.title
+      end
+
+      def to_yaml_properties
+        (YAML_ATTRIBUTES & instance_variables).sort
       end
 
       private
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 4db971477..aa650eea1 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -221,12 +221,6 @@ class Puppet::Transaction
     end
   end
 
-  # Generate a transaction report.
-  def generate_report
-    @report.calculate_metrics
-    @report
-  end
-
   # Should we ignore tags?
   def ignore_tags?
     ! (@catalog.host_config? or Puppet[:name] == "puppet")
@@ -284,26 +278,6 @@ 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: #{detail}"
-      return
-    end
-
-    puts report.summary if Puppet[:summarize]
-
-    if Puppet[:report]
-      begin
-        report.save
-      rescue => detail
-        Puppet.err "Reporting failed: #{detail}"
-      end
-    end
-  end
-
   def add_resource_status(status)
     report.add_resource_status status
   end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index da5b14727..64980f1d9 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -7,7 +7,8 @@ class Puppet::Transaction::Event
   include Puppet::Util::Tagging
   include Puppet::Util::Logging
 
-  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :node, :version, :file, :line, :source_description, :audited]
+  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :file, :line, :source_description, :audited]
+  YAML_ATTRIBUTES = %w{@audited @property @previous_value @desired_value @historical_value @message @name @status @time}
   attr_accessor *ATTRIBUTES
   attr_writer :tags
   attr_accessor :time
@@ -16,6 +17,7 @@ class Puppet::Transaction::Event
   EVENT_STATUSES = %w{noop success failure audit}
 
   def initialize(*args)
+    @audited = false
     options = args.last.is_a?(Hash) ? args.pop : ATTRIBUTES.inject({}) { |hash, attr| hash[attr] = args.pop; hash }
     options.each { |attr, value| send(attr.to_s + "=", value) unless value.nil? }
 
@@ -46,6 +48,10 @@ class Puppet::Transaction::Event
     message
   end
 
+  def to_yaml_properties
+    (YAML_ATTRIBUTES & instance_variables).sort
+  end
+
   private
 
   # If it's a failure, use 'err', else use either the resource's log level (if available)
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 8a928454f..8e04759ad 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -11,7 +11,7 @@ class Puppet::Transaction::Report
   indirects :report, :terminus_class => :processor
 
   attr_accessor :configuration_version
-  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind
+  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind, :status
 
   # This is necessary since Marshall doesn't know how to
   # dump hash with default proc (see below @records)
@@ -43,11 +43,23 @@ class Puppet::Transaction::Report
     @resource_statuses[status.resource] = status
   end
 
-  def calculate_metrics
-    calculate_resource_metrics
-    calculate_time_metrics
-    calculate_change_metrics
-    calculate_event_metrics
+  def compute_status(resource_metrics, change_metric)
+    if (resource_metrics["failed"] || 0) > 0
+      'failed'
+    elsif change_metric > 0
+      'changed'
+    else
+      'unchanged'
+    end
+  end
+
+  def finalize_report
+    resource_metrics = add_metric(:resources, calculate_resource_metrics)
+    add_metric(:time, calculate_time_metrics)
+    change_metric = calculate_change_metric
+    add_metric(:changes, {"total" => change_metric})
+    add_metric(:events, calculate_event_metrics)
+    @status = compute_status(resource_metrics, change_metric)
   end
 
   def initialize(kind, configuration_version=nil)
@@ -61,6 +73,7 @@ class Puppet::Transaction::Report
     @report_format = 2
     @puppet_version = Puppet.version
     @configuration_version = configuration_version
+    @status = 'failed' # assume failed until the report is finalized
   end
 
   def name
@@ -96,46 +109,46 @@ class Puppet::Transaction::Report
   # individual bits represent the presence of different metrics.
   def exit_status
     status = 0
-    status |= 2 if @metrics["changes"][:total] > 0
-    status |= 4 if @metrics["resources"][:failed] > 0
+    status |= 2 if @metrics["changes"]["total"] > 0
+    status |= 4 if @metrics["resources"]["failed"] > 0
     status
   end
 
-  private
+  def to_yaml_properties
+    (instance_variables - ["@external_times"]).sort
+  end
 
-  def calculate_change_metrics
-    metrics = Hash.new(0)
-    resource_statuses.each do |name, status|
-      metrics[:total] += status.change_count if status.change_count
-    end
+  private
 
-    add_metric(:changes, metrics)
+  def calculate_change_metric
+    resource_statuses.map { |name, status| status.change_count || 0 }.inject(0) { |a,b| a+b }
   end
 
   def calculate_event_metrics
     metrics = Hash.new(0)
+    metrics["total"] = 0
     resource_statuses.each do |name, status|
-      metrics[:total] += status.events.length
+      metrics["total"] += status.events.length
       status.events.each do |event|
         metrics[event.status] += 1
       end
     end
 
-    add_metric(:events, metrics)
+    metrics
   end
 
   def calculate_resource_metrics
     metrics = Hash.new(0)
-    metrics[:total] = resource_statuses.length
+    metrics["total"] = resource_statuses.length
 
     resource_statuses.each do |name, status|
 
       Puppet::Resource::Status::STATES.each do |state|
-        metrics[state] += 1 if status.send(state)
+        metrics[state.to_s] += 1 if status.send(state)
       end
     end
 
-    add_metric(:resources, metrics)
+    metrics
   end
 
   def calculate_time_metrics
@@ -149,6 +162,8 @@ class Puppet::Transaction::Report
       metrics[name.to_s.downcase] = value
     end
 
-    add_metric(:time, metrics)
+    metrics["total"] = metrics.values.inject(0) { |a,b| a+b }
+
+    metrics
   end
 end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 1b6e7dcd7..ea3944b4e 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -446,7 +446,7 @@ class Type
   # Create a transaction event.  Called by Transaction or by
   # a property.
   def event(options = {})
-    Puppet::Transaction::Event.new({:resource => self, :file => file, :line => line, :tags => tags, :version => version}.merge(options))
+    Puppet::Transaction::Event.new({:resource => self, :file => file, :line => line, :tags => tags}.merge(options))
   end
 
   # Let the catalog determine whether a given cached value is
diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb
index 7764dc1d1..3fdac3f69 100644
--- a/lib/puppet/util/log.rb
+++ b/lib/puppet/util/log.rb
@@ -189,7 +189,7 @@ class Puppet::Util::Log
     @levels.include?(level)
   end
 
-  attr_accessor :time, :remote, :file, :line, :version, :source
+  attr_accessor :time, :remote, :file, :line, :source
   attr_reader :level, :message
 
   def initialize(args)
@@ -203,7 +203,7 @@ class Puppet::Util::Log
       tags.each { |t| self.tag(t) }
     end
 
-    [:file, :line, :version].each do |attr|
+    [:file, :line].each do |attr|
       next unless value = args[attr]
       send(attr.to_s + "=", value)
     end
@@ -234,7 +234,7 @@ class Puppet::Util::Log
 
       descriptors[:tags].each { |t| tag(t) }
 
-      [:file, :line, :version].each do |param|
+      [:file, :line].each do |param|
         next unless descriptors[param]
         send(param.to_s + "=", descriptors[param])
       end
diff --git a/lib/puppet/util/log_paths.rb b/lib/puppet/util/log_paths.rb
index f59197ed1..2fefd4505 100644
--- a/lib/puppet/util/log_paths.rb
+++ b/lib/puppet/util/log_paths.rb
@@ -15,7 +15,7 @@ module Puppet::Util::LogPaths
 
     descriptors[:tags] = tags
 
-    [:path, :file, :line, :version].each do |param|
+    [:path, :file, :line].each do |param|
       next unless value = send(param)
       descriptors[param] = value
     end
diff --git a/lib/puppet/util/logging.rb b/lib/puppet/util/logging.rb
index f20444a3b..bc52b17f0 100644
--- a/lib/puppet/util/logging.rb
+++ b/lib/puppet/util/logging.rb
@@ -26,7 +26,7 @@ module Puppet::Util::Logging
   end
 
   def log_metadata
-    [:file, :line, :version, :tags].inject({}) do |result, attr|
+    [:file, :line, :tags].inject({}) do |result, attr|
       result[attr] = send(attr) if respond_to?(attr)
       result
     end
diff --git a/lib/puppet/util/metric.rb b/lib/puppet/util/metric.rb
index 8f55e7b44..09bbb6137 100644
--- a/lib/puppet/util/metric.rb
+++ b/lib/puppet/util/metric.rb
@@ -132,6 +132,7 @@ class Puppet::Util::Metric
   end
 
   def newvalue(name,value,label = nil)
+    raise ArgumentError.new("metric name #{name.inspect} is not a string") unless name.is_a? String
     label ||= labelize(name)
     @values.push [name,label,value]
   end