Merge pull request #61 from github/kpaulisse-absent-to-filter

Change the "ignore changes to absent file" to a filter

Author: kpaulisse

doc/advanced-filter.md

@@ -9,8 +9,66 @@ Please note that there are other options to ignore specified diffs, including:
 
 Here is the list of available filters and an explanation of each:
 
+- [Absent file](/doc/advanced-filter.md#absent-file) - Ignore parameter changes of a file that is declared to be absent
 - [YAML](/doc/advanced-filter.md#yaml) - Ignore whitespace/comment differences if YAML parses to the same object
 
+## Absent File
+
+#### Usage
+
+```
+--filters AbsentFile
+```
+
+#### Description
+
+When the `AbsentFile` filter is enabled, if any file is `ensure => absent` in the *new* catalog, then changes to any other parameters will be suppressed.
+
+Consider that a file resource is declared as follows in two catalogs:
+
+```
+# Old catalog
+file { '/etc/some-file':
+  ensure  => present,
+  owner   => 'root',
+  group   => 'nobody',
+  content => 'my content here',
+}
+
+# New catalog
+file { '/etc/some-file':
+  ensure => absent,
+  owner  => 'bob',
+}
+```
+
+Since the practical effect of the new catalog will be to remove the file, it doesn't matter that the owner of the (non-existent) file has changed from 'root' to 'bob', or that the content and group have changed from a string to undefined. Consider the default output without the filter:
+
+```
+  File[/etc/some-file] =>
+   parameters =>
+     ensure =>
+      - present
+      + absent
+     group =>
+      - nobody
+     owner =>
+      - root
+      + bob
+     content =>
+      - my content here
+```
+
+Wouldn't it be nice if the meaningless information didn't appear, and all you saw was the transition you actually care about, from present to absent? With `--filters AbsentFile` it does just this:
+
+```
+  File[/etc/some-file] =>
+   parameters =>
+     ensure =>
+      - present
+      + absent
+```
+
 ## YAML
 
 #### Usage

doc/optionsref.md

@@ -1036,7 +1036,8 @@ cached directory). (<a href="../lib/octocatalog-diff/cli/options/safe_to_delete_
     <td valign=top>
       If enabled, this option will suppress changes to certain attributes of a file, if the
 file is specified to be 'absent' in the target catalog. Suppressed changes in this case
-include user, group, mode, and content, because a removed file has none of those. (<a href="../lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb">suppress_absent_file_details.rb</a>)
+include user, group, mode, and content, because a removed file has none of those.
+<i>This option is DEPRECATED; please use <code>--filters AbsentFile</code> instead.</i> (<a href="../lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb">suppress_absent_file_details.rb</a>)
     </td>
   </tr>
 

lib/octocatalog-diff/catalog-diff/differ.rb

@@ -147,12 +147,14 @@ def catdiff
         # Remove resources that have been explicitly ignored
         filter_diffs_for_ignored_items(result)
 
-        # If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
-        # out any such parameters from the result array.
-        filter_diffs_for_absent_files(result) if @opts[:suppress_absent_file_details]
+        # Legacy options which are now filters
+        @opts[:filters] ||= []
+        if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('absent_file')
+          @opts[:filters] << 'AbsentFile'
+        end
 
         # Apply any additional pluggable filters.
-        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters])
+        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters]) if @opts[:filters].any?
 
         # That's it!
         @logger.debug "Exiting catdiff; change count: #{result.size}"
@@ -166,43 +168,6 @@ def filter_diffs_for_ignored_items(result)
         result.reject! { |item| ignored?(item) }
       end
 
-      # If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
-      # out any such parameters from the result array.
-      # @param result [Array] Diff result list (modified by this method)
-      def filter_diffs_for_absent_files(result)
-        @logger.debug "Entering filter_diffs_for_absent_files with #{result.size} diffs"
-
-        # Scan for files in the result that are file resources with ensure => absent.
-        absent_files = Set.new
-        result.each do |diff|
-          next unless diff[0] == '~' || diff[0] == '!'
-          next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
-          next unless ['absent', 'false', false].include?(diff[3])
-          absent_files.add Regexp.last_match(1)
-        end
-
-        # If there are any absent files, remove all diffs referencing that file, except for
-        # the change to 'ensure'.
-        if absent_files.any?
-          keep = %w(ensure backup force provider)
-          result.map! do |diff|
-            if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
-              if absent_files.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
-                @logger.debug "Removing file=#{Regexp.last_match(1)} parameter=#{Regexp.last_match(2)} for absent file"
-                nil
-              else
-                diff
-              end
-            else
-              diff
-            end
-          end
-          result.compact!
-        end
-
-        @logger.debug "Exiting filter_diffs_for_absent_files with #{result.size} diffs"
-      end
-
       # Pre-processing of a catalog.
       # - Remove 'before' and 'require' from parameters
       # - Sort 'tags' array, or remove the tags array if tags are being ignored

lib/octocatalog-diff/catalog-diff/filter.rb

@@ -1,3 +1,4 @@
+require_relative 'filter/absent_file'
 require_relative 'filter/yaml'
 
 module OctocatalogDiff
@@ -24,14 +25,19 @@ def self.apply_filters(result, filter_names, options = {})
       # @param options [Hash] Additional options (optional) to pass to filtered? method
       def self.filter(result, filter_class_name, options = {})
         filter_class_name = [name.to_s, filter_class_name].join('::')
-        clazz = Kernel.const_get(filter_class_name)
-        result.reject! { |item| clazz.filtered?(item, options) }
+        obj = Kernel.const_get(filter_class_name).new(result)
+        result.reject! { |item| obj.filtered?(item, options) }
+      end
+
+      # Inherited: Constructor that does nothing. Some filters require working on the entire
+      # data set and will override this method to perform some pre-processing for efficiency.
+      def initialize(_diff_array = [])
       end
 
       # Inherited: Construct a default `filtered?` method for the subclass via inheritance.
       # Each subclass must implement this method, so the default method errors.
-      def self.filtered?(_item, _options = {})
-        raise "No `filtered?` method is implemented in #{name}"
+      def filtered?(_item, _options = {})
+        raise "No `filtered?` method is implemented in #{self.class}"
       end
     end
   end

lib/octocatalog-diff/catalog-diff/filter/absent_file.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'set'
+
+module OctocatalogDiff
+  module CatalogDiff
+    class Filter
+      # Filter out changes in parameters when the "to" resource has ensure => absent.
+      class AbsentFile < OctocatalogDiff::CatalogDiff::Filter
+        # Constructor: Since this filter requires knowledge of the entire array of diffs,
+        # override the inherited method to store those diffs in an instance variable.
+        def initialize(diffs)
+          @diffs = diffs
+          @results = nil
+        end
+
+        # Public: If a file has ensure => absent, there are certain parameters that don't
+        # matter anymore. Filter out any such parameters from the result array.
+        # Return true if the difference is in a resource where `ensure => absent` has been
+        # declared. Return false if they this is not the case.
+        #
+        # @param diff [internal diff format] Difference
+        # @param _options [Hash] Additional options (there are none for this filter)
+        # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
+        def filtered?(diff, _options = {})
+          build_results if @results.nil?
+          @results.include?(diff)
+        end
+
+        private
+
+        # Private: The first time `.filtered?` is called, build up the cache of results.
+        # Returns nothing, but populates @results.
+        def build_results
+          # Which files can we ignore?
+          @files_to_ignore = Set.new
+          @diffs.each do |diff|
+            next unless diff[0] == '~' || diff[0] == '!'
+            next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
+            next unless ['absent', 'false', false].include?(diff[3])
+            @files_to_ignore.add Regexp.last_match(1)
+          end
+
+          # Based on that, which diffs can we ignore?
+          @results = Set.new @diffs.reject { |diff| keep_diff?(diff) }
+        end
+
+        # Private: Determine whether to keep a particular diff.
+        # @param diff [OctocatalogDiff::API::V1::Diff] Difference under consideration
+        # @return [Boolean] true = keep, false = discard
+        def keep_diff?(diff)
+          keep = %w(ensure backup force provider)
+          if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
+            if @files_to_ignore.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
+              return false
+            end
+          end
+          true
+        end
+      end
+    end
+  end
+end

lib/octocatalog-diff/catalog-diff/filter/yaml.rb

@@ -14,7 +14,7 @@ class YAML < OctocatalogDiff::CatalogDiff::Filter
         # @param diff [Array] Difference
         # @param _options [Hash] Additional options (there are none for this filter)
         # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
-        def self.filtered?(diff, _options = {})
+        def filtered?(diff, _options = {})
           # Skip additions or removals - focus only on changes
           return false unless diff[0] == '~' || diff[0] == '!'
 

lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb

@@ -3,6 +3,7 @@
 # If enabled, this option will suppress changes to certain attributes of a file, if the
 # file is specified to be 'absent' in the target catalog. Suppressed changes in this case
 # include user, group, mode, and content, because a removed file has none of those.
+# <i>This option is DEPRECATED; please use <code>--filters AbsentFile</code> instead.</i>
 # @param parser [OptionParser object] The OptionParser argument
 # @param options [Hash] Options hash being constructed; this is modified in this method.
 OctocatalogDiff::Cli::Options::Option.newoption(:suppress_absent_file_details) do

spec/octocatalog-diff/integration/file_absent_spec.rb

@@ -22,14 +22,6 @@
       diff = diffs.first
       expect(diff[1..3]).to eq(["File\f/tmp/foo\fparameters\fensure", nil, 'absent'])
     end
-
-    it 'should print the proper debug log messages' do
-      expect(@result[:logs]).to match(/Entering filter_diffs_for_absent_files with 4 diffs/)
-      expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=group for absent file})
-      expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=owner for absent file})
-      expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=mode for absent file})
-      expect(@result[:logs]).to match(/Exiting filter_diffs_for_absent_files with 1 diffs/)
-    end
   end
 
   context 'with file absent suppression disabled' do
@@ -54,9 +46,5 @@
       expect(diffs[2][1..3]).to eq(["File\f/tmp/foo\fparameters\fowner", 'root', 'git'])
       expect(diffs[3][1..3]).to eq(["File\f/tmp/foo\fparameters\fensure", nil, 'absent'])
     end
-
-    it 'should print the proper debug log messages' do
-      expect(@result[:logs]).not_to match(/Entering filter_diffs_for_absent_files/)
-    end
   end
 end

spec/octocatalog-diff/tests/catalog-diff/differ_spec.rb

@@ -1251,35 +1251,4 @@
       expect(result[0]).to eq(['!', "Class\fOpenssl::Package\fparameters\fcommon-array", [1, 2, 3], [1, 5, 25], fileref, fileref])
     end
   end
-
-  describe '#filter_diffs_for_absent_files' do
-    before(:each) do
-      empty_puppet_catalog_json = File.read(OctocatalogDiff::Spec.fixture_path('catalogs/catalog-empty.json'))
-      empty_puppet_catalog = OctocatalogDiff::Catalog.new(json: empty_puppet_catalog_json)
-      logger, @logger_str = OctocatalogDiff::Spec.setup_logger
-      @obj = OctocatalogDiff::CatalogDiff::Differ.new({ logger: logger }, empty_puppet_catalog, empty_puppet_catalog)
-      @orig = [
-        ['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
-        ['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
-        ['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
-        ['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
-        ['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
-        ['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
-        ['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
-        ['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
-      ]
-      @result = @orig.dup
-      @obj.send(:filter_diffs_for_absent_files, @result)
-    end
-
-    it 'should filter out some attributes for ensure=>absent file' do
-      expect(@result).to eq(@orig.values_at(0, 2, 3, 4, 5, 6, 7))
-    end
-
-    it 'should log messages' do
-      expect(@logger_str.string).to match(/Entering filter_diffs_for_absent_files with 8 diffs/)
-      expect(@logger_str.string).to match(%r{Removing file=/tmp/foo parameter=owner for absent file})
-      expect(@logger_str.string).to match(/Exiting filter_diffs_for_absent_files with 7 diffs/)
-    end
-  end
 end

spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require_relative '../../spec_helper'
+require OctocatalogDiff::Spec.require_path('/catalog')
+require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/absent_file')
+
+describe OctocatalogDiff::CatalogDiff::Filter::AbsentFile do
+  it 'should filter out some attributes for ensure=>absent file' do
+    orig = [
+      ['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
+      ['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
+      ['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
+      ['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
+      ['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
+      ['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
+      ['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
+      ['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
+    ]
+    testobj = described_class.new(orig)
+    result = orig.reject { |x| testobj.filtered?(x) }
+    expect(result).to eq(orig.values_at(0, 2, 3, 4, 5, 6, 7))
+  end
+end