Merge remote-tracking branch 'masterzen/bugfix/fix-compilatior-dir-arrays' into 1-5-4

Author: kpaulisse

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.)
 

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

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

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": [

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": [

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