Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate ids #221

Merged
2 commits merged into from
Jun 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ jobs:
- run:
name: Rspec tests
command: bundle exec rspec --color --require spec_helper spec
- run:
name: Duplicate rule check
command: bundle exec ./bin/cfn_nag_rules
end_to_end_test:
docker:
- image: circleci/ruby:2.5.5-browsers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def rule_type
end

def rule_id
'F24'
'F33'
end

def audit_impl(cfn_model)
Expand Down
14 changes: 14 additions & 0 deletions lib/cfn-nag/result_view/rules_view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,24 @@ def emit(rule_registry, profile)
puts
puts 'FAILING VIOLATIONS:'
emit_failings rule_registry.failings, profile

if rule_registry.duplicate_ids?
emit_duplicates(rule_registry.duplicate_ids)
exit 1
end
end

private

def emit_duplicates(duplicates)
duplicates.each do |info|
puts '------------------'.red
puts "Rule ID conflict detected for #{info[:id]}.".red
puts "New rule: #{info[:new_message]}".red
puts "Registered rule: #{info[:registered_message]}".red
end
end

def emit_warnings(warnings, profile)
warnings.sort { |left, right| sort_id(left, right) }.each do |warning|
if profile.nil?
Expand Down
13 changes: 11 additions & 2 deletions lib/cfn-nag/rule_registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,15 @@
require_relative 'rule_definition'

class RuleRegistry
attr_reader :rules
attr_reader :rules, :duplicate_ids

def initialize
@rules = []
@duplicate_ids = []
end

def duplicate_ids?
@duplicate_ids.count.positive?
end

def definition(id:,
Expand All @@ -20,7 +25,11 @@ def definition(id:,
if existing_def.nil?
add_rule rule_definition
else
existing_def
@duplicate_ids << {
id: id,
new_message: message,
Copy link

@ghost ghost Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new v. old is fine, but poking at two things:

  1. is the order stable for this to have meaning?
  2. on the off chance we have > 2 collisions... i guess it shows A/B and then B/C?

Perhaps make @duplicate_ids a Hash with id key and array of messages?

I guess there's no way to say where the duplicate class is coming from? for core rules this is a bug that we rightfully stop the release pipeline for..... but when there are custom rule directories that users are pointing to....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have suggested that custom rules have their own number system with a prefix of C. But yeah, this is just a barebones approach to fail CI builds when there are duplicates. The messages aren't perfect but they enabled me to quickly troubleshoot the one duplicate we had. If we want something fancier, we can always iterate on it in the future as need arises.

registered_message: existing_def.message
}
end
end

Expand Down
37 changes: 37 additions & 0 deletions spec/end_to_end/cfn_nag_rules/rules_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,41 @@
expect(failure_rule_count).not_to eq 0
end
end

context 'detecting a duplicate rule id' do
before(:all) do
@duplicate_rule = 'lib/cfn-nag/custom_rules/DuplicateRule.rb'
duplicate_rule_content = <<-CONTENT
require 'cfn-nag/violation'
require_relative 'base'

class DuplicateRule < BaseRule
def rule_text
'Example duplicate rule'
end

def rule_type
Violation::FAILING_VIOLATION
end

def rule_id
'F1'
end

def audit_impl(cfn_model); end
end
CONTENT
File.open(@duplicate_rule, 'w') {|f| f.write(duplicate_rule_content) }
@stdout,@stderr,@status = Open3.capture3("bundle exec ./bin/cfn_nag_rules")
end

after(:all) do
File.delete(@duplicate_rule)
end

it 'blah' do
puts @status.exitstatus
expect(@status.exitstatus).to eq 1
end
end
end
11 changes: 10 additions & 1 deletion spec/rule_registry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,22 @@
]

expect(rule_registry.rules).to eq expected_rules
expect(rule_registry.duplicate_ids?).to be false

# add a dupe
rule_registry.definition(id: 'F444',
type: RuleDefinition::WARNING,
message: 'you have been warned!!')
message: 'you have been warned!! dupe')

expected_duplicate = {
id: 'F444',
new_message: 'you have been warned!! dupe',
registered_message: 'you have been warned!!'
}

expect(rule_registry.rules).to eq expected_rules
expect(rule_registry.duplicate_ids?).to be true
expect(rule_registry.duplicate_ids.first).to eq expected_duplicate
end
end
end
Expand Down