Increase rubocop version to 0.51.x & remove Ruby 2.0 testing
#15
Conversation
|
Tests failed due to lots of rubocop failures. Working through them now :) |
bin/metrics-zookeeper-cluster.rb
Outdated
| @@ -49,6 +49,8 @@ class ZookeeperMetrics < Sensu::Plugin::Metric::CLI::Graphite | |||
| default: 2181, | |||
| proc: proc(&:to_i) | |||
|
|
|||
| # TODO: Remove below when Rubocop is updated (https://github.com/sensu-plugins/community/issues/77#issuecomment-345813238) | |||
| # rubocop:disable Style/CommentedKeyword | |||
There was a problem hiding this comment.
this should go on the same line as the offense to disable it inline.
There was a problem hiding this comment.
Oddly on this one, when disabling inline it raises another (possibly erroneous) warning:
bin/metrics-zookeeper-cluster.rb:53:159: W: Unnecessary disabling of Style/CommentedKeyword.
def follow_url(uri_str, agent = "sensu-plugins-zookeeper/#{SensuPluginsZookeeper::Version::VER_STRING}", max_attempts = 10, timeout = 10) # rubocop:disable Style/CommentedKeyword, Metrics/LineLength
I have instead changed it to disable this cop on just the function in question. Thoughts?
There was a problem hiding this comment.
😢 sounds like we have to choose one bad or the other. Go for it disabling the whole file.
There was a problem hiding this comment.
Actually the best solution would be to just break up the parameters on that line. That way nothing needs disabling and it actually reads better.
bin/metrics-zookeeper-cluster.rb
Outdated
| @@ -119,6 +121,8 @@ def run | |||
| timestamp = Time.now.to_i | |||
|
|
|||
| json = exhibitor_status | |||
|
|
|||
| # rubocop:disable Metrics/BlockLength | |||
There was a problem hiding this comment.
this should go on the same line as the offense to disable it inline.
| @@ -4,14 +4,14 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) | |||
| require 'date' | |||
| require_relative 'lib/sensu-plugins-zookeeper' | |||
|
|
|||
| Gem::Specification.new do |s| | |||
| Gem::Specification.new do |s| # rubocop:disable Metrics/BlockLength | |||
majormoses
changed the title
0.51.x & remove Ruby 2.0 testing
|
also just a minor nitpick your commit message and PR title (fixed) says |
|
@thomasriley any chance you can come back and rebase? I'd prefer if we lock it more closely to avoid rubocop randomly deciding to change a rule and now suddenly unrelated code to the PR starts failing lint. |
majormoses
force-pushed
the
fix/CVE-2017-8418
branch
from
c01926a to
326f95f
Compare
|
@thomasriley I rebased on your behalf and made a minor tweak. |
|
Assuming the tests pass now (they should) I think this is good to go, Ideally if you could squash to a single commit that would be best. If not I can do it for you. |
| @@ -16,3 +16,4 @@ mkmf.log | |||
| .DS_Store | |||
| .idea/* | |||
| *.gem | |||
| /vendor/ | |||
There was a problem hiding this comment.
probably better to use vendor/* as this will catch anything with vendor in its name (not that I can think of a great real use case that this blocks).
Pull Request Checklist
sensu-plugins/community#77
General
Update Changelog following the conventions laid out on Our CHANGELOG Guidelines
RuboCop passes
Existing tests pass
Purpose
Increase rubocop version for CVE.
Known Compatibility Issues
Now requires Ruby 2.1 and greater