(PUP-11654) Remove keys from resource_statuses when omitting excess data from report

Previously, when :exclude_unchanged_resources was set to true, the
key name would remain in the `resource_statuses` map with an
empty hash. This ended up breaking the puppetdb report class
because of an assumption that elements in `resource_statuses`
would be a Puppet::Resource::Status instance. This change removes
the key entirely from `resource_statuses`, so puppetdb no longer
errors when interating of the keys.

Additionally, there is a nebulous definition of what really a
`unchanged_resource` might be; this change expands to to be
any resource that is not failed, failed_to_restart, out_of_sync,
or skipped. In the future, it may be a good idea to add a
state named 'unchanged' instead of relying on the absence of
those other 4 attributes listed.

Author: tvpartytonight

lib/puppet/transaction/report.rb

@@ -38,6 +38,7 @@ class Puppet::Transaction::Report
   include Puppet::Util::PsychSupport
   extend Puppet::Indirector
 
+  STATES_FOR_EXCLUSION_FROM_REPORT = [:failed, :failed_to_restart, :out_of_sync, :skipped].freeze
   indirects :report, :terminus_class => :processor
 
   # The version of the configuration
@@ -308,14 +309,20 @@ def initialize_from_hash(data)
     end
   end
 
-  def skip_or_to_data_hash(rs)
-    if rs.nil?
-      nil
-    elsif !rs.out_of_sync? && Puppet[:exclude_unchanged_resources]
-      {}
-    else
-      rs.to_data_hash
+  def resource_unchanged?(rs)
+    STATES_FOR_EXCLUSION_FROM_REPORT.each do |state|
+      return false if rs.send(state)
     end
+    true
+  end
+
+  def calculate_resource_statuses
+    resource_statuses = if Puppet[:exclude_unchanged_resources]
+                          @resource_statuses.reject { |_key, rs| resource_unchanged?(rs) }
+                        else
+                          @resource_statuses
+                        end
+    Hash[resource_statuses.map { |key, rs| [key, rs.nil? ? nil : rs.to_data_hash] }]
   end
 
   def to_data_hash
@@ -333,7 +340,7 @@ def to_data_hash
       'environment' => @environment,
       'logs' => @logs.map { |log| log.to_data_hash },
       'metrics' => Hash[@metrics.map { |key, metric| [key, metric.to_data_hash] }],
-      'resource_statuses' => Hash[@resource_statuses.map { |key, rs| [key, skip_or_to_data_hash(rs)] }],
+      'resource_statuses' => calculate_resource_statuses,
       'corrective_change' => @corrective_change,
     }
 

spec/unit/transaction/report_spec.rb

@@ -143,24 +143,33 @@
     end
 
     let(:test_dir) { tmpdir('unchanged_resources') }
-    def generate_report_and_get_resource_statuses(test_dir)
+    let(:test_dir2) { tmpdir('unchanged_resources') }
+    let(:test_file) { tmpfile('some_path')}
+    it 'should still list "changed" resource statuses but remove "unchanged"' do
       transaction = apply_compiled_manifest(<<-END)
+        notify { "hi": } ~>
+        exec { "/bin/this_command_does_not_exist":
+          command => "#{make_absolute('/bin/this_command_does_not_exist')}",
+          refreshonly => true,
+        }
         file { '#{test_dir}':
           ensure => directory
         }
+        file { 'failing_file':
+          path => '#{test_dir2}',
+          ensure => file
+        }
+        file { 'skipped_file':
+          path => '#{test_file}',
+          require => File[failing_file]
+        }
       END
-      return transaction.report.to_data_hash['resource_statuses']
-    end
-
-    it 'a changed resource is still reported correctly' do
-      FileUtils.rm_rf(test_dir)
-      resource_statuses = generate_report_and_get_resource_statuses(test_dir)
-      expect(resource_statuses["File[#{test_dir}]"]['out_of_sync']).to be_truthy
-    end
-
-    it 'the status for the unchanged resource should be empty' do
-      resource_statuses = generate_report_and_get_resource_statuses(test_dir)
-      expect(resource_statuses["File[#{test_dir}]"]).to eq({})
+      rs = transaction.report.to_data_hash['resource_statuses']
+      expect(rs["Notify[hi]"]['out_of_sync']).to be true
+      expect(rs["Exec[/bin/this_command_does_not_exist]"]['failed_to_restart']).to be true
+      expect(rs["File[failing_file]"]['failed']).to be true
+      expect(rs["File[skipped_file]"]['skipped']).to be true
+      expect(rs).to_not have_key(["File[#{test_dir}]"])
     end
   end