Merge pull request #184 from github/kpaulisse-regexp-attributes

Better regular expression attribute handling

Author: kpaulisse

.travis.yml

@@ -3,6 +3,7 @@
 language: ruby
 install: true
 script: "script/cibuild"
+before_install: gem install bundler
 
 matrix:
   include:

doc/dev/api/v1/calls/catalog-diff.md

@@ -167,12 +167,16 @@ In this case, "owner", "notify", and "content" are nested under "parameters". In
 
   ```
   # Ignore all changes to the `owner` attribute of a file.
-  [ { type: Regexp.new('\AFile\z'), attr: Regexp.new("\Aparameters\fowner\z" } ]
+  [ { type: Regexp.new('\AFile\z'), attr: Regexp.new("\\Aparameters\fowner\\z" } ]
 
   # Ignore changes to `owner` or `group` for a file or an exec.
-  [ { type: Regexp.new('\A(File|Exec)\z'), attr: Regexp.new("\Aparameters\f(owner|group)\z" } ]
+  [ { type: Regexp.new('\A(File|Exec)\z'), attr: Regexp.new("\\Aparameters\f(owner|group)\\z" } ]
   ```
 
+When using regular expressions, `\f` (form feed character) is used to separate the structure (e.g. `parameters\fowner` refers to the `parameters` hash, `owner` key).
+
+:bulb: Note that `\A` in Ruby matches the beginning of the string and `\z` matches the end, but these are not actual characters. Therefore, if you are using `\A` or `\z` in double quotes (`"`), be sure to heed the examples above and write your expression like: `Regexp.new("\\Aparameters\fowner\\z")`.
+
 #### `:validate_references` (Array<String>, Optional)
 
 Invoke the [catalog validation](/doc/advanced-catalog-validation.md) feature to ensure resources targeted by `before`, `notify`, `require`, and/or `subscribe` exist in the catalog. If this parameter is not defined, no reference validation occurs.

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

@@ -294,10 +294,13 @@ def regexp_operator_match?(operator, regex, old_val, new_val)
         # Use diffy to get only the lines that have changed in a text object.
         # As we iterate through the diff, jump out if we have our answer: either
         # true if '=~>' finds ANY match, or false if '=&>' fails to find a match.
-        Diffy::Diff.new(old_val, new_val, context: 0).each do |line|
+        diffy_result = Diffy::Diff.new(old_val, new_val, context: 0)
+        newline_alerts = diffy_result.count { |line| line.strip == '\\ No newline at end of file' }
+        diffy_result.each do |line|
           if regex.match(line.strip)
             return true if operator == '=~>'
           elsif operator == '=&>'
+            next if line.strip == '\\ No newline at end of file' && newline_alerts == 2
             return false
           end
         end
@@ -394,6 +397,13 @@ def ignore_match?(rule_in, diff_type, hsh, old_val, new_val)
           return false unless rule[:title].casecmp(hsh[:title]).zero?
         end
 
+        # If rule[:attr] is a regular expression, handle that case here.
+        if rule[:attr].is_a?(Regexp)
+          return false unless hsh[:attr].is_a?(String)
+          return false unless rule[:attr].match(hsh[:attr])
+          return ignore_match_true(hsh, rule)
+        end
+
         # Special 'attributes': Ignore specific diff types (+ add, - remove, ~ and ! change)
         if rule[:attr] =~ /\A[\-\+~!]+\Z/
           return ignore_match_true(hsh, rule) if rule[:attr].include?(diff_type)

spec/octocatalog-diff/integration/ignore_regexp_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require_relative 'integration_helper'
+
+describe 'ignore regexp integration' do
+  before(:all) do
+    @result = OctocatalogDiff::API::V1.catalog_diff(
+      to_catalog: OctocatalogDiff::Spec.fixture_path('catalogs/default-catalog-changed.json'),
+      from_catalog: OctocatalogDiff::Spec.fixture_path('catalogs/default-catalog-v4.json'),
+      ignore: [
+        {
+          type: Regexp.new('\ASsh_authorized_key\z'),
+          attr: Regexp.new("\\Aparameters\f(foo|bar)\\z")
+        }
+      ]
+    )
+  end
+
+  it 'should succeed' do
+    expect(@result.diffs).to be_a_kind_of(Array)
+  end
+
+  it 'should contain a non-ignored diff in another type' do
+    lookup = { diff_type: '+', type: 'Group', title: 'bill' }
+    expect(OctocatalogDiff::Spec.diff_match?(@result.diffs, lookup)).to eq(true), @result.diffs.inspect
+  end
+
+  it 'should contain a non-ignored removal in the same type' do
+    lookup = { type: 'Ssh_authorized_key', title: 'root@6def27049c06f48eea8b8f37329f40799d07dc84' }
+    expect(OctocatalogDiff::Spec.diff_match?(@result.diffs, lookup)).to eq(true), @result.diffs.inspect
+  end
+
+  it 'should contain a non-ignored diff in the same type' do
+    lookup = { type: 'Ssh_authorized_key', title: 'bob@local', structure: %w[parameters type] }
+    expect(OctocatalogDiff::Spec.diff_match?(@result.diffs, lookup)).to eq(true), @result.diffs.inspect
+  end
+
+  it 'should not contain an ignored diff in the same type' do
+    lookup = { type: 'Ssh_authorized_key', title: 'bob@local', structure: %w[parameters foo] }
+    expect(OctocatalogDiff::Spec.diff_match?(@result.diffs, lookup)).to eq(false), @result.diffs.inspect
+  end
+end

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

@@ -1285,6 +1285,24 @@
         expect(logger_str.string).not_to match(/Ignoring .+ matches {:type=>"\*", :title=>"dell", :attr=>"\*"}/)
       end
     end
+
+    context 'attrs regexp' do
+      it 'should filter on regular expression match' do
+        rule = { type: 'Apple', title: 'delicious', attr: Regexp.new("\\Aparameters\f(color|taste)\\z") }
+        logger, logger_str = OctocatalogDiff::Spec.setup_logger
+        testobj.instance_variable_set('@logger', logger)
+        expect(testobj.send(:"ignore_match?", rule, '+', resource, 'old_value', 'new_value')).to eq(true)
+        expect(logger_str.string).to match(/Ignoring .+ matches {:type=>"Apple", :title=>"delicious", :attr=>/)
+      end
+
+      it 'should not filter on regular expression non-match' do
+        rule = { type: 'Apple', title: 'delicious', attr: Regexp.new("\\Aparameters\f(odor|has_worms)\\z") }
+        logger, logger_str = OctocatalogDiff::Spec.setup_logger
+        testobj.instance_variable_set('@logger', logger)
+        expect(testobj.send(:"ignore_match?", rule, '+', resource, 'old_value', 'new_value')).to eq(false)
+        expect(logger_str.string).not_to match(/Ignoring .+ matches/)
+      end
+    end
   end
 
   describe '#ignore_tags' do
@@ -1373,4 +1391,134 @@
       expect(result[0]).to eq(['!', "Class\fOpenssl::Package\fparameters\fcommon-array", [1, 2, 3], [1, 5, 25], fileref, fileref])
     end
   end
+
+  describe '#regexp_operator_match?' do
+    let(:subject) { described_class.allocate }
+
+    context 'for a multi-line diff' do
+      context 'for operator =~>' do
+        let(:operator) { '=~>' }
+        let(:regex) { Regexp.new('\\A.(kittens|cats)\\z') }
+
+        it 'should return true when at least one line matches' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = "puppies\ndogs\n"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when neither line matches' do
+          old_val = "puppies\ndogs\ndonkeys\n"
+          new_val = "puppies\ndogs\n"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+
+      context 'for operator =&>' do
+        let(:operator) { '=&>' }
+        let(:regex) { Regexp.new('\\A(-|\\+)(kittens )*(kittens|cats)\\z') }
+
+        it 'should return true when all lines match' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = "kittens\nkittens\n"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when the regex does not match line' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = "kittens\nkittens\ndogs\n"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+
+        it 'should return true if both old and new do not end in a newline' do
+          old_val = "kittens\nkittens\ncats"
+          new_val = "kittens\nkittens\nkittens"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false if one ends in a newline and the other does not' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = "kittens\nkittens\nkittens"
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+    end
+
+    context 'for a single-line diff' do
+      context 'for operator =~>' do
+        let(:operator) { '=~>' }
+        let(:regex) { Regexp.new('\\A(-|\\+)(kittens )+(kittens|cats)\\z') }
+
+        it 'should return true when at least one line matches' do
+          old_val = 'kittens kittens kittens'
+          new_val = 'kittens cats'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when neither line matches' do
+          old_val = 'kittens dogs cats kittens kittens'
+          new_val = 'kittens cats dogs'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+
+      context 'for operator =&>' do
+        let(:operator) { '=&>' }
+        let(:regex) { Regexp.new('\\A(-|\\+)(kittens )+(kittens|cats)\\z') }
+
+        it 'should return true when both lines match' do
+          old_val = 'kittens kittens kittens'
+          new_val = 'kittens cats'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when the regex does not match line' do
+          old_val = 'kittens kittens dogs kittens'
+          new_val = 'kittens cats'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+    end
+
+    context 'for a multi-line versus a single-line diff' do
+      context 'for operator =~>' do
+        let(:operator) { '=~>' }
+        let(:regex) { Regexp.new('\\A.(kittens|cats)\\z') }
+
+        it 'should return true when at least one line matches' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = 'puppies'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when neither line matches' do
+          old_val = "puppies\ndogs\ndonkeys\n"
+          new_val = 'puppies'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+
+      context 'for operator =&>' do
+        let(:operator) { '=&>' }
+        let(:regex) { Regexp.new('\\A(-|\\+)(kittens )*(kittens|cats)\\z') }
+
+        it 'should return true when all lines match and both old and new do not end in newline' do
+          old_val = "kittens\nkittens\ncats"
+          new_val = 'kittens'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(true)
+        end
+
+        it 'should return false when all lines match but ending in newlines differs' do
+          old_val = "kittens\nkittens\ncats\n"
+          new_val = 'kittens'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+
+        it 'should return false when the regex does not match line' do
+          old_val = "kittens\nkittens\ncats"
+          new_val = 'dogs'
+          expect(subject.send(:regexp_operator_match?, operator, regex, old_val, new_val)).to eq(false)
+        end
+      end
+    end
+  end
 end