Rubocop + fixes

- Enables rubocop
- Fixes rubocop ofenses
- Fixes broken `facts_to_tags` on A5, which was broken because the tests
didn't run because the syntax was not correct (uncovered by rubocop)
- Adds a test for `facts_to_tags` on A6/A7.

Author: albertvaka

.rubocop.yml

@@ -131,3 +131,5 @@ Style/IfUnlessModifier:
   Enabled: false
 Style/SymbolProc:
   Enabled: false
+Style/NumericPredicate:
+  Enabled: false

Gemfile

@@ -28,6 +28,9 @@ group :development do
     gem "puppet-lint", "~> 2.4.2"
     gem "metadata-json-lint", "~> 1.2.2"
     gem "puppet-syntax", "~> 2.5.0"
-    gem "rspec-puppet", '2.6.9'
+    gem "rspec-puppet", "~> 2.6.9"
+    gem "rubocop", "~> 0.49.1"
+    gem "rubocop-i18n", "~> 1.2.0"
+    gem "rubocop-rspec", "~> 1.16.0"
   end
 end

Rakefile

@@ -2,6 +2,7 @@ require 'puppetlabs_spec_helper/rake_tasks'
 require 'puppet-syntax/tasks/puppet-syntax'
 require 'puppet_blacksmith/rake_tasks' if Bundler.rubygems.find_name('puppet-blacksmith').any?
 require 'puppet-lint/tasks/puppet-lint'
+require 'rubocop/rake_task'
 
 exclude_paths = [
   "pkg/**/*",
@@ -19,5 +20,6 @@ task :test => [
   'check:symlinks',
   'check:git_ignore',
   'check:dot_underscore',
+  'rubocop',
   'spec',
 ]

lib/puppet/parser/functions/to_instances_yaml.rb

@@ -1,17 +1,17 @@
 require 'yaml'
 
 module Puppet::Parser::Functions
-    newfunction(:to_instances_yaml, :type => :rvalue) do |args|
-        init_config = args[0]
-        instances = args[1]
-        logs = args[2]
-        default_values = {
-            'init_config'  => init_config.is_a?(String) ? nil: init_config,
-            'instances' => instances
-        }
-        if !logs.nil? && !logs.empty? then
-          default_values.merge!({'logs' => logs})
-        end
-        YAML::dump(default_values)
+  newfunction(:to_instances_yaml, type: :rvalue) do |args|
+    init_config = args[0]
+    instances = args[1]
+    logs = args[2]
+    default_values = {
+      'init_config' => init_config.is_a?(String) ? nil : init_config,
+      'instances' => instances,
+    }
+    if !logs.nil? && !logs.empty?
+      default_values['logs'] = logs
     end
+    YAML.dump(default_values)
+  end
 end

lib/puppet/reports/datadog_reports.rb

@@ -3,13 +3,12 @@
 
 begin
   require 'dogapi'
-rescue LoadError => e
-  Puppet.info "You need the `dogapi` gem to use the Datadog report (run puppet with puppet_run_reports on your master)"
+rescue LoadError
+  Puppet.info 'You need the `dogapi` gem to use the Datadog report (run puppet with puppet_run_reports on your master)'
 end
 
 Puppet::Reports.register_report(:datadog_reports) do
-
-  configfile = "/etc/datadog-agent/datadog-reports.yaml"
+  configfile = '/etc/datadog-agent/datadog-reports.yaml'
   raise(Puppet::ParseError, "Datadog report config file #{configfile} not readable") unless File.readable?(configfile)
   config = YAML.load_file(configfile)
   API_KEY = config[:datadog_api_key]
@@ -22,7 +21,7 @@
     rescue
       raise(Puppet::ParseError, "Invalid hostname_extraction_regex #{HOSTNAME_REGEX}")
     end
-    unless HOSTNAME_EXTRACTION_REGEX.named_captures.has_key? "hostname"
+    unless HOSTNAME_EXTRACTION_REGEX.named_captures.key? 'hostname'
       raise(Puppet::ParseError, "hostname_extraction_regex doesn't include a match group named 'hostname': #{HOSTNAME_REGEX}")
     end
   else
@@ -34,25 +33,22 @@
   DESC
 
   def pluralize(number, noun)
-    begin
-      if number == 0 then
-        "no #{noun}"
-      elsif number < 1 then
-        "less than 1 #{noun}"
-      elsif number == 1 then
-        "1 #{noun}"
-      else
-        "#{number.round} #{noun}s"
-      end
-    rescue
-      "#{number} #{noun}(s)"
+    if number == 0
+      "no #{noun}"
+    elsif number < 1
+      "less than 1 #{noun}"
+    elsif number == 1
+      "1 #{noun}"
+    else
+      "#{number.round} #{noun}s"
     end
+  rescue
+    "#{number} #{noun}(s)"
   end
 
-
   def process
-    @summary = self.summary
-    @msg_host = self.host
+    @summary = summary
+    @msg_host = host
     unless HOSTNAME_EXTRACTION_REGEX.nil?
       m = @msg_host.match(HOSTNAME_EXTRACTION_REGEX)
       if !m.nil? && !m[:hostname].nil?
@@ -65,23 +61,23 @@ def process
     event_priority = 'low'
     event_data = ''
 
-    if defined?(self.status)
+    if defined?(status)
       # for puppet log format 2 and above
-      @status = self.status
+      @status = status
       if @status == 'failed'
         event_title = "Puppet failed on #{@msg_host}"
-        alert_type = "error"
-        event_priority = "normal"
+        alert_type = 'error'
+        event_priority = 'normal'
       elsif @status == 'changed'
         event_title = "Puppet changed resources on #{@msg_host}"
-        alert_type = "success"
-        event_priority = "normal"
-      elsif @status == "unchanged"
+        alert_type = 'success'
+        event_priority = 'normal'
+      elsif @status == 'unchanged'
         event_title = "Puppet ran on, and left #{@msg_host} unchanged"
-        alert_type = "success"
+        alert_type = 'success'
       else
         event_title = "Puppet ran on #{@msg_host}"
-        alert_type = "success"
+        alert_type = 'success'
       end
 
     else
@@ -90,49 +86,48 @@ def process
     end
 
     # Extract statuses
-    total_resource_count = self.resource_statuses.length
-    changed_resources    = self.resource_statuses.values.find_all {|s| s.changed }
-    failed_resources     = self.resource_statuses.values.find_all {|s| s.failed }
+    total_resource_count = resource_statuses.length
+    changed_resources    = resource_statuses.values.select { |s| s.changed }
+    failed_resources     = resource_statuses.values.select { |s| s.failed }
 
     # Little insert if we know the config
-    config_version_blurb = if defined?(self.configuration_version) then "applied version #{self.configuration_version} and" else "" end
+    config_version_blurb = defined?(configuration_version) ? "applied version #{configuration_version} and" : ''
 
     event_data << "Puppet #{config_version_blurb} changed #{pluralize(changed_resources.length, 'resource')} out of #{total_resource_count}."
 
     # List changed resources
-    if changed_resources.length > 0
+    unless changed_resources.empty?
       event_data << "\nThe resources that changed are:\n@@@\n"
-      changed_resources.each {|s| event_data << "#{s.title} in #{s.file}:#{s.line}\n" }
+      changed_resources.each { |s| event_data << "#{s.title} in #{s.file}:#{s.line}\n" }
       event_data << "\n@@@\n"
     end
 
     # List failed resources
-    if failed_resources.length > 0
+    unless failed_resources.empty?
       event_data << "\nThe resources that failed are:\n@@@\n"
-      failed_resources.each {|s| event_data << "#{s.title} in #{s.file}:#{s.line}\n" }
+      failed_resources.each { |s| event_data << "#{s.title} in #{s.file}:#{s.line}\n" }
       event_data << "\n@@@\n"
     end
 
     Puppet.debug "Sending metrics for #{@msg_host} to Datadog"
     @dog = Dogapi::Client.new(API_KEY, nil, nil, nil, nil, nil, API_URL)
     @dog.batch_metrics do
-      self.metrics.each { |metric,data|
-        data.values.each { |val|
-          name = "puppet.#{val[1].gsub(/ /, '_')}.#{metric}".downcase
+      metrics.each do |metric, data|
+        data.values.each do |val|
+          name = "puppet.#{val[1].tr(' ', '_')}.#{metric}".downcase
           value = val[2]
-          @dog.emit_point("#{name}", value, :host => "#{@msg_host}")
-        }
-      }
+          @dog.emit_point(name.to_s, value, host: @msg_host.to_s)
+        end
+      end
     end
 
     Puppet.debug "Sending events for #{@msg_host} to Datadog"
     @dog.emit_event(Dogapi::Event.new(event_data,
-                                      :msg_title => event_title,
-                                      :event_type => 'config_management.run',
-                                      :event_object => @msg_host,
-                                      :alert_type => alert_type,
-                                      :priority => event_priority,
-                                      :source_type_name => 'puppet'
-                                      ), :host => @msg_host)
+                                      msg_title: event_title,
+                                      event_type: 'config_management.run',
+                                      event_object: @msg_host,
+                                      alert_type: alert_type,
+                                      priority: event_priority,
+                                      source_type_name: 'puppet'), host: @msg_host)
   end
 end

manifests/tag5.pp

@@ -9,7 +9,7 @@
 
     if is_array($value){
       $tags = prefix($value, "${tag_name}:")
-      datadog_agent::tag{$tags: }
+      datadog_agent::tag5{$tags: }
     } else {
       if $value {
         concat::fragment{ "datadog tag ${tag_name}:${value}":

spec/classes/datadog_agent_integrations_activemq_xml_spec.rb

@@ -4,53 +4,61 @@
   context 'supported agents' do
     ALL_SUPPORTED_AGENTS.each do |agent_major_version|
       let(:pre_condition) { "class {'::datadog_agent': agent_major_version => #{agent_major_version}}" }
+
       if agent_major_version == 5
-        let(:conf_file) { "/etc/dd-agent/conf.d/activemq_xml.yaml" }
+        let(:conf_file) { '/etc/dd-agent/conf.d/activemq_xml.yaml' }
       else
         let(:conf_file) { "#{CONF_DIR}/activemq_xml.d/conf.yaml" }
       end
 
       context 'with default parameters' do
-        it { should compile }
+        it { is_expected.to compile }
       end
 
       context 'with password set' do
-        let(:params) {{
-          username: 'foo',
-          password: 'abc123',
-        }}
+        let(:params) do
+          {
+            username: 'foo',
+            password: 'abc123',
+          }
+        end
 
-        it { should compile.with_all_deps }
-        it { should contain_file(conf_file).with(
-          owner: DD_USER,
-          group: DD_GROUP,
-          mode: PERMISSIONS_PROTECTED_FILE,
-        )}
-        it { should contain_file(conf_file).that_requires("Package[#{PACKAGE_NAME}]") }
-        it { should contain_file(conf_file).that_notifies("Service[#{SERVICE_NAME}]") }
-        it { should contain_file(conf_file).with_content(/username: foo/) }
-        it { should contain_file(conf_file).with_content(/password: abc123/) }
+        it { is_expected.to compile.with_all_deps }
+        it {
+          is_expected.to contain_file(conf_file).with(
+            owner: DD_USER,
+            group: DD_GROUP,
+            mode: PERMISSIONS_PROTECTED_FILE,
+          )
+        }
+        it { is_expected.to contain_file(conf_file).that_requires("Package[#{PACKAGE_NAME}]") }
+        it { is_expected.to contain_file(conf_file).that_notifies("Service[#{SERVICE_NAME}]") }
+        it { is_expected.to contain_file(conf_file).with_content(%r{username: foo}) }
+        it { is_expected.to contain_file(conf_file).with_content(%r{password: abc123}) }
 
         context 'with default parameters' do
           it {
-            should contain_file(conf_file)
-                .with_content(%r{http://localhost:8161})
-                .with_content(%r{supress_errors: false})
-                .without_content(%r{detailed_queues:})
-                .without_content(%r{detailed_topics:})
-                .without_content(%r{detailed_subscribers:})
+            is_expected.to contain_file(conf_file)
+              .with_content(%r{http://localhost:8161})
+              .with_content(%r{supress_errors: false})
+              .without_content(%r{detailed_queues:})
+              .without_content(%r{detailed_topics:})
+              .without_content(%r{detailed_subscribers:})
           }
         end
 
         context 'with extra detailed parameters' do
-          let(:params) {{
-            supress_errors: true,
-            detailed_queues: %w(queue1 queue2),
-            detailed_topics: %w(topic1 topic2),
-            detailed_subscribers: %w(subscriber1 subscriber2),
-          }}
+          let(:params) do
+            {
+              supress_errors: true,
+              detailed_queues: ['queue1', 'queue2'],
+              detailed_topics: ['topic1', 'topic2'],
+              detailed_subscribers: ['subscriber1', 'subscriber2'],
+            }
+          end
+
           it {
-            should contain_file(conf_file)
+            is_expected.to contain_file(conf_file)
               .with_content(%r{http://localhost:8161})
               .with_content(%r{supress_errors: true})
               .with_content(%r{detailed_queues:.*\s+- queue1\s+- queue2})
@@ -60,28 +68,31 @@
         end
 
         context 'with instances set' do
-          let(:params) {{
-            instances: [
+          let(:params) do
+            {
+              instances: [
                 {
-                    'url'     => 'http://localhost:8161',
-                    'username' => 'joe',
-                    'password' => 'hunter1',
-                    'detailed_queues' => %w(queue1 queue2),
-                    'detailed_topics' => %w(topic1 topic2),
-                    'detailed_subscribers' => %w(subscriber1 subscriber2),
+                  'url' => 'http://localhost:8161',
+                  'username' => 'joe',
+                  'password' => 'hunter1',
+                  'detailed_queues' => ['queue1', 'queue2'],
+                  'detailed_topics' => ['topic1', 'topic2'],
+                  'detailed_subscribers' => ['subscriber1', 'subscriber2'],
                 },
                 {
-                    'url'     => 'http://remotehost:8162',
-                    'username' => 'moe',
-                    'password' => 'hunter2',
-                    'detailed_queues' => %w(queue3 queue4),
-                    'detailed_topics' => %w(topic3 topic4),
-                    'detailed_subscribers' => %w(subscriber3 subscriber4),
+                  'url' => 'http://remotehost:8162',
+                  'username' => 'moe',
+                  'password' => 'hunter2',
+                  'detailed_queues' => ['queue3', 'queue4'],
+                  'detailed_topics' => ['topic3', 'topic4'],
+                  'detailed_subscribers' => ['subscriber3', 'subscriber4'],
                 },
-            ],
-          }}
+              ],
+            }
+          end
+
           it {
-            should contain_file(conf_file)
+            is_expected.to contain_file(conf_file)
               .with_content(%r{url: http://localhost:8161})
               .without_content(%r{supress_errors:})
               .with_content(%r{username: joe})