From 63e2d2f2ec87d833408b7e8b6fb33e3f0fa0c804 Mon Sep 17 00:00:00 2001 From: Steve Kemp Date: Thu, 10 Aug 2017 10:17:51 +0300 Subject: Significant rubocop fixups. This merge-request contains almost entirely mechanical changes, with a few exceptions: * I changed `do_ipv4` and `do_ipv6` to `ipv4` and `ipv6` respectively. * This fixed a warning about normal-casing. * I changed a test-case to compare against both `Integer` and `Fixnum` * Suspect this is a ruby-versionism. The tests continue to pass, so I believe this is safe to merge, but of course it is still not 100%: lib/custodian/queue.rb:135:21: W: Assignment in condition - you probably meant to use ==. added = true ^ lib/custodian/protocoltest/ssl.rb:218:5: W: Do not shadow rescued Exceptions rescue OpenSSL::SSL::SSLError => err ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/custodian/protocoltest/ssl.rb:286:5: W: Do not shadow rescued Exceptions rescue OpenSSL::SSL::SSLError => err ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/custodian/protocoltest/http.rb:307:7: C: Assignment Branch Condition size for run_test is too high. [84.53/72] def run_test ^^^ lib/custodian/protocoltest/http.rb:307:7: C: Cyclomatic complexity for run_test is too high. [22/19] def run_test ^^^ lib/custodian/protocoltest/http.rb:307:7: C: Method has too many lines. [97/87] def run_test ... ^^^^^^^^^^^^ lib/custodian/protocoltest/http.rb:307:7: C: Perceived complexity for run_test is too high. [23/21] def run_test In short this takes care of _most_ of the warnings, but updates requiring significant code-change have not been applied. --- bin/multi-ping | 3 ++- lib/custodian/alerts/mauve.rb | 4 ++-- lib/custodian/parser.rb | 2 +- lib/custodian/protocoltest/http.rb | 16 ++++++++-------- lib/custodian/protocoltest/ping.rb | 12 ++++++------ lib/custodian/protocoltest/ssl.rb | 22 +++++++++++----------- lib/custodian/protocoltest/tcp.rb | 14 +++++++------- lib/custodian/queue.rb | 2 +- lib/custodian/util/ping.rb | 4 ++-- lib/custodian/util/prefix.rb | 12 ++++++------ t/test-custodian-parser.rb | 22 +++++++++++----------- t/test-custodian-settings.rb | 13 +++++++------ t/test-custodian-util-ping.rb | 2 +- t/test-custodian-util-timespan.rb | 2 +- t/test-rubocop.rb | 2 +- 15 files changed, 67 insertions(+), 65 deletions(-) diff --git a/bin/multi-ping b/bin/multi-ping index 6d905bb..1045635 100755 --- a/bin/multi-ping +++ b/bin/multi-ping @@ -14,7 +14,8 @@ manual = false opts = GetoptLong.new( ['--help', '-h', GetoptLong::NO_ARGUMENT], - ['--manual', '-m', GetoptLong::NO_ARGUMENT]) + ['--manual', '-m', GetoptLong::NO_ARGUMENT] +) begin opts.each do |opt, _arg| diff --git a/lib/custodian/alerts/mauve.rb b/lib/custodian/alerts/mauve.rb index 0aed390..bf4d08f 100644 --- a/lib/custodian/alerts/mauve.rb +++ b/lib/custodian/alerts/mauve.rb @@ -110,7 +110,7 @@ module Custodian # If we're Monday-Friday, between the start & end time, then # we're in the working day. # - if ((wday != 0) && (wday != 6)) && + if ((wday.nonzero?) && (wday != 6)) && (hour >= day_start && hour < day_end) working = true end @@ -229,7 +229,7 @@ module Custodian alert.id = Digest::SHA1.hexdigest(id_key) # Look for a subject-prefix - subject_prefix = Custodian::Util::Prefix.text() + subject_prefix = Custodian::Util::Prefix.text alert.subject = subject_prefix + subject alert.summary = "The #{test_type} test failed against #{test_host}" diff --git a/lib/custodian/parser.rb b/lib/custodian/parser.rb index 941a549..0f00e0d 100644 --- a/lib/custodian/parser.rb +++ b/lib/custodian/parser.rb @@ -96,7 +96,7 @@ module Custodian case response when Net::HTTPRedirection then - newURL = (response['location'] =~ /^http/) ? response['Location'] : uri_str + response['Location'] + newURL = response['location'] =~ /^http/ ? response['Location'] : uri_str + response['Location'] return(get_url_contents(newURL)) else return response.body diff --git a/lib/custodian/protocoltest/http.rb b/lib/custodian/protocoltest/http.rb index 0bf1a68..084e9aa 100644 --- a/lib/custodian/protocoltest/http.rb +++ b/lib/custodian/protocoltest/http.rb @@ -59,7 +59,7 @@ module Custodian # raise an alert, but if it is this false-error then we # will silently disable this test-run. # - def ignore_failure?( protocol ) + def ignore_failure?(protocol) # Get the hostname we're connecting to. u = URI.parse(@url) @@ -116,7 +116,7 @@ module Custodian # # "ips" being empty because the DNS failure was genuine # - return ( ! ips.empty? ) + return (!ips.empty?) end @@ -189,8 +189,8 @@ module Custodian # # Save username/password if they were specified # - @username = u.user if ( u.user ) - @password = u.password if ( u.password ) + @username = u.user if (u.user) + @password = u.password if (u.password) # # Expected status @@ -278,7 +278,7 @@ module Custodian # Do we have basic auth? # def basic_auth? - ( @username.nil? == false ) && ( @password.nil? == false ) + (@username.nil? == false) && (@password.nil? == false) end # @@ -341,7 +341,7 @@ module Custodian # If we're running with HTTP-basic-auth we should remove # the username/password from the URL we're passing to curb. # - if ( basic_auth? ) + if (basic_auth?) u = URI.parse(test_url) u.user = nil u.password = nil @@ -365,7 +365,7 @@ module Custodian # # Should we use HTTP basic-auth? # - if basic_auth? + if basic_auth? c.http_auth_types = :basic c.username = basic_auth_username c.password = basic_auth_password @@ -421,7 +421,7 @@ module Custodian rescue Curl::Err::TooManyRedirectsError errors << "#{protocol_msg}: More than 10 redirections." rescue Curl::Err::HostResolutionError => x - resolution_errors << "#{protocol_msg}: #{x.class}: #{x.message}\n #{x.backtrace.join("\n ")}." unless ignore_failure?( resolve_mode) + resolution_errors << "#{protocol_msg}: #{x.class}: #{x.message}\n #{x.backtrace.join("\n ")}." unless ignore_failure?(resolve_mode) rescue => x errors << "#{protocol_msg}: #{x.class}: #{x.message}\n #{x.backtrace.join("\n ")}." diff --git a/lib/custodian/protocoltest/ping.rb b/lib/custodian/protocoltest/ping.rb index 44da04d..3ee722e 100644 --- a/lib/custodian/protocoltest/ping.rb +++ b/lib/custodian/protocoltest/ping.rb @@ -112,17 +112,17 @@ module Custodian # # Both types? # - do_ipv6 = true - do_ipv4 = true + ipv6 = true + ipv4 = true # # Allow the test to disable one/both # if @line =~ /ipv4_only/ - do_ipv6 = false + ipv6 = false end if @line =~ /ipv6_only/ - do_ipv4 = false + ipv4 = false end # @@ -133,11 +133,11 @@ module Custodian timeout(period) do Resolv::DNS.open do |dns| - if do_ipv4 + if ipv4 ress = dns.getresources(@host, Resolv::DNS::Resource::IN::A) ress.map { |r| ips.push(r.address.to_s) } end - if do_ipv6 + if ipv6 ress = dns.getresources(@host, Resolv::DNS::Resource::IN::AAAA) ress.map { |r| ips.push(r.address.to_s) } end diff --git a/lib/custodian/protocoltest/ssl.rb b/lib/custodian/protocoltest/ssl.rb index 82754c6..6bc1a04 100644 --- a/lib/custodian/protocoltest/ssl.rb +++ b/lib/custodian/protocoltest/ssl.rb @@ -162,25 +162,25 @@ class SSLCheck # is valid. # def certificate_fallback - cert = "" + cert = '' in_cert = false # Run the command. out = `echo "" | openssl s_client -servername #{uri.host} -connect #{uri.host}:#{uri.port} 2>/dev/null` # For each line of the output - out.split( /[\r\n]/ ).each do |line| + out.split(/[\r\n]/).each do |line| # Are we in a certificate? - in_cert = true if ( line =~ /BEGIN CERT/ ) + in_cert = true if (line =~ /BEGIN CERT/) # If so append the line. - if ( in_cert ) + if (in_cert) cert += line cert += "\n" end # Are we at the end? - in_cert = false if ( line =~ /END CERT/ ) + in_cert = false if (line =~ /END CERT/) end # Return the certificate @@ -248,10 +248,10 @@ class SSLCheck if self.certificate.nil? # Use our fallback method. - fallback = certificate_fallback() + fallback = certificate_fallback # If we failed to fetch it then we cannot do anything useful. - if ( fallback.nil? ) + if (fallback.nil?) self.errors << verbose("Failed to fetch certificate for #{self.domain}") return nil else @@ -373,9 +373,9 @@ class SSLCheck # signature is valid, because we're missing the bundle that # the remote server should have sent us. # - if ( @fallback ) + if (@fallback) verbose "Skipping certificate signature validation for #{self.domain} because fallback SSL-certificate had to be used and we think we'll fail" - return true; + return true end unless self.tests.include?(:signature) @@ -514,7 +514,7 @@ module Custodian return Custodian::TestResult::TEST_SKIPPED end - s = SSLCheck.new(@host,@expiry_days) + s = SSLCheck.new(@host, @expiry_days) result = s.verify if true == result @@ -522,7 +522,7 @@ module Custodian return Custodian::TestResult::TEST_PASSED elsif result.nil? puts("SSL Verification returned no result #{@host}") - @error = "SSL Verification for #{@host} failed - TLS negotiation failure?\n"; + @error = "SSL Verification for #{@host} failed - TLS negotiation failure?\n" @error += s.errors.join("\n") return Custodian::TestResult::TEST_FAILED else diff --git a/lib/custodian/protocoltest/tcp.rb b/lib/custodian/protocoltest/tcp.rb index 0081a18..c62303f 100644 --- a/lib/custodian/protocoltest/tcp.rb +++ b/lib/custodian/protocoltest/tcp.rb @@ -113,7 +113,7 @@ module Custodian # reset the error, in case we were previously executed. @error = nil - (run_test_internal(@host, @port, @banner, (! @banner.nil?))) + (run_test_internal(@host, @port, @banner, (!@banner.nil?))) end @@ -165,17 +165,17 @@ module Custodian # # Both types? # - do_ipv6 = true - do_ipv4 = true + ipv6 = true + ipv4 = true # # Allow the test to disable one/both # if @line =~ /ipv4_only/ - do_ipv6 = false + ipv6 = false end if @line =~ /ipv6_only/ - do_ipv4 = false + ipv4 = false end @@ -188,11 +188,11 @@ module Custodian Resolv::DNS.open do |dns| - if do_ipv4 + if ipv4 ress = dns.getresources(host, Resolv::DNS::Resource::IN::A) ress.map { |r| ips.push(r.address.to_s) } end - if do_ipv6 + if ipv6 ress = dns.getresources(host, Resolv::DNS::Resource::IN::AAAA) ress.map { |r| ips.push(r.address.to_s) } end diff --git a/lib/custodian/queue.rb b/lib/custodian/queue.rb index 9c33825..ceb84b5 100644 --- a/lib/custodian/queue.rb +++ b/lib/custodian/queue.rb @@ -3,7 +3,7 @@ # # Without this we cannot connect to our queue. # -%w( redis ).each do |library| +%w(redis).each do |library| begin require library rescue LoadError diff --git a/lib/custodian/util/ping.rb b/lib/custodian/util/ping.rb index 21836ac..f0aa15b 100644 --- a/lib/custodian/util/ping.rb +++ b/lib/custodian/util/ping.rb @@ -45,7 +45,7 @@ module Custodian # Does the hostname resolve to an IPv4 address? # def is_ipv4? - if (! @resolved.nil?) && (@resolved =~ /^([0-9]+).([0-9]+).([0-9]+).([0-9]+)$/) + if (!@resolved.nil?) && (@resolved =~ /^([0-9]+).([0-9]+).([0-9]+).([0-9]+)$/) true else false @@ -57,7 +57,7 @@ module Custodian # Does the hostname resolve to an IPv6 address? # def is_ipv6? - if (! @resolved.nil?) && (@resolved =~ /^([a-f0-9:]+)$/i) + if (!@resolved.nil?) && (@resolved =~ /^([a-f0-9:]+)$/i) true else false diff --git a/lib/custodian/util/prefix.rb b/lib/custodian/util/prefix.rb index 743a422..9ac027c 100644 --- a/lib/custodian/util/prefix.rb +++ b/lib/custodian/util/prefix.rb @@ -14,25 +14,25 @@ module Custodian # # Return the custom-prefix to use, if any. # - def Prefix.text() + def Prefix.text # Default to no prefix. - default = "" + default = '' # Look for matches - last one wins. - Dir.glob( "/store/clients/*/custodian-prefix.cfg" ).each do |file| + Dir.glob('/store/clients/*/custodian-prefix.cfg').each do |file| begin - default = File.read( file ) + default = File.read(file) rescue Errno::EACCES # Permission-denied. end end # Remove any newline characters - default.gsub!( /[\r\n]/, '' ) + default.gsub!(/[\r\n]/, '') # Truncate, if required. max = 32 - default = default[0...max] if ( default.length > max ) + default = default[0...max] if (default.length > max) default end diff --git a/t/test-custodian-parser.rb b/t/test-custodian-parser.rb index 2239503..62a39dc 100755 --- a/t/test-custodian-parser.rb +++ b/t/test-custodian-parser.rb @@ -438,15 +438,15 @@ EOF # test data # data = { - 'http://bob:steve@example must run http.' => - { username: 'bob', password: 'steve'}, + 'http://bob:steve@example must run http.' => + { :username => 'bob', :password => 'steve' }, 'http://stee\':steve@example must run http.' => - { username: 'stee\'', password: 'steve'}, + { :username => 'stee\'', :password => 'steve' }, 'http://e\'e:pa$$w0rd@example must run http.' => - { username: 'e\'e', password: 'pa$$w0rd'}, + { :username => 'e\'e', :password => 'pa$$w0rd' } } - data.each do |str, hash | + data.each do |str, hash| assert_nothing_raised do # @@ -461,8 +461,8 @@ EOF # There should be auth-enabled assert(obj[0].basic_auth?) - assert(obj[0].basic_auth_username == hash[:username] ) - assert(obj[0].basic_auth_password == hash[:password] ) + assert(obj[0].basic_auth_username == hash[:username]) + assert(obj[0].basic_auth_password == hash[:password]) end end @@ -488,7 +488,7 @@ EOF # # Test the parser with this text # - expiries.each do |str,days| + expiries.each do |str, days| assert_nothing_raised do # @@ -505,13 +505,13 @@ EOF # Test both of them to make sure we got our expiry period. obj.each do |x| - if ( x.class.name =~ /SSL/ ) - found_days = x.expiry_days + if (x.class.name =~ /SSL/) + found_days = x.expiry_days end end # Ensure we did find a match. - assert(found_days != -1 ) + assert(found_days != -1) assert(found_days == days) end diff --git a/t/test-custodian-settings.rb b/t/test-custodian-settings.rb index 74f881b..1630859 100755 --- a/t/test-custodian-settings.rb +++ b/t/test-custodian-settings.rb @@ -47,36 +47,37 @@ class TestConfigurationSingleton < Test::Unit::TestCase # retry delay - probably unset. a = settings.retry_delay - assert(a.class == Fixnum) + assert(a.class == Integer || + a.class == Fixnum) # store a number settings._store('retry_delay', 5) a = settings.retry_delay - assert(a.class == Fixnum) + assert(a.class == Integer || a.class == Fixnum ) assert(a == 5) # store a string settings._store('retry_delay', '35') a = settings.retry_delay - assert(a.class == Fixnum) + assert(a.class == Integer || a.class == Fixnum ) assert(a == 35) # timeout - probably unset. a = settings.timeout - assert(a.class == Fixnum) + assert(a.class == Integer || a.class == Fixnum ) # store a number settings._store('timeout', 5) a = settings.timeout - assert(a.class == Fixnum) + assert(a.class == Integer || a.class == Fixnum ) assert(a == 5) # store a string settings._store('timeout', '35') a = settings.timeout - assert(a.class == Fixnum) + assert(a.class == Integer || a.class == Fixnum ) assert(a == 35) diff --git a/t/test-custodian-util-ping.rb b/t/test-custodian-util-ping.rb index 99b61c1..d7d16b8 100755 --- a/t/test-custodian-util-ping.rb +++ b/t/test-custodian-util-ping.rb @@ -63,7 +63,7 @@ class TestPingUtil < Test::Unit::TestCase # Test lookup of hosts that don't work # def test_lookup_fail - %w( tessf.dfsdf.sdf.sdfsdf fdsfkljflj3.fdsfds.f3.dfs ).each do |name| + %w(tessf.dfsdf.sdf.sdfsdf fdsfkljflj3.fdsfds.f3.dfs).each do |name| assert_nothing_raised do helper = Custodian::Util::Ping.new(name) diff --git a/t/test-custodian-util-timespan.rb b/t/test-custodian-util-timespan.rb index fec079f..5098589 100755 --- a/t/test-custodian-util-timespan.rb +++ b/t/test-custodian-util-timespan.rb @@ -214,7 +214,7 @@ class TestTimeSpanUtil < Test::Unit::TestCase # def test_wrap_around - for h in 00..23 + for h in 0o0..23 assert_equal(1, Custodian::Util::TimeSpan.to_hours(h, h).size) end diff --git a/t/test-rubocop.rb b/t/test-rubocop.rb index 57457b7..cb597e8 100755 --- a/t/test-rubocop.rb +++ b/t/test-rubocop.rb @@ -20,7 +20,7 @@ class TestRubocop < Test::Unit::TestCase cli = RuboCop::CLI.new result = cli.run - assert(result == 0, 'No errors found') + assert(result.zero?, 'No errors found') rescue LoadError => ex if methods.include?(:skip) -- cgit v1.2.1