Skip to content

Commit 95bc03d

Browse files
authored
Merge pull request #61 from github/kpaulisse-absent-to-filter
Change the "ignore changes to absent file" to a filter
2 parents be5e316 + b5a920e commit 95bc03d

File tree

12 files changed

+192
-107
lines changed

12 files changed

+192
-107
lines changed

doc/advanced-filter.md

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,66 @@ Please note that there are other options to ignore specified diffs, including:
99

1010
Here is the list of available filters and an explanation of each:
1111

12+
- [Absent file](/doc/advanced-filter.md#absent-file) - Ignore parameter changes of a file that is declared to be absent
1213
- [YAML](/doc/advanced-filter.md#yaml) - Ignore whitespace/comment differences if YAML parses to the same object
1314

15+
## Absent File
16+
17+
#### Usage
18+
19+
```
20+
--filters AbsentFile
21+
```
22+
23+
#### Description
24+
25+
When the `AbsentFile` filter is enabled, if any file is `ensure => absent` in the *new* catalog, then changes to any other parameters will be suppressed.
26+
27+
Consider that a file resource is declared as follows in two catalogs:
28+
29+
```
30+
# Old catalog
31+
file { '/etc/some-file':
32+
ensure => present,
33+
owner => 'root',
34+
group => 'nobody',
35+
content => 'my content here',
36+
}
37+
38+
# New catalog
39+
file { '/etc/some-file':
40+
ensure => absent,
41+
owner => 'bob',
42+
}
43+
```
44+
45+
Since the practical effect of the new catalog will be to remove the file, it doesn't matter that the owner of the (non-existent) file has changed from 'root' to 'bob', or that the content and group have changed from a string to undefined. Consider the default output without the filter:
46+
47+
```
48+
File[/etc/some-file] =>
49+
parameters =>
50+
ensure =>
51+
- present
52+
+ absent
53+
group =>
54+
- nobody
55+
owner =>
56+
- root
57+
+ bob
58+
content =>
59+
- my content here
60+
```
61+
62+
Wouldn't it be nice if the meaningless information didn't appear, and all you saw was the transition you actually care about, from present to absent? With `--filters AbsentFile` it does just this:
63+
64+
```
65+
File[/etc/some-file] =>
66+
parameters =>
67+
ensure =>
68+
- present
69+
+ absent
70+
```
71+
1472
## YAML
1573

1674
#### Usage

doc/optionsref.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1036,7 +1036,8 @@ cached directory). (<a href="../lib/octocatalog-diff/cli/options/safe_to_delete_
10361036
<td valign=top>
10371037
If enabled, this option will suppress changes to certain attributes of a file, if the
10381038
file is specified to be 'absent' in the target catalog. Suppressed changes in this case
1039-
include user, group, mode, and content, because a removed file has none of those. (<a href="../lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb">suppress_absent_file_details.rb</a>)
1039+
include user, group, mode, and content, because a removed file has none of those.
1040+
<i>This option is DEPRECATED; please use <code>--filters AbsentFile</code> instead.</i> (<a href="../lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb">suppress_absent_file_details.rb</a>)
10401041
</td>
10411042
</tr>
10421043

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

Lines changed: 6 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -147,12 +147,14 @@ def catdiff
147147
# Remove resources that have been explicitly ignored
148148
filter_diffs_for_ignored_items(result)
149149

150-
# If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
151-
# out any such parameters from the result array.
152-
filter_diffs_for_absent_files(result) if @opts[:suppress_absent_file_details]
150+
# Legacy options which are now filters
151+
@opts[:filters] ||= []
152+
if @opts[:suppress_absent_file_details] && !@opts[:filters].include?('absent_file')
153+
@opts[:filters] << 'AbsentFile'
154+
end
153155

154156
# Apply any additional pluggable filters.
155-
OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters])
157+
OctocatalogDiff::CatalogDiff::Filter.apply_filters(result, @opts[:filters]) if @opts[:filters].any?
156158

157159
# That's it!
158160
@logger.debug "Exiting catdiff; change count: #{result.size}"
@@ -166,43 +168,6 @@ def filter_diffs_for_ignored_items(result)
166168
result.reject! { |item| ignored?(item) }
167169
end
168170

169-
# If a file has ensure => absent, there are certain parameters that don't matter anymore. Filter
170-
# out any such parameters from the result array.
171-
# @param result [Array] Diff result list (modified by this method)
172-
def filter_diffs_for_absent_files(result)
173-
@logger.debug "Entering filter_diffs_for_absent_files with #{result.size} diffs"
174-
175-
# Scan for files in the result that are file resources with ensure => absent.
176-
absent_files = Set.new
177-
result.each do |diff|
178-
next unless diff[0] == '~' || diff[0] == '!'
179-
next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
180-
next unless ['absent', 'false', false].include?(diff[3])
181-
absent_files.add Regexp.last_match(1)
182-
end
183-
184-
# If there are any absent files, remove all diffs referencing that file, except for
185-
# the change to 'ensure'.
186-
if absent_files.any?
187-
keep = %w(ensure backup force provider)
188-
result.map! do |diff|
189-
if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
190-
if absent_files.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
191-
@logger.debug "Removing file=#{Regexp.last_match(1)} parameter=#{Regexp.last_match(2)} for absent file"
192-
nil
193-
else
194-
diff
195-
end
196-
else
197-
diff
198-
end
199-
end
200-
result.compact!
201-
end
202-
203-
@logger.debug "Exiting filter_diffs_for_absent_files with #{result.size} diffs"
204-
end
205-
206171
# Pre-processing of a catalog.
207172
# - Remove 'before' and 'require' from parameters
208173
# - Sort 'tags' array, or remove the tags array if tags are being ignored

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
require_relative 'filter/absent_file'
12
require_relative 'filter/yaml'
23

34
module OctocatalogDiff
@@ -24,14 +25,19 @@ def self.apply_filters(result, filter_names, options = {})
2425
# @param options [Hash] Additional options (optional) to pass to filtered? method
2526
def self.filter(result, filter_class_name, options = {})
2627
filter_class_name = [name.to_s, filter_class_name].join('::')
27-
clazz = Kernel.const_get(filter_class_name)
28-
result.reject! { |item| clazz.filtered?(item, options) }
28+
obj = Kernel.const_get(filter_class_name).new(result)
29+
result.reject! { |item| obj.filtered?(item, options) }
30+
end
31+
32+
# Inherited: Constructor that does nothing. Some filters require working on the entire
33+
# data set and will override this method to perform some pre-processing for efficiency.
34+
def initialize(_diff_array = [])
2935
end
3036

3137
# Inherited: Construct a default `filtered?` method for the subclass via inheritance.
3238
# Each subclass must implement this method, so the default method errors.
33-
def self.filtered?(_item, _options = {})
34-
raise "No `filtered?` method is implemented in #{name}"
39+
def filtered?(_item, _options = {})
40+
raise "No `filtered?` method is implemented in #{self.class}"
3541
end
3642
end
3743
end
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# frozen_string_literal: true
2+
3+
require 'set'
4+
5+
module OctocatalogDiff
6+
module CatalogDiff
7+
class Filter
8+
# Filter out changes in parameters when the "to" resource has ensure => absent.
9+
class AbsentFile < OctocatalogDiff::CatalogDiff::Filter
10+
# Constructor: Since this filter requires knowledge of the entire array of diffs,
11+
# override the inherited method to store those diffs in an instance variable.
12+
def initialize(diffs)
13+
@diffs = diffs
14+
@results = nil
15+
end
16+
17+
# Public: If a file has ensure => absent, there are certain parameters that don't
18+
# matter anymore. Filter out any such parameters from the result array.
19+
# Return true if the difference is in a resource where `ensure => absent` has been
20+
# declared. Return false if they this is not the case.
21+
#
22+
# @param diff [internal diff format] Difference
23+
# @param _options [Hash] Additional options (there are none for this filter)
24+
# @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
25+
def filtered?(diff, _options = {})
26+
build_results if @results.nil?
27+
@results.include?(diff)
28+
end
29+
30+
private
31+
32+
# Private: The first time `.filtered?` is called, build up the cache of results.
33+
# Returns nothing, but populates @results.
34+
def build_results
35+
# Which files can we ignore?
36+
@files_to_ignore = Set.new
37+
@diffs.each do |diff|
38+
next unless diff[0] == '~' || diff[0] == '!'
39+
next unless diff[1] =~ /^File\f([^\f]+)\fparameters\fensure$/
40+
next unless ['absent', 'false', false].include?(diff[3])
41+
@files_to_ignore.add Regexp.last_match(1)
42+
end
43+
44+
# Based on that, which diffs can we ignore?
45+
@results = Set.new @diffs.reject { |diff| keep_diff?(diff) }
46+
end
47+
48+
# Private: Determine whether to keep a particular diff.
49+
# @param diff [OctocatalogDiff::API::V1::Diff] Difference under consideration
50+
# @return [Boolean] true = keep, false = discard
51+
def keep_diff?(diff)
52+
keep = %w(ensure backup force provider)
53+
if (diff[0] == '!' || diff[0] == '~') && diff[1] =~ /^File\f(.+)\fparameters\f(.+)$/
54+
if @files_to_ignore.include?(Regexp.last_match(1)) && !keep.include?(Regexp.last_match(2))
55+
return false
56+
end
57+
end
58+
true
59+
end
60+
end
61+
end
62+
end
63+
end

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ class YAML < OctocatalogDiff::CatalogDiff::Filter
1414
# @param diff [Array] Difference
1515
# @param _options [Hash] Additional options (there are none for this filter)
1616
# @return [Boolean] true if this difference is a YAML file with identical objects, false otherwise
17-
def self.filtered?(diff, _options = {})
17+
def filtered?(diff, _options = {})
1818
# Skip additions or removals - focus only on changes
1919
return false unless diff[0] == '~' || diff[0] == '!'
2020

lib/octocatalog-diff/cli/options/suppress_absent_file_details.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# If enabled, this option will suppress changes to certain attributes of a file, if the
44
# file is specified to be 'absent' in the target catalog. Suppressed changes in this case
55
# include user, group, mode, and content, because a removed file has none of those.
6+
# <i>This option is DEPRECATED; please use <code>--filters AbsentFile</code> instead.</i>
67
# @param parser [OptionParser object] The OptionParser argument
78
# @param options [Hash] Options hash being constructed; this is modified in this method.
89
OctocatalogDiff::Cli::Options::Option.newoption(:suppress_absent_file_details) do

spec/octocatalog-diff/integration/file_absent_spec.rb

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,6 @@
2222
diff = diffs.first
2323
expect(diff[1..3]).to eq(["File\f/tmp/foo\fparameters\fensure", nil, 'absent'])
2424
end
25-
26-
it 'should print the proper debug log messages' do
27-
expect(@result[:logs]).to match(/Entering filter_diffs_for_absent_files with 4 diffs/)
28-
expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=group for absent file})
29-
expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=owner for absent file})
30-
expect(@result[:logs]).to match(%r{Removing file=/tmp/foo parameter=mode for absent file})
31-
expect(@result[:logs]).to match(/Exiting filter_diffs_for_absent_files with 1 diffs/)
32-
end
3325
end
3426

3527
context 'with file absent suppression disabled' do
@@ -54,9 +46,5 @@
5446
expect(diffs[2][1..3]).to eq(["File\f/tmp/foo\fparameters\fowner", 'root', 'git'])
5547
expect(diffs[3][1..3]).to eq(["File\f/tmp/foo\fparameters\fensure", nil, 'absent'])
5648
end
57-
58-
it 'should print the proper debug log messages' do
59-
expect(@result[:logs]).not_to match(/Entering filter_diffs_for_absent_files/)
60-
end
6149
end
6250
end

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

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1251,35 +1251,4 @@
12511251
expect(result[0]).to eq(['!', "Class\fOpenssl::Package\fparameters\fcommon-array", [1, 2, 3], [1, 5, 25], fileref, fileref])
12521252
end
12531253
end
1254-
1255-
describe '#filter_diffs_for_absent_files' do
1256-
before(:each) do
1257-
empty_puppet_catalog_json = File.read(OctocatalogDiff::Spec.fixture_path('catalogs/catalog-empty.json'))
1258-
empty_puppet_catalog = OctocatalogDiff::Catalog.new(json: empty_puppet_catalog_json)
1259-
logger, @logger_str = OctocatalogDiff::Spec.setup_logger
1260-
@obj = OctocatalogDiff::CatalogDiff::Differ.new({ logger: logger }, empty_puppet_catalog, empty_puppet_catalog)
1261-
@orig = [
1262-
['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
1263-
['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
1264-
['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
1265-
['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
1266-
['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
1267-
['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
1268-
['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
1269-
['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
1270-
]
1271-
@result = @orig.dup
1272-
@obj.send(:filter_diffs_for_absent_files, @result)
1273-
end
1274-
1275-
it 'should filter out some attributes for ensure=>absent file' do
1276-
expect(@result).to eq(@orig.values_at(0, 2, 3, 4, 5, 6, 7))
1277-
end
1278-
1279-
it 'should log messages' do
1280-
expect(@logger_str.string).to match(/Entering filter_diffs_for_absent_files with 8 diffs/)
1281-
expect(@logger_str.string).to match(%r{Removing file=/tmp/foo parameter=owner for absent file})
1282-
expect(@logger_str.string).to match(/Exiting filter_diffs_for_absent_files with 7 diffs/)
1283-
end
1284-
end
12851254
end
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# frozen_string_literal: true
2+
3+
require_relative '../../spec_helper'
4+
require OctocatalogDiff::Spec.require_path('/catalog')
5+
require OctocatalogDiff::Spec.require_path('/catalog-diff/filter/absent_file')
6+
7+
describe OctocatalogDiff::CatalogDiff::Filter::AbsentFile do
8+
it 'should filter out some attributes for ensure=>absent file' do
9+
orig = [
10+
['~', "File\f/tmp/foo\fparameters\fensure", 'file', 'absent'],
11+
['~', "File\f/tmp/foo\fparameters\fowner", 'root', 'nobody'],
12+
['~', "File\f/tmp/foo\fparameters\fbackup", true, nil],
13+
['~', "File\f/tmp/foo\fparameters\fforce", false, nil],
14+
['~', "File\f/tmp/foo\fparameters\fprovider", 'root', nil],
15+
['~', "File\f/tmp/bar\fparameters\fensure", 'file', 'link'],
16+
['~', "File\f/tmp/bar\fparameters\ftarget", nil, '/tmp/foo'],
17+
['~', "Exec\f/tmp/bar\fparameters\fcommand", nil, '/tmp/foo']
18+
]
19+
testobj = described_class.new(orig)
20+
result = orig.reject { |x| testobj.filtered?(x) }
21+
expect(result).to eq(orig.values_at(0, 2, 3, 4, 5, 6, 7))
22+
end
23+
end

0 commit comments

Comments
 (0)