Merge pull request #69 from github/kpaulisse-misc-fixes

Miscellaneous fixes for 0.99.rc1

Author: kpaulisse

doc/advanced-filter.md

@@ -9,7 +9,7 @@ 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
+- [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

lib/octocatalog-diff/api/v1/catalog-diff.rb

@@ -57,7 +57,7 @@ def self.catalog_diff(options = nil)
 
           # Return diffs and catalogs in expected format
           OpenStruct.new(
-            diffs: diffs.map { |x| OctocatalogDiff::API::V1::Diff.new(x) },
+            diffs: diffs.map { |x| OctocatalogDiff::API::V1::Diff.factory(x) },
             from: OctocatalogDiff::API::V1::Catalog.new(catalogs[:from]),
             to: OctocatalogDiff::API::V1::Catalog.new(catalogs[:to])
           )

lib/octocatalog-diff/api/v1/common.rb

@@ -12,7 +12,8 @@ def self.logger_from_options(options)
           logger = options[:logger] || Logger.new(StringIO.new)
 
           # We can't keep :logger in the options due to marshal/unmarshal as part of parallelization.
-          pass_opts = options.merge(logger: nil)
+          pass_opts = options.dup
+          pass_opts.delete(:logger)
 
           # Return cleaned options and logger
           [pass_opts, logger]

lib/octocatalog-diff/api/v1/diff.rb

@@ -17,20 +17,30 @@ module V1
       # easier to deal with. We recommend using the named options, rather than #raw or the indexed array,
       # as the raw object and indexed array are not guaranteed to be stable.
       class Diff
-        attr_reader :raw
+        attr_reader :raw, :diff_type, :type, :title, :structure
+
+        # Public: Construct a OctocatalogDiff::API::V1::Diff object from many different types of
+        # input. This includes passing a OctocatalogDiff::API::V1::Diff object and getting that
+        # identical object back.
+        # @param object_in [?] Object in
+        # @return [OctocatalogDiff::API::V1::Diff] Object out
+        def self.factory(object_in)
+          return object_in if object_in.is_a?(OctocatalogDiff::API::V1::Diff)
+
+          return new(object_in) if object_in.is_a?(Array)
+
+          raise ArgumentError, "Cannot construct OctocatalogDiff::API::V1::Diff from #{object_in.class}"
+        end
 
         # Constructor: Accepts a diff in the traditional array format and stores it.
         # @param raw [Array] Diff in the traditional format
         def initialize(raw)
-          if raw.is_a?(OctocatalogDiff::API::V1::Diff)
-            @raw = raw.raw
-            return
-          end
-
           unless raw.is_a?(Array)
             raise ArgumentError, "OctocatalogDiff::API::V1::Diff#initialize expects Array argument (got #{raw.class})"
           end
+
           @raw = raw
+          initialize_helper
         end
 
         # Public: Retrieve an indexed value from the array
@@ -39,12 +49,6 @@ def [](i)
           @raw[i]
         end
 
-        # Public: Get the change type
-        # @return [String] Change type symbol (~, !, +, -)
-        def diff_type
-          @raw[0]
-        end
-
         # Public: Is this an addition?
         # @return [Boolean] True if this is an addition
         def addition?
@@ -63,35 +67,17 @@ def change?
           diff_type == '~' || diff_type == '!'
         end
 
-        # Public: Get the resource type
-        # @return [String] Resource type
-        def type
-          @raw[1].split(/\f/)[0]
-        end
-
-        # Public: Get the resource title
-        # @return [String] Resource title
-        def title
-          @raw[1].split(/\f/)[1]
-        end
-
-        # Public: Get the structure of the resource as an array
-        # @return [Array] Structure of resource
-        def structure
-          @raw[1].split(/\f/)[2..-1]
-        end
-
         # Public: Get the "old" value, i.e. "from" catalog
         # @return [?] "old" value
         def old_value
-          return nil if addition?
+          return if addition?
           @raw[2]
         end
 
         # Public: Get the "new" value, i.e. "to" catalog
-        # @return [?] "old" value
+        # @return [?] "new" value
         def new_value
-          return nil if removal?
+          return if removal?
           return @raw[2] if addition?
           @raw[3]
         end
@@ -127,7 +113,7 @@ def new_line
         # Public: Get the "old" location, i.e. location in the "from" catalog
         # @return [Hash] <file:, line:> of resource
         def old_location
-          return nil if addition?
+          return if addition?
           return @raw[3] if removal?
           @raw[4]
         end
@@ -136,7 +122,7 @@ def old_location
         # @return [Hash] <file:, line:> of resource
         def new_location
           return @raw[3] if addition?
-          return nil if removal?
+          return if removal?
           @raw[5]
         end
 
@@ -178,6 +164,24 @@ def inspect
         def to_s
           raw.inspect
         end
+
+        private
+
+        # Private: Initialize further instance variables
+        def initialize_helper
+          unless ['+', '-', '~', '!'].include?(@raw[0])
+            raise ArgumentError, 'Invalid first element array: diff type needs to be one of: +, -, ~, !'
+          end
+          @diff_type = @raw[0]
+
+          unless @raw[1].is_a?(String)
+            raise ArgumentError, "Invalid second element array: type-title-structure needs to be a string not #{@raw[1].class}"
+          end
+          raw_1_split = @raw[1].split(/\f/)
+          @type = raw_1_split[0]
+          @title = raw_1_split[1]
+          @structure = raw_1_split[2..-1]
+        end
       end
     end
   end

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

@@ -191,10 +191,8 @@ def catdiff
 
         # Legacy options which are now filters
         @opts[:filters] ||= []
-        if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('AbsentFile')
-          @opts[:filters] << 'AbsentFile'
-        end
-        @opts[:filters] << 'CompilationDir' unless @opts[:filters].include?('CompilationDir')
+        add_element_to_array(@opts[:filters], 'CompilationDir')
+        add_element_to_array(@opts[:filters], 'AbsentFile') if @opts[:suppress_absent_file_details]
 
         # Apply any additional pluggable filters.
         filter_opts = {
@@ -209,6 +207,13 @@ def catdiff
         result
       end
 
+      # Add an element to an array if it doesn't already exist in that array
+      # @param array_in [Array] Array to have element added (**mutated** by this method)
+      # @param element [?] Element to add
+      def add_element_to_array(array_in, element)
+        array_in << element unless array_in.include?(element)
+      end
+
       # Filter the differences for any items that were ignored, by some combination of type, title, and
       # attribute. This modifies the array itself by selecting only items that do not meet the ignored
       # filter.

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

@@ -25,7 +25,7 @@ class Display
       def self.output(diff_in, options = {}, logger = nil)
         diff_x = diff_in.is_a?(OctocatalogDiff::CatalogDiff::Differ) ? diff_in.diff : diff_in
         raise ArgumentError, "text_output requires Array<Diff results>; passed in #{diff_in.class}" unless diff_x.is_a?(Array)
-        diff = diff_x.map { |x| OctocatalogDiff::API::V1::Diff.new(x) }
+        diff = diff_x.map { |x| OctocatalogDiff::API::V1::Diff.factory(x) }
 
         # req_format means 'requested format' because 'format' has a built-in meaning to Ruby
         req_format = options.fetch(:format, :color_text)

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

@@ -1,3 +1,4 @@
+require_relative '../api/v1/diff'
 require_relative 'filter/absent_file'
 require_relative 'filter/compilation_dir'
 require_relative 'filter/yaml'
@@ -49,8 +50,14 @@ def self.apply_filters(result, filter_names, options = {})
       def self.filter(result, filter_class_name, options = {})
         assert_that_filter_exists(filter_class_name)
         filter_class_name = [name.to_s, filter_class_name].join('::')
-        obj = Kernel.const_get(filter_class_name).new(result, options[:logger])
-        result.reject! { |item| obj.filtered?(item, options) }
+
+        # Need to convert each of the results array to the OctocatalogDiff::API::V1::Diff object, if
+        # it isn't already. The comparison is done on that array which is then applied back to the
+        # original array.
+        result_hash = {}
+        result.each { |x| result_hash[x] = OctocatalogDiff::API::V1::Diff.factory(x) }
+        obj = Kernel.const_get(filter_class_name).new(result_hash.values, options[:logger])
+        result.reject! { |item| obj.filtered?(result_hash[item], options) }
       end
 
       # Inherited: Constructor. Some filters require working on the entire data set and

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

@@ -1,14 +1,20 @@
 # frozen_string_literal: true
 
+require_relative '../filter'
+
 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
+        KEEP_ATTRIBUTES = (Set.new %w(ensure backup force provider)).freeze
+
         # Constructor: Since this filter requires knowledge of the entire array of diffs,
         # override the inherited method to store those diffs in an instance variable.
+        # @param diffs [Array<OctocatalogDiff::API::V1::Diff>] Difference array
+        # @param _logger [?] Ignored
         def initialize(diffs, _logger = nil)
           @diffs = diffs
           @results = nil
@@ -19,7 +25,7 @@ def initialize(diffs, _logger = nil)
         # 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 diff [OctocatalogDiff::API::V1::Diff] 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 = {})
@@ -35,10 +41,9 @@ 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)
+            next unless diff.change? && diff.type == 'File' && diff.structure == %w(parameters ensure)
+            next unless ['absent', 'false', false].include?(diff.new_value)
+            @files_to_ignore.add diff.title
           end
 
           # Based on that, which diffs can we ignore?
@@ -49,13 +54,10 @@ def build_results
         # @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
+          return true unless diff.change? && diff.type == 'File' && diff.structure.first == 'parameters'
+          return true unless @files_to_ignore.include?(diff.title)
+          return true if KEEP_ATTRIBUTES.include?(diff.structure.last)
+          false
         end
       end
     end

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

@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require_relative '../../api/v1/diff'
 require_relative '../filter'
 
 module OctocatalogDiff
@@ -13,19 +12,18 @@ class CompilationDir < OctocatalogDiff::CatalogDiff::Filter
         # (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 diff [OctocatalogDiff::API::V1::Diff] 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 = {})
+        def filtered?(diff, 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)
 
           # Check for added/removed resources where the title of the resource includes the compilation directory
           if (diff.addition? || diff.removal?) && diff.title.match(dir)

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

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative '../filter'
+
 require 'yaml'
 
 module OctocatalogDiff
@@ -11,20 +13,21 @@ class YAML < OctocatalogDiff::CatalogDiff::Filter
         # 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 diff_in [OctocatalogDiff::API::V1::Diff] 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 = {})
           # Skip additions or removals - focus only on changes
-          return false unless diff[0] == '~' || diff[0] == '!'
+          return false unless diff.change?
 
           # 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$/
+          return false unless diff.type == 'File' && diff.structure == %w(parameters content)
+          return false unless diff.title =~ /\.ya?ml\z/
 
-          # Attempt to convert the old (diff[2]) and new (diff[3]) into YAML objects. Assuming
+          # Attempt to convert the old value and new value 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 = ::YAML.load(diff.old_value)
+          obj_new = ::YAML.load(diff.new_value)
           obj_old == obj_new
         rescue # Rescue everything - if something failed, we aren't sure what's going on, so we'll return false.
           false