diff --git a/doc/dev/api/v1/objects/diff.md b/doc/dev/api/v1/objects/diff.md index ae34a7e6..b06dfdb0 100644 --- a/doc/dev/api/v1/objects/diff.md +++ b/doc/dev/api/v1/objects/diff.md @@ -94,9 +94,9 @@ Returns the value of the resource from the new catalog. } } - # Demonstrates structure and old_value + # Demonstrates structure and new_value diff.structure #=> ['parameters', 'content'] - diff.old_value #=> 'This is the NEW FILE!!!!!' + diff.new_value #=> 'This is the NEW FILE!!!!!' ``` #### `#old_file` (String) @@ -107,7 +107,7 @@ Note that this is a pass-through of information provided in the Puppet catalog, Note also that if the diff represents addition of a resource, this will return `nil`, because the resource does not exist in the old catalog. -#### `#old_file` (String) +#### `#old_line` (String) Returns the line number within the Puppet manifest giving rise to the resource as it exists in the old catalog. (See `#old_file` for the filename of the Puppet manifest.) diff --git a/lib/octocatalog-diff/catalog-diff/filter/compilation_dir.rb b/lib/octocatalog-diff/catalog-diff/filter/compilation_dir.rb index 442fa700..5e2747dd 100644 --- a/lib/octocatalog-diff/catalog-diff/filter/compilation_dir.rb +++ b/lib/octocatalog-diff/catalog-diff/filter/compilation_dir.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative '../filter' +require_relative '../../util/util' module OctocatalogDiff module CatalogDiff @@ -35,43 +36,46 @@ def filtered?(diff, options = {}) # 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.change? + o = remove_compilation_dir(diff.old_value, dir2) + n = remove_compilation_dir(diff.new_value, dir1) - 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 + if o != diff.old_value || n != diff.new_value message = "Resource key #{diff.type}[#{diff.title}] #{diff.structure.join(' => ')}" - message += ' appears to depend on catalog compilation directory. Suppressed from results.' + 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 - return true end - if from_match || to_match + if o == n 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.' + message += ' appears to depend on catalog compilation directory. Suppressed from results.' @logger.warn message + return true end end false end + + def remove_compilation_dir(v, dir) + value = OctocatalogDiff::Util::Util.deep_dup(v) + traverse(value) do |e| + e.gsub!(dir, '') if e.respond_to?(:gsub!) + end + value + end + + def traverse(a) + case a + when Array + a.map { |v| traverse(v, &Proc.new) } + when Hash + traverse(a.values, &Proc.new) + else + yield a + end + end end end end diff --git a/script/git-pre-commit b/script/git-pre-commit index 64c997fd..2d3db5ba 100755 --- a/script/git-pre-commit +++ b/script/git-pre-commit @@ -4,8 +4,10 @@ # base directory is up two levels, not just one. DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && cd ../.. && pwd )" +cd "$DIR" + # Make sure we can use git correctly -cd "$DIR" && git rev-parse --verify HEAD >/dev/null 2>&1 +git rev-parse --verify HEAD >/dev/null 2>&1 if [ $? -ne 0 ]; then echo "Unable to determine revision of this git repo" exit 1 diff --git a/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-1.json b/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-1.json index 01178024..9d0ba818 100644 --- a/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-1.json +++ b/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-1.json @@ -53,6 +53,23 @@ "parameters": { "dir": "/path/to/catalog1/onetime" } + }, + { + "type": "Varies_Due_To_Compilation_Dir_5", + "title": "Common Title", + "tags": [ + "ignoreme" + ], + "exported": false, + "parameters": { + "dir": { + "component": [ + "path", + "/path/to/catalog1/twotimes", + "otherpath" + ] + } + } } ], "classes": [ diff --git a/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-2.json b/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-2.json index f510384b..9790d04a 100644 --- a/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-2.json +++ b/spec/octocatalog-diff/fixtures/catalogs/compilation-dir-2.json @@ -53,6 +53,19 @@ "parameters": { "dir": "/path/to/catalog2/twotimes" } + }, + { + "type": "Varies_Due_To_Compilation_Dir_5", + "title": "Common Title", + "tags": [ + "ignoreme" + ], + "exported": false, + "parameters": { + "dir": { + "component": [ "path", "/path/to/catalog2/twotimes", "otherpath" ] + } + } } ], "classes": [ diff --git a/spec/octocatalog-diff/tests/catalog-diff/filter/compilation_dir_spec.rb b/spec/octocatalog-diff/tests/catalog-diff/filter/compilation_dir_spec.rb index 3180b372..28b09b07 100644 --- a/spec/octocatalog-diff/tests/catalog-diff/filter/compilation_dir_spec.rb +++ b/spec/octocatalog-diff/tests/catalog-diff/filter/compilation_dir_spec.rb @@ -165,6 +165,19 @@ diff_obj = OctocatalogDiff::API::V1::Diff.factory(diff) expect(subject.filtered?(diff_obj, opts)).to eq(false) end + + it 'should remove a change where directories appear more than one time' do + diff = [ + '~', + "Varies_Due_To_Compilation_Dir_3\fCommon Title\fparameters\fdir", + 'command -a /path/to/catalog1/something -b common-stuff -a /path/to/catalog1/otherthing', + 'command -a /path/to/catalog2/something -b common-stuff -a /path/to/catalog2/otherthing', + { 'file' => nil, 'line' => nil }, + { 'file' => nil, 'line' => nil } + ] + diff_obj = OctocatalogDiff::API::V1::Diff.factory(diff) + expect(subject.filtered?(diff_obj, opts)).to eq(true) + end end context '~ partial indeterminate matches' do @@ -190,4 +203,40 @@ expect(@logger_str.string).to match(/WARN.*Varies_Due_To_Compilation_Dir_3\[Common Title\] parameters => dir.+differences/) end end + + context '~ array value changes' do + let(:diff) do + [ + '~', + "Varies_Due_To_Compilation_Dir_3\fCommon Title\fparameters\fdir", + ['something that doesn\'t change', '/path/to/catalog1', 'something else'], + ['something that doesn\'t change', '/path/to/catalog2', 'something else'], + { 'file' => nil, 'line' => nil }, + { 'file' => nil, 'line' => nil } + ] + end + + it 'should remove a change where directories are a full match' do + diff_obj = OctocatalogDiff::API::V1::Diff.factory(diff) + expect(subject.filtered?(diff_obj, opts)).to eq(true) + end + end + + context '~ nested hash changes' do + let(:diff) do + [ + '~', + "Varies_Due_To_Compilation_Dir_3\fCommon Title\fparameters\fdir", + { 'value' => { 'path' => ['thing', '/path/to/catalog1/file', 'otherthing'] } }, + { 'value' => { 'path' => ['thing', '/path/to/catalog2/file', 'otherthing'] } }, + { 'file' => nil, 'line' => nil }, + { 'file' => nil, 'line' => nil } + ] + end + + it 'should remove a change where directories are a full match' do + diff_obj = OctocatalogDiff::API::V1::Diff.factory(diff) + expect(subject.filtered?(diff_obj, opts)).to eq(true) + end + end end