Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rubocop -a modules/exploits/unix/local/ #14640

Merged

Conversation

bcoles
Copy link
Contributor

@bcoles bcoles commented Jan 21, 2021

Clean Rubocop output is a requirement for new code. It makes sense that existing code should also conform to the same standards. This prevents copypasta of "bad" code style.

This PR applies changes to a directory with a small number of modules as a test run. Eventually, it would be nice if rubocop returned no errors or warnings for the entire repository.

These changes were applied automatically with rubocop -a and manually reviewed for sanity.

These were changed manually:

-      'DefaultOptions' => {'WfsDelay' => 60 * 60 * 24} # 24h
+      'DefaultOptions' => { 'WfsDelay' => 24.hours.seconds.to_i }
-  def exploit(c = payload.encoded)
+  def exploit(cmd = payload.encoded)

If you're unhappy with these changes, or think these changes broke something, this is a good opportunity to address customization of existing Rubocop rules or adding new custom rules.

@adfoster-r7
Copy link
Contributor

Hey this PR looks good to me. Just for visibility I've just created a separate Github discussion for a Rubocop rollout: #14642

I highlighted some of the impact that applying Rubocop to the codebase would have and the overhead in keeping aligned with 'bleeding edge' linters - like in #14639. Would be interested to hear your thoughts 👍

@bcoles
Copy link
Contributor Author

bcoles commented Jan 21, 2021

Hey this PR looks good to me. Just for visibility I've just created a separate Github discussion for a Rubocop rollout: #14642

I highlighted some of the impact that applying Rubocop to the codebase would have and the overhead in keeping aligned with 'bleeding edge' linters - like in #14639. Would be interested to hear your thoughts +1

Bleeding edge linters should be included in the Rubocop config file. This prevents a bunch of warnings which may be confusing to contributors. Edit: It's also explicit. "hey we know about these rules but don't want them or haven't reviewed them yet."

When adding new cops, they could/should be set to false with a comment stating that they need to be reviewed. And then someone should actually review them.

In #14639 I enabled everything to see what breaks (and because none of the new linters appeared particularly awful).

Also, we currently ship a Gemfile.lock file with a specified version of Rubocop:

rubocop (1.8.1)

However the existing cops haven't been updated. For example, PR #14639 updates Metrics/LineLength to Layout/LineLength which was changed in Rubocop over a year ago.

If keeping up with Rubocop rules is problematic, we should resolve this by locking the version of Rubocop in the Gemfile.lock file, not by blatantly ignoring new rules or rule name changes and hoping the problem will go away.

Also, the Rubocop configuration file was never updated to specify Ruby version 2.5 instead of 2.4. Ruby 2.4 has been end of life for almost a year.

@gwillcox-r7 gwillcox-r7 self-assigned this Jan 22, 2021
@gwillcox-r7 gwillcox-r7 merged commit 72ef81d into rapid7:master Jan 22, 2021
@bcoles bcoles deleted the rubocop-modules-exploits-unix-local branch January 22, 2021 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants