Move compilation directory logic into a filter

Author: Kevin Paulisse

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

@@ -60,6 +60,8 @@ class Differ
       # @param catalog1_in [OctocatalogDiff::Catalog] First catalog to compare
       # @param catalog2_in [OctocatalogDiff::Catalog] Second catalog to compare
       def initialize(opts, catalog1_in, catalog2_in)
+        @catalog1_raw = catalog1_in
+        @catalog2_raw = catalog2_in
         @catalog1 = catalog_resources(catalog1_in, 'First catalog')
         @catalog2 = catalog_resources(catalog2_in, 'Second catalog')
         @logger = opts.fetch(:logger, Logger.new(StringIO.new))
@@ -149,12 +151,18 @@ def catdiff
 
         # Legacy options which are now filters
         @opts[:filters] ||= []
-        if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('absent_file')
+        if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('AbsentFile')
           @opts[:filters] << 'AbsentFile'
         end
+        @opts[:filters] << 'CompilationDir' unless @opts[:filters].include?('CompilationDir')
 
         # Apply any additional pluggable filters.
-        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters]) if @opts[:filters].any?
+        filter_opts = {
+          logger: @logger,
+          from_compilation_dir: @catalog1_raw.compilation_dir,
+          to_compilation_dir: @catalog2_raw.compilation_dir
+        }
+        OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters], filter_opts) if @opts[:filters].any?
 
         # That's it!
         @logger.debug "Exiting catdiff; change count: #{result.size}"

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

@@ -1,19 +1,24 @@
 require_relative 'filter/absent_file'
+require_relative 'filter/compilation_dir'
 require_relative 'filter/yaml'
 
+require 'stringio'
+
 module OctocatalogDiff
   module CatalogDiff
     # Filtering of diffs, and parent class for inheritance.
     class Filter
+      attr_accessor :logger
+
       # 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)
+      # @param options [Hash] Options for each filter
       def self.apply_filters(result, filter_names, options = {})
         return unless filter_names.is_a?(Array)
-        filter_names.each { |x| filter(result, x, options[x] || {}) }
+        filter_names.each { |x| filter(result, x, options || {}) }
       end
 
       # Public: Perform a filter on `result` using the specified filter class.
@@ -25,13 +30,15 @@ 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('::')
-        obj = Kernel.const_get(filter_class_name).new(result)
+        obj = Kernel.const_get(filter_class_name).new(result, options[:logger])
         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 = [])
+      # Inherited: Constructor. Some filters require working on the entire data set and
+      # will override this method to perform some pre-processing for efficiency. This also
+      # sets up the logger object.
+      def initialize(_diff_array = [], logger = Logger.new(StringIO.new))
+        @logger = logger
       end
 
       # Inherited: Construct a default `filtered?` method for the subclass via inheritance.

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

@@ -9,7 +9,7 @@ class Filter
       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)
+        def initialize(diffs, _logger = nil)
           @diffs = diffs
           @results = nil
         end

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

@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+require_relative '../../api/v1/diff'
+
+module OctocatalogDiff
+  module CatalogDiff
+    class Filter
+      # Filter out changes that are due to the catalog compilation directory.
+      class CompilationDir < OctocatalogDiff::CatalogDiff::Filter
+        # Public: Filter the diff if the change is due to the catalog compilation directory.
+        # Determine this by obtaining the compiilation directory from each of the catalogs
+        # (supplied via options) and checking the differences. If the only thing different
+        # is the compilation directory, filter it out with a warning.
+        #
+        # @param diff_in [Array (Internal Diff Format)] Difference
+        # @param options [Hash] Additional options:
+        #   :from_compilation_dir [String] Compilation directory for the "from" catalog
+        #   :to_compilation_dir [String] Compilation directory for the "to" catalog
+        # @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
+        def filtered?(diff_in, options = {})
+          return false unless options[:from_compilation_dir] && options[:to_compilation_dir]
+          dir1 = options[:to_compilation_dir]
+          dir1_rexp = Regexp.escape(dir1)
+          dir2 = options[:from_compilation_dir]
+          dir2_rexp = Regexp.escape(dir2)
+          dir = Regexp.new("(?:#{dir1_rexp}|#{dir2_rexp})")
+          diff = OctocatalogDiff::API::V1::Diff.new(diff_in)
+          #          raise "Called on #{diff.inspect} with #{options.inspect}"
+
+          # Check for added/removed resources where the title of the resource includes the compilation directory
+          if (diff.addition? || diff.removal?) && diff.title.match(dir)
+            message = "Resource #{diff.type}[#{diff.title}]"
+            message += ' appears to depend on catalog compilation directory. Suppressed from results.'
+            logger.warn message
+            return true
+          end
+
+          # Check for a change where the difference in a parameter exactly corresponds to the difference in the
+          # compilation directory.
+          if diff.change? && (diff.old_value.is_a?(String) || diff.new_value.is_a?(String))
+            from_before = nil
+            from_after = nil
+            from_match = false
+            to_before = nil
+            to_after = nil
+            to_match = false
+
+            if diff.old_value =~ /^(.*)#{dir2}(.*)$/m
+              from_before = Regexp.last_match(1) || ''
+              from_after = Regexp.last_match(2) || ''
+              from_match = true
+            end
+
+            if diff.new_value =~ /^(.*)#{dir1}(.*)$/m
+              to_before = Regexp.last_match(1) || ''
+              to_after = Regexp.last_match(2) || ''
+              to_match = true
+            end
+
+            if from_match && to_match && to_before == from_before && to_after == from_after
+              message = "Resource key #{diff.type}[#{diff.title}] #{diff.structure.join(' => ')}"
+              message += ' appears to depend on catalog compilation directory. Suppressed from results.'
+              @logger.warn message
+              return true
+            end
+
+            if from_match || to_match
+              message = "Resource key #{diff.type}[#{diff.title}] #{diff.structure.join(' => ')}"
+              message += ' may depend on catalog compilation directory, but there may be differences.'
+              message += ' This is included in results for now, but please verify.'
+              @logger.warn message
+            end
+          end
+
+          false
+        end
+      end
+    end
+  end
+end

lib/octocatalog-diff/cli/diffs.rb

@@ -55,76 +55,10 @@ def diffs(catalogs)
 
         # Actually perform the diff
         diff_result = differ.diff
-        diff_result.delete_if { |element| change_due_to_compilation_dir?(element, catalogs) }
         @logger.debug 'Success compute diffs between catalogs'
         diff_result
       end
 
-      # Catch anything that explictly changed as a result of different compilation directories and
-      # warn about it. These are probably things that should be refactored. For now we're going to pull
-      # these out after the fact so we can warn about them if they do show up.
-      # @param change [Array(diff format)] Change in diff format
-      # @param catalogs [Hash] { :to => OctocatalogDiff::Catalog, :from => OctocatalogDiff::Catalog }
-      # @return [Boolean] True if change includes compilation directory, false otherwise
-      def change_due_to_compilation_dir?(change, catalogs)
-        dir1 = catalogs.fetch(:to).compilation_dir
-        dir2 = catalogs.fetch(:from).compilation_dir
-        return false if dir1.nil? || dir2.nil?
-
-        dir1_rexp = Regexp.escape(dir1)
-        dir2_rexp = Regexp.escape(dir2)
-        dir = "(?:#{dir1_rexp}|#{dir2_rexp})"
-
-        # Check for added/removed resources where the title of the resource includes the compilation directory
-        if change[0] == '+' || change[0] == '-'
-          if change[1] =~ /^([^\f]+)\f([^\f]*#{dir}[^\f]*)/
-            message = "Resource #{Regexp.last_match(1)}[#{Regexp.last_match(2)}]"
-            message += ' appears to depend on catalog compilation directory. Suppressed from results.'
-            @logger.warn message
-            return true
-          end
-        end
-
-        # Check for a change where the difference in a parameter exactly corresponds to the difference in the
-        # compilation directory.
-        if change[0] == '~' || change[0] == '!'
-          from_before = nil
-          from_after = nil
-          from_match = false
-          to_before = nil
-          to_after = nil
-          to_match = false
-
-          if change[2] =~ /^(.*)#{dir2}(.*)$/m
-            from_before = Regexp.last_match(1) || ''
-            from_after = Regexp.last_match(2) || ''
-            from_match = true
-          end
-
-          if change[3] =~ /^(.*)#{dir1}(.*)$/m
-            to_before = Regexp.last_match(1) || ''
-            to_after = Regexp.last_match(2) || ''
-            to_match = true
-          end
-
-          if from_match && to_match && to_before == from_before && to_after == from_after
-            message = "Resource key #{change[1].gsub(/\f/, ' => ')}"
-            message += ' appears to depend on catalog compilation directory. Suppressed from results.'
-            @logger.warn message
-            return true
-          end
-
-          if from_match || to_match
-            message = "Resource key #{change[1].gsub(/\f/, ' => ')}"
-            message += ' may depend on catalog compilation directory, but there may be differences.'
-            message += ' This is included in results for now, but please verify.'
-            @logger.warn message
-          end
-        end
-
-        false
-      end
-
       private
 
       # Determine if a resource is tagged with any ignore-tag.

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

@@ -0,0 +1,111 @@
+# frozen_string_literal: true
+
+require_relative '../../spec_helper'
+require OctocatalogDiff::Spec.require_path('/catalog')
+require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/compilation_dir')
+require OctocatalogDiff::Spec.require_path('/cli/diffs')
+
+describe OctocatalogDiff::CatalogDiff::Filter::CompilationDir do
+  before(:all) do
+    @cat_compilation_dir_1 = OctocatalogDiff::Catalog.new(
+      node: 'my.rspec.node',
+      basedir: '/path/to/catalog1',
+      json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/compilation-dir-1.json'))
+    )
+    @cat_compilation_dir_2 = OctocatalogDiff::Catalog.new(
+      node: 'my.rspec.node',
+      basedir: '/path/to/catalog2',
+      json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/compilation-dir-2.json'))
+    )
+  end
+
+  before(:each) do
+    @logger, @logger_str = OctocatalogDiff::Spec.setup_logger
+  end
+
+  it 'should remove +/- full title changes due to compilation dirs' do
+    opts = {
+      ignore: [
+        { type: 'Varies_Due_To_Compilation_Dir_2' },
+        { type: 'Varies_Due_To_Compilation_Dir_3' },
+        { type: 'Varies_Due_To_Compilation_Dir_4' }
+      ]
+    }
+    testobj = OctocatalogDiff::Cli::Diffs.new(opts, @logger)
+    result = testobj.diffs(from: @cat_compilation_dir_1, to: @cat_compilation_dir_2)
+    expect(result).to eq([])
+    expect(@logger_str.string).to match(%r{Varies_Due_To_Compilation_Dir_1\[/path/to/catalog2\] .*Suppressed})
+    expect(@logger_str.string).to match(%r{Varies_Due_To_Compilation_Dir_1\[/path/to/catalog1\] .*Suppressed})
+  end
+
+  it 'should remove +/- partial title changes due to compilation dirs' do
+    opts = { ignore:
+      [
+        { type: 'Varies_Due_To_Compilation_Dir_1' },
+        { type: 'Varies_Due_To_Compilation_Dir_3' },
+        { type: 'Varies_Due_To_Compilation_Dir_4' }
+      ] }
+    testobj = OctocatalogDiff::Cli::Diffs.new(opts, @logger)
+    result = testobj.diffs(from: @cat_compilation_dir_1, to: @cat_compilation_dir_2)
+    expect(result).to eq([])
+    r1 = %r{Varies_Due_To_Compilation_Dir_2\[/aldsfjalkfjalksfd/path/to/catalog2/dflkjasfkljasdf\].*Suppressed}
+    expect(@logger_str.string).to match(r1)
+    r2 = %r{Varies_Due_To_Compilation_Dir_2\[/aldsfjalkfjalksfd/path/to/catalog1/dflkjasfkljasdf\].*Suppressed}
+    expect(@logger_str.string).to match(r2)
+  end
+
+  it 'should remove ~ changes due to compilation dirs' do
+    opts = { ignore:
+      [
+        { type: 'Varies_Due_To_Compilation_Dir_1' },
+        { type: 'Varies_Due_To_Compilation_Dir_2' },
+        { type: 'Varies_Due_To_Compilation_Dir_4' },
+        { attr: "parameters\fdir_in_first_cat" },
+        { attr: "parameters\fdir_in_second_cat" }
+      ] }
+    testobj = OctocatalogDiff::Cli::Diffs.new(opts, @logger)
+    result = testobj.diffs(from: @cat_compilation_dir_1, to: @cat_compilation_dir_2)
+    expect(result).to eq([])
+    expect(@logger_str.string).to match(/WARN -- : Resource key.*parameters => dir .*Suppressed/)
+    expect(@logger_str.string).to match(/WARN -- : Resource key.*parameters => dir_in_middle .*Suppressed/)
+  end
+
+  it 'should warn but not remove non-matching ! changes due to compilation dirs' do
+    opts = { ignore:
+      [
+        { type: 'Varies_Due_To_Compilation_Dir_1' },
+        { type: 'Varies_Due_To_Compilation_Dir_2' },
+        { type: 'Varies_Due_To_Compilation_Dir_4' },
+        { attr: "parameters\fdir" },
+        { attr: "parameters\fdir_in_middle" }
+      ] }
+    testobj = OctocatalogDiff::Cli::Diffs.new(opts, @logger)
+    result = testobj.diffs(from: @cat_compilation_dir_1, to: @cat_compilation_dir_2)
+    expect(result.size).to eq(2)
+    common_str = "Varies_Due_To_Compilation_Dir_3\fCommon Title\fparameters\f"
+    expect(result[0][1]).to eq("#{common_str}dir_in_first_cat")
+    expect(result[1][1]).to eq("#{common_str}dir_in_second_cat")
+    expect(@logger_str.string).to match(/WARN.+Varies_Due_To_Compilation_Dir_3\[Common Title\] parameters => dir.*verify/)
+  end
+
+  it 'should warn but not remove non-matching ~ changes due to compilation dirs' do
+    opts = { ignore:
+      [
+        { type: 'Varies_Due_To_Compilation_Dir_1' },
+        { type: 'Varies_Due_To_Compilation_Dir_2' },
+        { type: 'Varies_Due_To_Compilation_Dir_3' }
+      ] }
+    testobj = OctocatalogDiff::Cli::Diffs.new(opts, @logger)
+    result = testobj.diffs(from: @cat_compilation_dir_1, to: @cat_compilation_dir_2)
+    answer = [[
+      '~',
+      "Varies_Due_To_Compilation_Dir_4\fCommon Title\fparameters\fdir",
+      '/path/to/catalog1/onetime',
+      '/path/to/catalog2/twotimes',
+      { 'file' => nil, 'line' => nil },
+      { 'file' => nil, 'line' => nil }
+    ]]
+    expect(result).to eq(answer)
+    expect(@logger_str.string).to match(/WARN -- : Resource key Varies_Due_To_Compilation_Dir_4.*please verify/)
+  end
+end

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

@@ -22,10 +22,10 @@ class OctocatalogDiff::CatalogDiff::Filter::Fake2 < OctocatalogDiff::CatalogDiff
   describe '#apply_filters' do
     it 'should call self.filter() with appropriate options for each class' do
       result = [false]
-      options = { 'Fake1' => { foo: 'bar' } }
+      options = { foo: 'bar' }
       classes = %w(Fake1 Fake2)
       expect_any_instance_of(@class_1).to receive(:'filtered?').with(false, foo: 'bar').and_return(false)
-      expect_any_instance_of(@class_2).to receive(:'filtered?').with(false, {}).and_return(false)
+      expect_any_instance_of(@class_2).to receive(:'filtered?').with(false, foo: 'bar').and_return(false)
       expect { described_class.apply_filters(result, classes, options) }.not_to raise_error
       expect(result).to eq([false])
     end