Rewrite YAML feature as a more modular filter

Author: Kevin Paulisse

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

@@ -5,9 +5,9 @@
 require 'json'
 require 'set'
 require 'stringio'
-require 'yaml'
 
 require_relative '../catalog'
+require_relative 'filter'
 
 module OctocatalogDiff
   module CatalogDiff
@@ -157,7 +157,9 @@ def catdiff
         # If --ignore-equivalent-yaml-files is specified, then for any YAML file that is changed, parse the YAML
         # to construct the object. If the objects are identical this means the YAML file differs only in whitespace
         # or comments but not in function. Remove these.
-        filter_diffs_for_equivalent_yaml_files(result) if @opts[:ignore_equivalent_yaml_files]
+        if @opts[:ignore_equivalent_yaml_files]
+          OctocatalogDiff::CatalogDiff::Filter.filter(result, 'YAML')
+        end
 
         # That's it!
         @logger.debug "Exiting catdiff; change count: #{result.size}"
@@ -171,33 +173,6 @@ def filter_diffs_for_ignored_items(result)
         result.reject! { |item| ignored?(item) }
       end
 
-      # To ignore YAML file differences that are only due to whitespace or comments (i.e., the YAML files themselves
-      # are functionally equivalent), parse the YAML content from both catalogs and compare objects. Remove when
-      # objects are identical.
-      # @param result [Array] Full result array (modified by this method)
-      def filter_diffs_for_equivalent_yaml_files(result)
-        result.reject! { |item| equivalent_yaml_files?(item) }
-      end
-
-      # Actually do the comparison of YAML objects for appropriate resources.
-      # @param diff [Array] Difference
-      # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
-      def equivalent_yaml_files?(diff)
-        # Skip additions or removals - focus only on changes
-        return false unless diff[0] == '~' || diff[0] == '!'
-
-        # Make sure we are comparing file content for a file ending in .yaml or .yml extension
-        return false unless diff[1] =~ /^File\f([^\f]+)\.ya?ml\fparameters\fcontent$/
-
-        # Attempt to convert the old (diff[2]) and new (diff[3]) into YAML objects. Assuming
-        # that doesn't error out, the return value is whether or not they're equal.
-        obj_old = YAML.load(diff[2])
-        obj_new = YAML.load(diff[3])
-        obj_old == obj_new
-      rescue # Rescue everything - if something failed, we aren't sure what's going on, so we'll return false.
-        false
-      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)

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

@@ -0,0 +1,37 @@
+require_relative 'filter/yaml'
+
+module OctocatalogDiff
+  module CatalogDiff
+    # Filtering of diffs, and parent class for inheritance.
+    class Filter
+      # Public: Apply multiple filters by repeatedly calling the `filter` method for each
+      # filter in an array. This method returns nothing.
+      #
+      # @param result [Array] Difference array (mutated)
+      # @param filter_names [Array] Filters to run
+      # @param options [Hash] Options for each filter (hashed by name)
+      def self.apply_filters(result, filter_names, options = {})
+        filter_names.each { |x| filter(result, x, options[x] || {}) }
+      end
+
+      # Public: Perform a filter on `result` using the specified filter class.
+      # This mutates `result` by removing items that are ignored. This method
+      # returns nothing.
+      #
+      # @param result [Array] Difference array (mutated)
+      # @param filter_class_name [String] Filter class name (from `filter` subdirectory)
+      # @param options [Hash] Additional options (optional) to pass to filtered? method
+      def self.filter(result, filter_class_name, options = {})
+        filter_class_name = [self.class.name.to_s, filter_class_name].join('::')
+        clazz = Kernel.get_const(filter_class_name)
+        result.reject! { |item| clazz.filtered?(item, options) }
+      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 #{self.class.name}"
+      end
+    end
+  end
+end

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

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require 'yaml'
+
+module OctocatalogDiff
+  module CatalogDiff
+    class Filter
+      # Filter based on equivalence of YAML objects for file resources with named extensions.
+      class YAML < OctocatalogDiff::CatalogDiff::Filter
+        # Public: Actually do the comparison of YAML objects for appropriate resources.
+        # Return true if the YAML objects are known to be equivalent. Return false if they
+        # are not equivalent, or if equivalence cannot be determined.
+        #
+        # @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 = {})
+          # Skip additions or removals - focus only on changes
+          return false unless diff[0] == '~' || diff[0] == '!'
+
+          # Make sure we are comparing file content for a file ending in .yaml or .yml extension
+          return false unless diff[1] =~ /^File\f([^\f]+)\.ya?ml\fparameters\fcontent$/
+
+          # Attempt to convert the old (diff[2]) and new (diff[3]) into YAML objects. Assuming
+          # that doesn't error out, the return value is whether or not they're equal.
+          obj_old = ::YAML.load(diff[2])
+          obj_new = ::YAML.load(diff[3])
+          obj_old == obj_new
+        rescue # Rescue everything - if something failed, we aren't sure what's going on, so we'll return false.
+          false
+        end
+      end
+    end
+  end
+end

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

@@ -1281,73 +1281,4 @@
       expect(@logger_str.string).to match(/Exiting filter_diffs_for_absent_files with 7 diffs/)
     end
   end
-
-  describe '#filter_diffs_for_equivalent_yaml_files' do
-    let(:obj) { described_class.allocate }
-    let(:str1a) { "---\n  foo: bar" }
-    let(:str1b) { "---\nfoo: bar" }
-    let(:str2) { "---\n  foo: baz" }
-
-    it 'should not filter out an added resource' do
-      result = [['+', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a removed resource' do
-      result = [['-', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a non-file resource' do
-      result = [['~', "Exec\ffoobar.yaml\fparameters\fcontent", str1a, str1b]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a file whose extension is not .yaml / .yml' do
-      result = [['~', "File\ffoobar.json\fparameters\fcontent", str1a, str1b]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a change with no content change' do
-      result = [['~', "File\ffoobar.json\fparameters\fowner", 'root', 'nobody']]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a change where YAML objects are dissimilar' do
-      result = [['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str2]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a change where YAML is invalid' do
-      x_str = '---{ "blah": "foo" }'
-      result = [['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should not filter out a change where YAML is unparseable' do
-      x_str = "--- !ruby/object:This::Does::Not::Exist\n  foo: bar"
-      result = [['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(1)
-    end
-
-    it 'should filter out a whitespace-only change to a .yaml file' do
-      result = [['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str1b]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(0)
-    end
-
-    it 'should filter out a whitespace-only change to a .yml file' do
-      result = [['~', "File\ffoobar.yml\fparameters\fcontent", str1a, str1b]]
-      obj.send(:filter_diffs_for_equivalent_yaml_files, result)
-      expect(result.size).to eq(0)
-    end
-  end
 end

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

@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require_relative '../../spec_helper'
+require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/yaml')
+
+describe OctocatalogDiff::CatalogDiff::Filter::YAML do
+  describe '#filtered?' do
+    let(:str1a) { "---\n  foo: bar" }
+    let(:str1b) { "---\nfoo: bar" }
+    let(:str2) { "---\n  foo: baz" }
+
+    it 'should not filter out an added resource' do
+      diff = ['+', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a removed resource' do
+      diff = ['-', "File\ffoobar.yaml", { 'parameters' => { 'content' => str1a } }]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a non-file resource' do
+      diff = ['~', "Exec\ffoobar.yaml\fparameters\fcontent", str1a, str1b]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a file whose extension is not .yaml / .yml' do
+      diff = ['~', "File\ffoobar.json\fparameters\fcontent", str1a, str1b]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a change with no content change' do
+      diff = ['~', "File\ffoobar.json\fparameters\fowner", 'root', 'nobody']
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a change where YAML objects are dissimilar' do
+      diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str2]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a change where YAML is invalid' do
+      x_str = '---{ "blah": "foo" }'
+      diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should not filter out a change where YAML is unparseable' do
+      x_str = "--- !ruby/object:This::Does::Not::Exist\n  foo: bar"
+      diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", x_str, x_str]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(false)
+    end
+
+    it 'should filter out a whitespace-only change to a .yaml file' do
+      diff = ['~', "File\ffoobar.yaml\fparameters\fcontent", str1a, str1b]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(true)
+    end
+
+    it 'should filter out a whitespace-only change to a .yml file' do
+      diff = ['~', "File\ffoobar.yml\fparameters\fcontent", str1a, str1b]
+      result = described_class.filtered?(diff)
+      expect(result).to eq(true)
+    end
+  end
+end