adds rubocop test and fixes code to pass it

luisdavim commented 7 years ago

Description

This adds rubocop tests to the Rakefile and fixes the code to pass them. Please review this carefully, the PR is a bit long.

Motivation and Context

I was adding a new feature to lib/sensu-plugin/metric/cli.rb and after it I decided to run rubocop. I was surprised with close to 300 offences so, I've reverted my changes (I'll create a separate PR for that after this one) and started fixing the offences.

How Has This Been Tested?

I've ran the existing unit/integration tests

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have updated the Changelog following the conventions laid out on Keep A Changelog

Known Caveats

I've disabled some cops either in .rubocop.yml or using comments, we can review these in another iteration.

luisdavim commented 7 years ago

Sorry, I must confess, after dealing with the first ~100 offences I got bored and did use some automation and I might have overlooked some changes. I think my new commit fixes everything you pointed out.

Thanks for the review.

majormoses commented 7 years ago

@luisdavim completely understand I'd have done the same and this is why we have code review. As this is a fairly large change and there is possibility some hidden land mines we definitely need to do some serious testing and I tried to find the hidden land mines best I could.

majormoses commented 7 years ago

@luisdavim it looks good without having done any testing, do you have any time to do some testing and validate we did not break anything I might have missed? If not I can try to do some testing when I have some time.

luisdavim commented 7 years ago

If this can wait til Monday I can do it on Monday...

majormoses commented 7 years ago

It can certainly wait.

luisdavim commented 7 years ago

Ok, I'll test it on Monday.

luisdavim commented 7 years ago

Hi, I've built a plugin (sensu-plugins-dcos) using this ran some tests and didn't find any problems. @majormoses do you have any suggestions on how to test this in any other way?

luisdavim commented 7 years ago

any ideas @cwjohnston ?

luisdavim commented 7 years ago

Hi, any idea when we can have this merged? I have other changes I would like to push but I would prefer to have this one done first.

majormoses commented 7 years ago

I am just gonna release this and if it causes issues we can revert.

majormoses commented 7 years ago

released: https://rubygems.org/gems/sensu-plugin/versions/2.2.0

sensu-plugins / sensu-plugin