Skip to content

Commit 4babf7f

Browse files
kpaulissekpaulisse
authored
Merge pull request #103 from github/kpaulisse-where-was-the-broken-reference-declared
Identify where was the broken reference was declared
2 parents 5d6851c + 5ab37f8 commit 4babf7f

File tree

3 files changed

+193
-42
lines changed

3 files changed

+193
-42
lines changed

‎lib/octocatalog-diff/catalog.rb

Lines changed: 43 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -201,29 +201,56 @@ def validate_references
201201
end
202202
return if missing.empty?
203203

204-
# At this point there is at least one broken/missing reference. Format an error message and
205-
# raise. Error message will look like this:
206-
# ---
207-
# Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]];
208-
# exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2] ->
209-
# subscribe[Exec[subscribe target 2]]
210-
# ---
204+
# At this point there is at least one broken/missing reference. Format an error message and raise.
205+
errors = format_missing_references(missing)
206+
plural = errors =~ /;/ ? 's' : ''
207+
raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
208+
end
209+
210+
private
211+
212+
# Private method: Format the name of the source file and line number, based on compilation directory and
213+
# other settings. This is used by format_missing_references.
214+
# @param source_file [String] Raw source file name from catalog
215+
# @param line_number [Fixnum] Line number from catalog
216+
# @return [String] Formatted source file
217+
def format_source_file_line(source_file, line_number)
218+
return '' if source_file.nil? || source_file.empty?
219+
filename = if compilation_dir && source_file.start_with?(compilation_dir)
220+
stripped_file = source_file[compilation_dir.length..-1]
221+
stripped_file.start_with?('/') ? stripped_file[1..-1] : stripped_file
222+
else
223+
source_file
224+
end
225+
"(#{filename.sub(%r{^environments/production/}, '')}:#{line_number})"
226+
end
227+
228+
# Private method: Format the missing references into human-readable text
229+
# Error message will look like this:
230+
# ---
231+
# Catalog has broken references: exec[subscribe caller 1](file:line) -> subscribe[Exec[subscribe target]];
232+
# exec[subscribe caller 2](file:line) -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2](file:line) ->
233+
# subscribe[Exec[subscribe target 2]]
234+
# ---
235+
# @param missing [Array] Array of missing references
236+
# @return [String] Formatted references
237+
def format_missing_references(missing)
238+
unless missing.is_a?(Array) && missing.any?
239+
raise ArgumentError, 'format_missing_references() requires a non-empty array as input'
240+
end
241+
211242
formatted_references = missing.map do |obj|
212243
# obj[:target_value] can be a string or an array. If it's an array, create a
213244
# separate error message per element of that array. This allows the total number
214245
# of errors to be correct.
215-
src = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
246+
src_ref = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
247+
src_file = format_source_file_line(obj[:source]['file'], obj[:source]['line'])
216248
target_val = obj[:target_value].is_a?(Array) ? obj[:target_value] : [obj[:target_value]]
217-
target_val.map { |tv| "#{src} -> #{obj[:target_type].downcase}[#{tv}]" }
218-
end
219-
formatted_references.flatten!
220-
plural = formatted_references.size == 1 ? '' : 's'
221-
errors = formatted_references.join('; ')
222-
raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
249+
target_val.map { |tv| "#{src_ref}#{src_file} -> #{obj[:target_type].downcase}[#{tv}]" }
250+
end.flatten
251+
formatted_references.join('; ')
223252
end
224253

225-
private
226-
227254
# Private method: Given a list of resources to check, return the references from
228255
# that list that are missing from the catalog. (An empty array returned would indicate
229256
# all references are present in the catalog.)

‎spec/octocatalog-diff/integration/reference_validation_spec.rb

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -102,14 +102,17 @@ def self.catalog_contains_resource(result, type, title)
102102
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
103103
end
104104

105+
# Multiple line numbers given because Puppet 4.x and 3.8 correspond to first and last line of resource, respectively.
106+
# rubocop:disable Metrics/LineLength
105107
it 'should have formatted error messages' do
106108
msg = @result.exception.message
107-
expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
108-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
109-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
110-
expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
111-
expect(msg).not_to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe caller 1\]\]/)
109+
expect(msg).to match(%r{exec\[subscribe caller 1\]\(modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
110+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
111+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
112+
expect(msg).to match(%r{exec\[subscribe caller 3\]\(modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
113+
expect(msg).not_to match(/exec\[subscribe caller 3\].+subscribe\[Exec\[subscribe caller 1\]\]/)
112114
end
115+
# rubocop:enable Metrics/LineLength
113116
end
114117

115118
context 'with broken before' do
@@ -125,10 +128,12 @@ def self.catalog_contains_resource(result, type, title)
125128
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
126129
end
127130

131+
# rubocop:disable Metrics/LineLength
128132
it 'should have formatted error messages' do
129133
msg = @result.exception.message
130-
expect(msg).to eq('Catalog has broken reference: exec[before caller] -> before[Exec[before target]]')
134+
expect(msg).to match(%r{Catalog has broken reference: exec\[before caller\]\(modules/test/manifests/before_callers.pp:(2|5)\) -> before\[Exec\[before target\]\]})
131135
end
136+
# rubocop:enable Metrics/LineLength
132137
end
133138

134139
context 'with broken notify' do
@@ -144,10 +149,12 @@ def self.catalog_contains_resource(result, type, title)
144149
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
145150
end
146151

152+
# rubocop:disable Metrics/LineLength
147153
it 'should have formatted error messages' do
148154
msg = @result.exception.message
149-
expect(msg).to match(/exec\[notify caller\] -> notify\[Test::Foo::Bar\[notify target\]\]/)
155+
expect(msg).to match(%r{exec\[notify caller\]\(modules/test/manifests/notify_callers.pp:(2|4)\) -> notify\[Test::Foo::Bar\[notify target\]\]})
150156
end
157+
# rubocop:enable Metrics/LineLength
151158
end
152159

153160
context 'with broken require' do
@@ -163,14 +170,16 @@ def self.catalog_contains_resource(result, type, title)
163170
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
164171
end
165172

173+
# rubocop:disable Metrics/LineLength
166174
it 'should have formatted error messages' do
167175
msg = @result.exception.message
168-
expect(msg).to match(/exec\[require caller\] -> require\[Exec\[require target\]\]/)
169-
expect(msg).to match(/exec\[require caller 3\] -> require\[Exec\[require target\]\]/)
170-
expect(msg).to match(/exec\[require caller 4\] -> require\[Exec\[require target\]\]/)
176+
expect(msg).to match(%r{exec\[require caller\]\(modules/test/manifests/require_callers.pp:(2|5)\) -> require\[Exec\[require target\]\]})
177+
expect(msg).to match(%r{exec\[require caller 3\]\(modules/test/manifests/require_callers.pp:(12|18)\) -> require\[Exec\[require target\]\]})
178+
expect(msg).to match(%r{exec\[require caller 4\]\(modules/test/manifests/require_callers.pp:(12|18)\) -> require\[Exec\[require target\]\]})
171179
expect(msg).not_to match(/exec\[require caller 2\]/)
172180
expect(msg).not_to match(/-> require\[Exec\[require caller\]\]/)
173181
end
182+
# rubocop:enable Metrics/LineLength
174183
end
175184

176185
context 'with broken subscribe but subscribe not checked' do
@@ -223,13 +232,15 @@ def self.catalog_contains_resource(result, type, title)
223232
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
224233
end
225234

235+
# rubocop:disable Metrics/LineLength
226236
it 'should have formatted error messages' do
227237
msg = @result.exception.message
228-
expect(msg).to match(/exec\[before alias caller\] -> before\[Exec\[before alias target\]\]/)
229-
expect(msg).to match(/exec\[notify alias caller\] -> before\[Exec\[notify alias target\]\]/)
230-
expect(msg).to match(/exec\[require alias caller\] -> before\[Exec\[require alias target\]\]/)
231-
expect(msg).to match(/exec\[subscribe alias caller\] -> before\[Exec\[subscribe alias target\]\]/)
238+
expect(msg).to match(%r{exec\[before alias caller\]\(modules/test/manifests/alias_callers.pp:(2|5)\) -> before\[Exec\[before alias target\]\]})
239+
expect(msg).to match(%r{exec\[notify alias caller\]\(modules/test/manifests/alias_callers.pp:(7|10)\) -> before\[Exec\[notify alias target\]\]})
240+
expect(msg).to match(%r{exec\[require alias caller\]\(modules/test/manifests/alias_callers.pp:(12|15)\) -> before\[Exec\[require alias target\]\]})
241+
expect(msg).to match(%r{exec\[subscribe alias caller\]\(modules/test/manifests/alias_callers.pp:(17|20)\) -> before\[Exec\[subscribe alias target\]\]})
232242
end
243+
# rubocop:enable Metrics/LineLength
233244
end
234245
end
235246

@@ -277,13 +288,15 @@ def self.catalog_contains_resource(result, type, title)
277288
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
278289
end
279290

291+
# rubocop:disable Metrics/LineLength
280292
it 'should have formatted error messages' do
281293
msg = @result.exception.message
282-
expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
283-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
284-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
285-
expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
294+
expect(msg).to match(%r{exec\[subscribe caller 1\]\(.+/modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
295+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
296+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
297+
expect(msg).to match(%r{exec\[subscribe caller 3\]\(.+/modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
286298
end
299+
# rubocop:enable Metrics/LineLength
287300
end
288301

289302
context 'with broken references in from-catalog' do
@@ -321,14 +334,16 @@ def self.catalog_contains_resource(result, type, title)
321334
expect(@result.exception).to be_a_kind_of(OctocatalogDiff::Errors::ReferenceValidationError)
322335
end
323336

337+
# rubocop:disable Metrics/LineLength
324338
it 'should have formatted error messages from to-catalog only' do
325339
msg = @result.exception.message
326-
expect(msg).to match(/exec\[subscribe caller 1\] -> subscribe\[Exec\[subscribe target\]\]/)
327-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target\]\]/)
328-
expect(msg).to match(/exec\[subscribe caller 2\] -> subscribe\[Exec\[subscribe target 2\]\]/)
329-
expect(msg).to match(/exec\[subscribe caller 3\] -> subscribe\[Exec\[subscribe target\]\]/)
340+
expect(msg).to match(%r{exec\[subscribe caller 1\]\(.+/modules/test/manifests/subscribe_callers.pp:(2|5)\) -> subscribe\[Exec\[subscribe target\]\]})
341+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target\]\]})
342+
expect(msg).to match(%r{exec\[subscribe caller 2\]\(.+/modules/test/manifests/subscribe_callers.pp:(7|13)\) -> subscribe\[Exec\[subscribe target 2\]\]})
343+
expect(msg).to match(%r{exec\[subscribe caller 3\]\(.+/modules/test/manifests/subscribe_callers.pp:(15|21)\) -> subscribe\[Exec\[subscribe target\]\]})
330344
expect(msg).not_to match(/require target/)
331345
end
346+
# rubocop:enable Metrics/LineLength
332347
end
333348

334349
context 'with broken references, but checking not enabled' do

‎spec/octocatalog-diff/tests/catalog_spec.rb

Lines changed: 113 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -471,16 +471,125 @@
471471
json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/reference-validation-broken.json'))
472472
}
473473
catalog = OctocatalogDiff::Catalog.new(opts)
474+
catalog.compilation_dir = '/var/folders/dw/5ftmkqk972j_kw2fdjyzdqdw0000gn/T/d20161223-46780-x10xaf/environments/production'
474475
error_str = [
475-
'Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]]',
476-
'exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]',
477-
'exec[subscribe caller 2] -> subscribe[Exec[subscribe target 2]]',
478-
'exec[subscribe caller 3] -> subscribe[Exec[subscribe target]]'
476+
'Catalog has broken references: exec[subscribe caller 1](modules/test/manifests/subscribe_callers.pp:2)' \
477+
' -> subscribe[Exec[subscribe target]]',
478+
'exec[subscribe caller 2](modules/test/manifests/subscribe_callers.pp:7) -> subscribe[Exec[subscribe target]]',
479+
'exec[subscribe caller 2](modules/test/manifests/subscribe_callers.pp:7) -> subscribe[Exec[subscribe target 2]]',
480+
'exec[subscribe caller 3](modules/test/manifests/subscribe_callers.pp:15) -> subscribe[Exec[subscribe target]]'
479481
].join('; ')
480482
expect { catalog.validate_references }.to raise_error(OctocatalogDiff::Errors::ReferenceValidationError, error_str)
481483
end
482484
end
483485

486+
describe '#format_missing_references' do
487+
before(:each) do
488+
opts = { json: File.read(OctocatalogDiff::Spec.fixture_path('catalogs/reference-validation-broken.json')) }
489+
@test_obj = OctocatalogDiff::Catalog.new(opts)
490+
end
491+
492+
context 'with invalid input' do
493+
it 'should raise ArgumentError if non-array is provided' do
494+
expect do
495+
@test_obj.send(:format_missing_references, 'Hi there')
496+
end.to raise_error(ArgumentError, /format_missing_references\(\) requires a non-empty array as input/)
497+
end
498+
499+
it 'should raise ArgumentError if empty array is provided' do
500+
expect do
501+
@test_obj.send(:format_missing_references, [])
502+
end.to raise_error(ArgumentError, /format_missing_references\(\) requires a non-empty array as input/)
503+
end
504+
end
505+
506+
context 'with compilation directory specified and matching' do
507+
it 'should strip compilation directory' do
508+
allow(@test_obj).to receive(:compilation_dir)
509+
.and_return('/var/folders/dw/foo/environments/production')
510+
obj = {
511+
source: {
512+
'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
513+
'line' => 23,
514+
'type' => 'Baz',
515+
'title' => 'buzz'
516+
},
517+
target_type: 'Foo',
518+
target_value: 'bar'
519+
}
520+
result = @test_obj.send(:format_missing_references, [obj])
521+
expect(result).to eq('baz[buzz](modules/foo/manifests/bar.pp:23) -> foo[bar]')
522+
end
523+
end
524+
525+
context 'with compilation directory specified and not matching' do
526+
it 'should not strip compilation directory' do
527+
allow(@test_obj).to receive(:compilation_dir)
528+
.and_return('/var/folders/dw/bar/environments/production')
529+
obj = {
530+
source: {
531+
'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
532+
'line' => 23,
533+
'type' => 'Baz',
534+
'title' => 'buzz'
535+
},
536+
target_type: 'Foo',
537+
target_value: 'bar'
538+
}
539+
result = @test_obj.send(:format_missing_references, [obj])
540+
expect(result).to eq('baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]')
541+
end
542+
end
543+
544+
context 'with compilation directory not specified' do
545+
it 'should not strip compilation directory' do
546+
allow(@test_obj).to receive(:compilation_dir).and_return(nil)
547+
obj = {
548+
source: {
549+
'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
550+
'line' => 23,
551+
'type' => 'Baz',
552+
'title' => 'buzz'
553+
},
554+
target_type: 'Foo',
555+
target_value: 'bar'
556+
}
557+
result = @test_obj.send(:format_missing_references, [obj])
558+
expect(result).to eq('baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]')
559+
end
560+
end
561+
562+
context 'with multiple targets for the same resource' do
563+
it 'should display each target separately' do
564+
allow(@test_obj).to receive(:compilation_dir).and_return(nil)
565+
src = {
566+
'file' => '/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp',
567+
'line' => 23,
568+
'type' => 'Baz',
569+
'title' => 'buzz'
570+
}
571+
obj = [
572+
{
573+
source: src,
574+
target_type: 'Foo',
575+
target_value: 'bar'
576+
},
577+
{
578+
source: src,
579+
target_type: 'Fizz',
580+
target_value: 'buzz'
581+
}
582+
]
583+
answer = [
584+
'baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> foo[bar]',
585+
'baz[buzz](/var/folders/dw/foo/environments/production/modules/foo/manifests/bar.pp:23) -> fizz[buzz]'
586+
].join('; ')
587+
result = @test_obj.send(:format_missing_references, obj)
588+
expect(result).to eq(answer)
589+
end
590+
end
591+
end
592+
484593
describe '#build_resource_hash' do
485594
before(:each) do
486595
resource_array = [

0 commit comments

Comments
 (0)