Merge pull request #285 from riemann/rubocop

Enable the `rubocop-rake` and `rubocop-rspec` gems

.rubocop.yml

@@ -1,14 +1,22 @@
 ---
 AllCops:
+  NewCops: enable
   Exclude:
     - lib/riemann/tools/*_parser.tab.rb
     - vendor/bundle/**/*
+require:
+  - rubocop-rake
+  - rubocop-rspec
+Gemspec/RequireMFA:
+  Enabled: false
 Gemspec/RequiredRubyVersion:
   Enabled: false
 Layout/HashAlignment:
   EnforcedHashRocketStyle: table
 Layout/LineLength:
   Enabled: false
+Lint/EmptyBlock:
+  Enabled: false
 Metrics/AbcSize:
   Enabled: false
 Metrics/BlockLength:
@@ -28,10 +36,22 @@ Naming/MethodParameterName:
     - az # availability zone
     - id
     - lb # load balancer
+RSpec/ExampleLength:
+  Enabled: false
+RSpec/MultipleExpectations:
+  Enabled: false
+RSpec/NamedSubject:
+  Enabled: false
+RSpec/NestedGroups:
+  Enabled: false
+RSpec/SubjectStub:
+  Enabled: false
 Style/Documentation:
   Enabled: false
 Style/HashSyntax:
   EnforcedShorthandSyntax: never
+Style/NegatedIfElseCondition:
+  Enabled: false
 Style/TrailingCommaInArguments:
   EnforcedStyleForMultiline: consistent_comma
 Style/TrailingCommaInArrayLiteral:

Gemfile

@@ -4,3 +4,24 @@ source 'https://rubygems.org'
 
 # Specify your gem's dependencies in riemann-tools.gemspec
 gemspec
+
+gem 'github_changelog_generator'
+gem 'racc'
+gem 'rake'
+gem 'rspec'
+gem 'rubocop'
+gem 'rubocop-rake'
+gem 'rubocop-rspec'
+gem 'sinatra'
+gem 'webrick'
+
+if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')
+  # XXX: Needed for Ruby 2.6 compatibility
+  #
+  # With Ruby 2.6 an older version of rakup is installed that cause other gems
+  # to be installed with a legacy version.
+  #
+  # Because rakup is only needed when using rack 3, we can just ignore this
+  # with Ruby 2.6.
+  gem 'rackup'
+end

Rakefile

@@ -20,7 +20,7 @@ task :rbuild do
   end
 end
 
-task build: :gen_parser
+Rake::Task['build'].enhance(['gen_parser'])
 
 desc 'Generate parsers'
 task gen_parser: [

bin/riemann-wrapper

@@ -116,7 +116,7 @@ if ARGV.size == 1
   end
 
   require 'yaml'
-  config = YAML.safe_load(File.read(ARGV[0]))
+  config = YAML.safe_load_file(ARGV[0])
 
   arguments = split_options(config['options'])
   config['tools'].each do |tool|

lib/riemann/tools.rb

@@ -51,10 +51,10 @@ def options
     alias opts options
 
     def attributes
-      @attributes ||= Hash[options[:attribute].map do |attr|
-        k, v = attr.split(/=/)
+      @attributes ||= options[:attribute].to_h do |attr|
+        k, v = attr.split('=')
         [k, v] if k && v
-      end]
+      end
     end
 
     def report(event)

lib/riemann/tools/bench.rb

@@ -19,8 +19,8 @@ def initialize
       end
 
       def evolve(state)
-        m = state[:metric] + (rand - 0.5) * 0.1
-        m = [[0, m].max, 1].min
+        m = state[:metric] + ((rand - 0.5) * 0.1)
+        m = m.clamp(0, 1)
 
         s = case m
             when 0...0.75

lib/riemann/tools/diskstats.rb

@@ -16,7 +16,7 @@ def initialize
 
       def state
         f = File.read('/proc/diskstats')
-        state = f.split("\n").reject { |d| d =~ /(ram|loop)/ }.each_with_object({}) do |line, s|
+        state = f.split("\n").grep_v(/(ram|loop)/).each_with_object({}) do |line, s|
           next unless line =~ /^(?:\s+\d+){2}\s+([\w\d-]+) (.*)$/
 
           dev = Regexp.last_match(1)
@@ -43,13 +43,13 @@ def state
         # Filter interfaces
         if (is = opts[:devices])
           state = state.select do |service, _value|
-            is.include? service.split(' ').first
+            is.include? service.split.first
           end
         end
 
         if (ign = opts[:ignore_devices])
           state = state.reject do |service, _value|
-            ign.include? service.split(' ').first
+            ign.include? service.split.first
           end
         end
 
@@ -80,7 +80,7 @@ def tick
             next unless service =~ /io time$/
 
             report(
-              service: "diskstats #{service.gsub(/time/, 'util')}",
+              service: "diskstats #{service.gsub('time', 'util')}",
               metric: (delta.to_f / (opts[:interval] * 1000)),
               state: 'ok',
             )

lib/riemann/tools/health.rb

@@ -188,7 +188,7 @@ def linux_load
       end
 
       def linux_memory
-        m = File.read('/proc/meminfo').split(/\n/).each_with_object({}) do |line, info|
+        m = File.read('/proc/meminfo').split("\n").each_with_object({}) do |line, info|
           x = line.split(/:?\s+/)
           # Assume kB...
           info[x[0]] = x[1].to_i

lib/riemann/tools/http_check.rb

@@ -37,10 +37,15 @@ def initialize
         @resolve_queue = Queue.new
         @work_queue = Queue.new
 
+        @resolvers = []
+        @workers = []
+
         opts[:resolvers].times do
-          Thread.new do
+          @resolvers << Thread.new do
             loop do
               uri = @resolve_queue.pop
+              Thread.exit unless uri
+
               host = uri.host
 
               addresses = Resolv::DNS.new.getaddresses(host)
@@ -59,9 +64,11 @@ def initialize
         end
 
         opts[:workers].times do
-          Thread.new do
+          @workers << Thread.new do
             loop do
               uri, addresses = @work_queue.pop
+              Thread.exit unless uri
+
               test_uri_addresses(uri, addresses)
             end
           end
@@ -70,6 +77,23 @@ def initialize
         super
       end
 
+      # Under normal operation, we have a single instance of this class for the
+      # lifetime of the process.  But when testing, we create a new instance
+      # for each test, each with its resolvers and worker threads.  The test
+      # process may end-up with a lot of running threads, hitting the OS limit
+      # of max threads by process and being unable to create more thread:
+      #
+      # ThreadError: can't create Thread: Resource temporarily unavailable
+      #
+      # To avoid this situation, we provide this method.
+      def shutdown
+        @resolve_queue.close
+        @resolvers.map(&:join)
+
+        @work_queue.close
+        @workers.map(&:join)
+      end
+
       def tick
         report(
           service: 'riemann http-check resolvers utilization',
@@ -265,8 +289,8 @@ def report_http_endpoint_max_redirects(http, uri)
       end
 
       def latency_state(name, latency)
-        critical_threshold = opts["#{name}_latency_critical".to_sym]
-        warning_threshold = opts["#{name}_latency_warning".to_sym]
+        critical_threshold = opts[:"#{name}_latency_critical"]
+        warning_threshold = opts[:"#{name}_latency_warning"]
 
         return if critical_threshold.zero? || warning_threshold.zero?
 

lib/riemann/tools/net.rb

@@ -118,13 +118,7 @@ def tick
 
             delta = metric - @old_state[service]
             svc_state = case service
-                        when /drop$/
-                          if delta.positive?
-                            'warning'
-                          else
-                            'ok'
-                          end
-                        when /errs$/
+                        when /drop$/, /errs$/
                           if delta.positive?
                             'warning'
                           else

lib/riemann/tools/nginx_status.rb

@@ -39,13 +39,13 @@ def initialize
       end
 
       def state(key, value)
-        if opts.key? "#{key}_critical".to_sym
-          critical_threshold = opts["#{key}_critical".to_sym]
+        if opts.key? :"#{key}_critical"
+          critical_threshold = opts[:"#{key}_critical"]
           return 'critical' if critical_threshold.positive? && (value >= critical_threshold)
         end
 
-        if opts.key? "#{key}_warning".to_sym
-          warning_threshold = opts["#{key}_warning".to_sym]
+        if opts.key? :"#{key}_warning"
+          warning_threshold = opts[:"#{key}_warning"]
           return 'warning' if warning_threshold.positive? && (value >= warning_threshold)
         end
 

riemann-tools.gemspec

@@ -40,14 +40,4 @@ Gem::Specification.new do |spec|
   spec.add_runtime_dependency 'json', '>= 1.8'
   spec.add_runtime_dependency 'optimist', '~> 3.0', '>= 3.0.0'
   spec.add_runtime_dependency 'riemann-client', '~> 1.1'
-
-  spec.add_development_dependency 'github_changelog_generator'
-  spec.add_development_dependency 'racc'
-  spec.add_development_dependency 'rake'
-  spec.add_development_dependency 'rspec'
-  spec.add_development_dependency 'rubocop'
-  spec.add_development_dependency 'rubocop-rake'
-  spec.add_development_dependency 'rubocop-rspec'
-  spec.add_development_dependency 'sinatra'
-  spec.add_development_dependency 'webrick'
 end

spec/riemann/tools/haproxy_spec.rb

@@ -3,7 +3,7 @@
 require 'riemann/tools/haproxy'
 
 RSpec.describe Riemann::Tools::Haproxy do
-  context('#tick') do
+  describe('#tick') do
     before do
       ARGV.replace(['--stats-url', 'http://localhost'])
 

spec/riemann/tools/health_spec.rb

@@ -3,7 +3,7 @@
 require 'riemann/tools/health'
 
 RSpec.describe Riemann::Tools::Health do
-  context('#disks') do
+  describe('#disks') do
     before do
       allow(subject).to receive(:df).and_return(<<~OUTPUT)
         Filesystem                         512-blocks       Used      Avail Capacity  Mounted on
@@ -66,7 +66,7 @@
     end
   end
 
-  context '#bsd_swap' do
+  describe '#bsd_swap' do
     context 'with swap devices' do
       before do
         allow(subject).to receive(:`).with('swapinfo').and_return(<<~OUTPUT)
@@ -99,7 +99,7 @@
     end
   end
 
-  context '#linux_swap' do
+  describe '#linux_swap' do
     context 'with swap devices' do
       before do
         allow(File).to receive(:read).with('/proc/swaps').and_return(<<~OUTPUT)
@@ -115,6 +115,7 @@
         expect(subject).to have_received(:report_pct).with(:swap, 0.339623244451355, 'used')
       end
     end
+
     context 'without swap devices' do
       before do
         allow(File).to receive(:read).with('/proc/swaps').and_return(<<~OUTPUT)
@@ -130,7 +131,7 @@
     end
   end
 
-  context '#bsd_uptime' do
+  describe '#bsd_uptime' do
     context 'when given unexpected data' do
       before do
         allow(subject).to receive(:`).with('uptime').and_return(<<~DOCUMENT)

spec/riemann/tools/http_check_spec.rb

@@ -1,7 +1,13 @@
 # frozen_string_literal: true
 
 require 'openssl'
-require 'rack/handler/webrick'
+begin
+  require 'rackup/handler/webrick'
+rescue LoadError
+  # XXX: Needed for Ruby 2.6 compatibility
+  # Moved to the rackup gem in recent versions
+  require 'rack/handler/webrick'
+end
 require 'sinatra/base'
 require 'webrick'
 require 'webrick/https'
@@ -65,6 +71,7 @@ def protected!
 RSpec.describe Riemann::Tools::HttpCheck, if: Gem::Version.new(RUBY_VERSION) >= Gem::Version.new(Riemann::Tools::HttpCheck::REQUIRED_RUBY_VERSION) do
   describe '#endpoint_name' do
     subject { described_class.new.endpoint_name(address, port) }
+
     let(:port) { 443 }
 
     context 'when using an IPv4 address' do
@@ -92,6 +99,7 @@ def protected!
     let(:http_timeout) { 2.0 }
 
     context 'when using unencrypted http' do
+      # rubocop:disable RSpec/BeforeAfterAll, RSpec/InstanceVariable
       before(:all) do
         server_options = {
           Port: 0,
@@ -109,8 +117,13 @@ def protected!
       after(:all) do
         @server&.shutdown
       end
+      # rubocop:enable RSpec/BeforeAfterAll, RSpec/InstanceVariable
 
-      let(:test_webserver_port) { @server.config[:Port] }
+      after do
+        subject.shutdown
+      end
+
+      let(:test_webserver_port) { @server.config[:Port] } # rubocop:disable RSpec/InstanceVariable
 
       let(:reported_uri) { uri }
 
@@ -236,6 +249,7 @@ def protected!
     end
 
     context 'when using encrypted https' do
+      # rubocop:disable RSpec/BeforeAfterAll, RSpec/InstanceVariable
       before(:all) do
         server_options = {
           Port: 0,
@@ -257,6 +271,7 @@ def protected!
       after(:all) do
         @server&.shutdown
       end
+      # rubocop:enable RSpec/BeforeAfterAll, RSpec/InstanceVariable
 
       context 'with an encrypted uri' do
         let(:uri) { URI("https://example.com:#{test_webserver_port}/") }

spec/riemann/tools/md_spec.rb

@@ -3,7 +3,7 @@
 require 'riemann/tools/md'
 
 RSpec.describe Riemann::Tools::Md do
-  context('#tick') do
+  describe('#tick') do
     context 'when all md devices are healthy' do
       before do
         allow(File).to receive(:read).with('/proc/mdstat').and_return(File.read('spec/fixtures/mdstat/example-8'))