diff --git a/lib/octocatalog-diff/catalog-diff/differ.rb b/lib/octocatalog-diff/catalog-diff/differ.rb index 3d959fa0..83be4c6f 100644 --- a/lib/octocatalog-diff/catalog-diff/differ.rb +++ b/lib/octocatalog-diff/catalog-diff/differ.rb @@ -202,7 +202,9 @@ def catdiff filter_opts = { logger: @logger, from_compilation_dir: @catalog1_raw.compilation_dir, - to_compilation_dir: @catalog2_raw.compilation_dir + to_compilation_dir: @catalog2_raw.compilation_dir, + from_resources: @catalog1_raw.resources, + to_resources: @catalog2_raw.resources } OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters], filter_opts) if @opts[:filters].any? diff --git a/lib/octocatalog-diff/catalog-diff/filter/absent_file.rb b/lib/octocatalog-diff/catalog-diff/filter/absent_file.rb index 17c5f845..0c66cd71 100644 --- a/lib/octocatalog-diff/catalog-diff/filter/absent_file.rb +++ b/lib/octocatalog-diff/catalog-diff/filter/absent_file.rb @@ -10,6 +10,7 @@ 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 + ABSENT_VALUES = ['absent', 'false', false].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. @@ -28,8 +29,8 @@ def initialize(diffs, _logger = nil) # @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 = {}) - build_results if @results.nil? + def filtered?(diff, options = {}) + build_results(options) if @results.nil? @results.include?(diff) end @@ -37,25 +38,60 @@ def filtered?(diff, _options = {}) # 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 + def build_results(options) + @files_absent_in_to = Set.new + @files_absent_in_from = Set.new + + if options[:to_resources].is_a?(Array) + options[:to_resources].each do |resource| + next unless resource.is_a?(Hash) && resource['type'] == 'File' + next unless resource.key?('parameters') && resource['parameters'].is_a?(Hash) + next unless ABSENT_VALUES.include?(resource['parameters']['ensure']) + @files_absent_in_to.add file_title_for_resource(resource) + end + end + + if options[:from_resources].is_a?(Array) + options[:from_resources].each do |resource| + next unless resource.is_a?(Hash) && resource['type'] == 'File' + next unless resource.key?('parameters') && resource['parameters'].is_a?(Hash) + next unless ABSENT_VALUES.include?(resource['parameters']['ensure']) + @files_absent_in_from.add file_title_for_resource(resource) + end + end + + # Backward-compatible behavior: if an ensure diff explicitly shows ensure => absent in the new + # catalog, make sure we ignore that file even if resource lists were not provided. @diffs.each do |diff| 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 + next unless ABSENT_VALUES.include?(diff.new_value) + @files_absent_in_to.add diff.title end + @files_absent_in_both = @files_absent_in_to & @files_absent_in_from + # Based on that, which diffs can we ignore? @results = Set.new @diffs.reject { |diff| keep_diff?(diff) } end + def file_title_for_resource(resource) + return resource['title'] unless resource.key?('parameters') && resource['parameters'].is_a?(Hash) + return resource['title'] unless resource['parameters'].key?('path') + + resource['parameters']['path'] + 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) - return true unless diff.change? && diff.type == 'File' && diff.structure.first == 'parameters' - return true unless @files_to_ignore.include?(diff.title) + return true unless diff.change? && diff.type == 'File' + + # If both catalogs declare the same file as absent, suppress all diffs for that file. + return false if @files_absent_in_both.include?(diff.title) + + return true unless diff.structure.first == 'parameters' + return true unless @files_absent_in_to.include?(diff.title) return true if KEEP_ATTRIBUTES.include?(diff.structure.last) false end diff --git a/spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb b/spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb index a019fd49..5ee4db9b 100644 --- a/spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb +++ b/spec/octocatalog-diff/tests/catalog-diff/filter/absent_file_spec.rb @@ -22,4 +22,22 @@ result = obj.reject { |x| testobj.filtered?(x) } expect(result).to eq(obj.values_at(0, 2, 3, 4, 5, 6, 7)) end + + it 'should suppress diffs when file is ensure=>absent in both catalogs' do + orig = [ + ['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'], + ['~', "File\f/tmp/foo\fparameters\fforce", false, true], + ['~', "File\f/tmp/bar\fparameters\fmode", '0644', '0600'] + ] + obj = orig.map { |x| OctocatalogDiff::API::V1::Diff.factory(x) } + testobj = described_class.new(obj) + result = obj.reject do |x| + testobj.filtered?(x, { + from_resources: [{ 'type' => 'File', 'title' => '/tmp/foo', 'parameters' => { 'ensure' => 'absent', 'owner' => 'root', 'force' => false } }], + to_resources: [{ 'type' => 'File', 'title' => '/tmp/foo', 'parameters' => { 'ensure' => 'absent', 'owner' => 'nobody', 'force' => true } }] + }) + end + + expect(result).to eq(obj.values_at(2)) + end end