Merge branch '2.6.x' into next

Conflicts:
	Rakefile
	lib/puppet/resource/type_collection.rb
	lib/puppet/simple_graph.rb
	lib/puppet/transaction.rb
	lib/puppet/transaction/report.rb
	lib/puppet/util/metric.rb
	spec/integration/indirector/report/rest_spec.rb
	spec/spec_specs/runnable_spec.rb
	spec/unit/configurer_spec.rb
	spec/unit/indirector_spec.rb
	spec/unit/transaction/change_spec.rb

4 files changed, 142 insertions, 151 deletions

diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
deleted file mode 100644
index d57ac1917..000000000
--- a/lib/puppet/transaction/change.rb
+++ /dev/null
@@ -1,75 +0,0 @@
-require 'puppet/transaction'
-require 'puppet/transaction/event'
-
-# Handle all of the work around performing an actual change,
-# including calling 'sync' on the properties and producing events.
-class Puppet::Transaction::Change
-  attr_accessor :is, :should, :property, :proxy, :auditing, :old_audit_value
-
-  def auditing?
-    auditing
-  end
-
-  def initialize(property, currentvalue)
-    @property = property
-    @is = currentvalue
-
-    @should = property.should
-
-    @changed = false
-  end
-
-  def apply
-    event = property.event
-    event.previous_value = is
-    event.desired_value = should
-    event.historical_value = old_audit_value
-
-    if auditing? and old_audit_value != is
-      event.message = "audit change: previously recorded value #{property.is_to_s(old_audit_value)} has been changed to #{property.is_to_s(is)}"
-      event.status = "audit"
-      event.audited = true
-      brief_audit_message = " (previously recorded value was #{property.is_to_s(old_audit_value)})" 
-    else
-      brief_audit_message = "" 
-    end
-
-    if property.insync?(is)
-      # nothing happens
-    elsif noop?
-      event.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)#{brief_audit_message}"
-      event.status = "noop"
-    else
-      property.sync
-      event.message = [ property.change_to_s(is, should), brief_audit_message ].join
-      event.status = "success"
-    end
-    event
-  rescue => detail
-    puts detail.backtrace if Puppet[:trace]
-    event.status = "failure"
-
-    event.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
-    event
-  ensure
-    event.send_log
-  end
-
-  # Is our property noop?  This is used for generating special events.
-  def noop?
-    @property.noop
-  end
-
-  # The resource that generated this change.  This is used for handling events,
-  # and the proxy resource is used for generated resources, since we can't
-  # send an event to a resource we don't have a direct relationship with.  If we
-  # have a proxy resource, then the events will be considered to be from
-  # that resource, rather than us, so the graph resolution will still work.
-  def resource
-    self.proxy || @property.resource
-  end
-
-  def to_s
-    "change #{@property.change_to_s(@is, @should)}"
-  end
-end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index da5b14727..cd695cff8 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
@@ -15,9 +16,9 @@ class Puppet::Transaction::Event
 
   EVENT_STATUSES = %w{noop success failure audit}
 
-  def initialize(*args)
-    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? }
+  def initialize(options = {})
+    @audited = false
+    options.each { |attr, value| send(attr.to_s + "=", value) }
 
     @time = Time.now
   end
@@ -46,6 +47,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 492d15da2..16fee42ae 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -10,7 +10,8 @@ class Puppet::Transaction::Report
 
   indirects :report, :terminus_class => :processor
 
-  attr_reader :resource_statuses, :logs, :metrics, :host, :time, :kind
+  attr_accessor :configuration_version
+  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)
@@ -42,14 +43,26 @@ 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 = "apply")
+  def initialize(kind, configuration_version=nil)
     @metrics = {}
     @logs = []
     @resource_statuses = {}
@@ -57,6 +70,10 @@ class Puppet::Transaction::Report
     @host = Puppet[:certname]
     @time = Time.now
     @kind = kind
+    @report_format = 2
+    @puppet_version = Puppet.version
+    @configuration_version = configuration_version
+    @status = 'failed' # assume failed until the report is finalized
   end
 
   def name
@@ -111,44 +128,45 @@ 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
 
+  def to_yaml_properties
+    (instance_variables - ["@external_times"]).sort
+  end
+
   private
 
-  def calculate_change_metrics
-    metrics = Hash.new(0)
-    resource_statuses.each do |name, status|
-      metrics[:total] += status.change_count if status.change_count
-    end
-    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
@@ -162,6 +180,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/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index c978e5545..c259d3e05 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -7,22 +7,15 @@ class Puppet::Transaction::ResourceHarness
   attr_reader :transaction
 
   def allow_changes?(resource)
-    return true unless resource.purging? and resource.deleting?
-    return true unless deps = relationship_graph.dependents(resource) and ! deps.empty? and deps.detect { |d| ! d.deleting? }
-
-    deplabel = deps.collect { |r| r.ref }.join(",")
-    plurality = deps.length > 1 ? "":"s"
-    resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
-    false
-  end
-
-  def apply_changes(status, changes)
-    changes.each do |change|
-      status << change.apply
-
-      cache(change.property.resource, change.property.name, change.is) if change.auditing?
+    if resource.purging? and resource.deleting? and deps = relationship_graph.dependents(resource) \
+            and ! deps.empty? and deps.detect { |d| ! d.deleting? }
+      deplabel = deps.collect { |r| r.ref }.join(",")
+      plurality = deps.length > 1 ? "":"s"
+      resource.warning "#{deplabel} still depend#{plurality} on me -- not purging"
+      false
+    else
+      true
     end
-    status.changed = true
   end
 
   # Used mostly for scheduling and auditing at this point.
@@ -35,66 +28,114 @@ class Puppet::Transaction::ResourceHarness
     Puppet::Util::Storage.cache(resource)[name] = value
   end
 
-  def changes_to_perform(status, resource)
+  def perform_changes(resource)
     current = resource.retrieve_resource
 
     cache resource, :checked, Time.now
 
     return [] if ! allow_changes?(resource)
 
-    audited = copy_audited_parameters(resource, current)
+    current_values = current.to_hash
+    historical_values = Puppet::Util::Storage.cache(resource).dup
+    desired_values = resource.to_resource.to_hash
+    audited_params = (resource[:audit] || []).map { |p| p.to_sym }
+    synced_params = []
 
-    if param = resource.parameter(:ensure)
-      return [] if absent_and_not_being_created?(current, param)
-      unless ensure_is_insync?(current, param)
-        audited.keys.reject{|name| name == :ensure}.each do |name|
-          resource.parameter(name).notice "audit change: previously recorded value #{audited[name]} has been changed to #{current[param]}"
-          cache(resource, name, current[param])
+    # Record the current state in state.yml.
+    audited_params.each do |param|
+      cache(resource, param, current_values[param])
+    end
+
+    # Update the machine state & create logs/events
+    events = []
+    ensure_param = resource.parameter(:ensure)
+    if desired_values[:ensure] && !ensure_param.insync?(current_values[:ensure])
+      events << apply_parameter(ensure_param, current_values[:ensure], audited_params.include?(:ensure), historical_values[:ensure])
+      synced_params << :ensure
+    elsif current_values[:ensure] != :absent
+      work_order = resource.properties # Note: only the resource knows what order to apply changes in
+      work_order.each do |param|
+        if !param.insync?(current_values[param.name])
+          events << apply_parameter(param, current_values[param.name], audited_params.include?(param.name), historical_values[param.name])
+          synced_params << param.name
         end
-        return [Puppet::Transaction::Change.new(param, current[:ensure])]
       end
-      return [] if ensure_should_be_absent?(current, param)
     end
 
-    resource.properties.reject { |param| param.name == :ensure }.select do |param|
-      (audited.include?(param.name) && audited[param.name] != current[param.name]) || (param.should != nil && !param_is_insync?(current, param))
-    end.collect do |param|
-      change = Puppet::Transaction::Change.new(param, current[param.name])
-      change.auditing = true if audited.include?(param.name)
-      change.old_audit_value = audited[param.name]
-      change
+    # Add more events to capture audit results
+    audited_params.each do |param_name|
+      if historical_values.include?(param_name)
+        if historical_values[param_name] != current_values[param_name] && !synced_params.include?(param_name)
+          event = create_change_event(resource.parameter(param_name), current_values[param_name], true, historical_values[param_name])
+          event.send_log
+          events << event
+        end
+      else
+        resource.property(param_name).notice "audit change: newly-recorded value #{current_values[param_name]}"
+      end
     end
+
+    events
   end
 
-  def copy_audited_parameters(resource, current)
-    return {} unless audit = resource[:audit]
-    audit = Array(audit).collect { |p| p.to_sym }
-    audited = {}
-    audit.find_all do |param|
-      if value = cached(resource, param)
-        audited[param] = value
-      else
-        resource.property(param).notice "audit change: newly-recorded recorded value #{current[param]}"
-        cache(resource, param, current[param])
+  def create_change_event(property, current_value, do_audit, historical_value)
+    event = property.event
+    event.previous_value = current_value
+    event.desired_value = property.should
+    event.historical_value = historical_value
+
+    if do_audit
+      event.audited = true
+      event.status = "audit"
+      if historical_value != current_value
+        event.message = "audit change: previously recorded value #{property.is_to_s(historical_value)} has been changed to #{property.is_to_s(current_value)}"
       end
     end
 
-    audited
+    event
+  end
+
+  def apply_parameter(property, current_value, do_audit, historical_value)
+    event = create_change_event(property, current_value, do_audit, historical_value)
+
+    if do_audit && historical_value && historical_value != current_value
+      brief_audit_message = " (previously recorded value was #{property.is_to_s(historical_value)})"
+    else
+      brief_audit_message = ""
+    end
+
+    if property.noop
+      event.message = "current_value #{property.is_to_s(current_value)}, should be #{property.should_to_s(property.should)} (noop)#{brief_audit_message}"
+      event.status = "noop"
+    else
+      property.sync
+      event.message = [ property.change_to_s(current_value, property.should), brief_audit_message ].join
+      event.status = "success"
+    end
+    event
+  rescue => detail
+    puts detail.backtrace if Puppet[:trace]
+    event.status = "failure"
+
+    event.message = "change from #{property.is_to_s(current_value)} to #{property.should_to_s(property.should)} failed: #{detail}"
+    event
+  ensure
+    event.send_log
   end
 
   def evaluate(resource)
     start = Time.now
     status = Puppet::Resource::Status.new(resource)
 
-    if changes = changes_to_perform(status, resource) and ! changes.empty?
-      status.out_of_sync = true
-      status.change_count = changes.length
-      apply_changes(status, changes)
-      if ! resource.noop?
-        cache(resource, :synced, Time.now)
-        resource.flush if resource.respond_to?(:flush)
-      end
+    perform_changes(resource).each do |event|
+      status << event
     end
+
+    if status.changed? && ! resource.noop?
+      cache(resource, :synced, Time.now)
+      resource.flush if resource.respond_to?(:flush)
+    end
+
     return status
   rescue => detail
     resource.fail "Could not create resource status: #{detail}" unless status