Page MenuHomePhabricator

Setup rubocop for operations/puppet ruby code lints
Closed, ResolvedPublic

Description

We have a good bunch of ruby code written by people who hate ruby, and apparently rubocop (https://github.com/bbatsov/rubocop) would've caught things like T101913. This analyzer is already used to check browser tests written in ruby at the foundation in other repositories.

Related Objects

Event Timeline

yuvipanda updated the task description. (Show Details)
yuvipanda raised the priority of this task from to Needs Triage.
yuvipanda added a subscriber: yuvipanda.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2015, 5:15 PM
EBernhardson updated the task description. (Show Details)Jun 10 2015, 5:16 PM
EBernhardson set Security to None.

We would likely have to setup some reduced rule sets though, rubocop also complains about some things (such as methods more than 10 lines) that people who already hate ruby would probably hate even more :)

@zeljkofilipin and @dduvall are our Ruby gurus :-}

In zuul/layout.yaml that should all be about adding to operations/puppet the generic job bundle-rubocop. And of course make sure it passes before deploying or ops will end up being blocked.

hashar triaged this task as Normal priority.Jun 10 2015, 8:42 PM

Will do next week.

Change 218389 had a related patch set uploaded (by Zfilipin):
The basic RuboCop configuration

https://gerrit.wikimedia.org/r/218389

Talked with @yuvipanda on IRC today, we will try to pair on rubocop during wikimania with him. This patch should be ready by then.

Change 224850 had a related patch set uploaded (by Zfilipin):
Run RuboCop experimentally for operations/puppet

https://gerrit.wikimedia.org/r/224850

Change 224850 merged by jenkins-bot:
Run RuboCop experimentally for operations/puppet

https://gerrit.wikimedia.org/r/224850

Zuul commit is merged and deployed:

https://gerrit.wikimedia.org/r/#/c/224850/

Rubocop job is green and in experimental pipeline:

https://gerrit.wikimedia.org/r/#/c/218389/
https://integration.wikimedia.org/ci/job/bundle-rubocop/612/console

The only thing that should be done is somebody reviewing and merging the puppet change. :)

I will try to grab @yuvipanda during the hackathon to introduce him to rubocop.

zeljkofilipin renamed this task from Setup rubycop for operations/puppet ruby code lints to Setup rubocop for operations/puppet ruby code lints.Jul 15 2015, 8:22 PM

Change 225238 had a related patch set uploaded (by Zfilipin):
Fixed Style/TrailingWhitespace RuboCop offense

https://gerrit.wikimedia.org/r/225238

Change 218389 merged by Yuvipanda:
rubocop: Add basic configuration

https://gerrit.wikimedia.org/r/218389

Change 225238 merged by Yuvipanda:
rubocop: Fixed Style/TrailingWhitespace offense

https://gerrit.wikimedia.org/r/225238

@faidon I have left a comment in gerrit, but I am not sure if you will notice it there, so also commenting here.

Commit 6a5a060e5f075dbd0c17d0d8a8082f307b7b9c47
Author Faidon Liambotis <faidon@wikimedia.org> Jul 24, 2015 10:05 PM
Committer Faidon Liambotis <faidon@wikimedia.org> Jul 24, 2015 10:05 PM
Revert "rubocop: Fixed Style/TrailingWhitespace offense"

This is all for third-party code. We don't own this and we
should not unnecessarily deviate from upstream for e.g.
stylistic reasons.

This reverts commit c259aed94be6e91001bafb77efb95c33bf7ac1a3.

Change-Id: I9768145d0ec84d6fd33c9ae4b7e1911dacefb43a

The commit touches code in modules/mysql and modules/stdlib. Should rubocop ignore those folders? Are there more folders with upstream code to ignore?

@hashar: any idea on which folders contain third party code?

Andrew added a subscriber: Andrew.Aug 25 2015, 4:49 PM
greg added a subscriber: greg.Aug 25 2015, 4:50 PM

@hashar: any idea on which folders contain third party code?

redirect this question to someone in Ops :)

$ git submodule status
-76a5e8627659197f81a4e5240c3eb4fe01cb9888 modules/cdh
-e4c66b04b3f4df82569f4d14d0cf74b6fb79e57d modules/jmxtrans
-b75128da275503c9523e7ec718e2003c8106da63 modules/kafka
-0a8d5ab8ca020d322d101930286e4b856c012bf2 modules/kafkatee
-02cb5f6e9c6b960b4c437ed52d6da921e499ff55 modules/mariadb
-1cf395818ff40e58b9206a6d73a1cce8770aa389 modules/nginx
-c8771d8cda5309362eb1fb37d0d4d4f923a59e7d modules/varnishkafka
-51be23f1843cef2e69d5f0a23ae116d1fc04ef2d modules/wikimetrics
-6e3a24b08327f61bb09a9924e76e51c09180b3d9 modules/zookeeper
$ git submodule status
-76a5e8627659197f81a4e5240c3eb4fe01cb9888 modules/cdh
-e4c66b04b3f4df82569f4d14d0cf74b6fb79e57d modules/jmxtrans
-b75128da275503c9523e7ec718e2003c8106da63 modules/kafka
-0a8d5ab8ca020d322d101930286e4b856c012bf2 modules/kafkatee
-02cb5f6e9c6b960b4c437ed52d6da921e499ff55 modules/mariadb
-1cf395818ff40e58b9206a6d73a1cce8770aa389 modules/nginx
-c8771d8cda5309362eb1fb37d0d4d4f923a59e7d modules/varnishkafka
-51be23f1843cef2e69d5f0a23ae116d1fc04ef2d modules/wikimetrics
-6e3a24b08327f61bb09a9924e76e51c09180b3d9 modules/zookeeper

AFAIK those are puppet modules maintained by Wikimedia. Maybe the job should be made to not process submodules?

greg added a comment.Aug 31 2015, 3:10 PM

Maybe the job should be made to not process submodules?

Logically that makes sense, and should probably be done by default for all repos (not just this oen).

If it's a submodule that pulls from a repo that is also in Gerrit then we should setup rubocop in that upstream repo.

225238 was reverted by 226898 because it changed upstream code. The code changed in the patch is in modules/mysqland modules/stdlib, but neither of them is a submodule (see the list of folders that are submodules in a previous comment).

The question is: which folders contain upstream code and hence should be ignored by RuboCop?

zeljkofilipin changed the task status from Open to Stalled.Sep 1 2015, 1:54 PM

We would likely have to setup some reduced rule sets though, rubocop also complains about some things (such as methods more than 10 lines) that people who already hate ruby would probably hate even more :)

See our Ruby coding conventions for a reduced ruleset. I worked with upstream a while back to hone in Rubocop's overzealous behavior so that it was more in line with the style guide we've (mostly) adopted.

The question is: which folders contain upstream code and hence should be ignored by RuboCop?

Modules published to Puppet Forge should have either a Modulefile or metadata.json in their root directory. There might still be modules imported from elsewhere, however.

$ ls modules/*/{Modulefile,metadata.json}
modules/mysql/Modulefile     modules/stdlib/Modulefile
modules/mysql/metadata.json  modules/stdlib/metadata.json
modules/rsync/Modulefile

Shouldn't the puppet modules that are imported be changed to submodules?

Change 235695 had a related patch set uploaded (by Zfilipin):
WIP rubocop: do not run for upstream code

https://gerrit.wikimedia.org/r/235695

I have disabled RuboCop for upstream code in 235695, but I am not sure if I have the correct list of folders.

RuboCop is happy with the commit: https://integration.wikimedia.org/ci/job/bundle-rubocop/994/console

@hashar: any idea on which folders contain third party code?

Very few. Those would be

modules/stdlib
modules/rsync
module/mysql

All three slightly to heavily patched by us (mostly additions, not many changes). The mysql repo might even be abandoned in the future. We don't have a clear policy on updating them, it's on a per case basis. For now ignore them in the rubocop checks.

The git submodules are all owned by Ops so running checks on them would be nice if not required.

Anything else I can help with?

zeljkofilipin changed the task status from Stalled to Open.Sep 11 2015, 9:57 AM
zeljkofilipin removed a project: Scrum-of-Scrums.

@akosiaris: Thanks! :)

The only thing left to do is reviewing and merging the related commit: 235695.

Change 235695 merged by Alexandros Kosiaris:
rubocop: do not run for upstream code

https://gerrit.wikimedia.org/r/235695

Done.

I noticed that git submodules are being excluded right now in https://gerrit.wikimedia.org/r/#/c/235695/6/.rubocop.yml,cm. Can we start adding them ? They are all owned by us so checks should be run on them as well

Sure you can add them. I would advise to enable the same Jenkins jobs on the submodule repo itself first (I didn't check if that is already done), so that failures can be fixed before they are merged.

@akosiaris: A quick look at the submodules (searching for .rb files) says there are none. Am I missing them?

zeljkofilipin closed this task as Resolved.Sep 15 2015, 1:52 PM

I think this task is resolved. The only subtask that is created is T110019. I plan to work on it this week. If you think there is something more to be done here, please reopen. If there are subtasks that need to be created, go ahead.

@akosiaris: A quick look at the submodules (searching for .rb files) says there are none. Am I missing them?

No, they don't have any. But they might at some point. And of course when we do add, we will forget to update the rubocop configuration. That's my main worry

Change 238471 had a related patch set uploaded (by JanZerebecki):
Also check submodules with rubocop

https://gerrit.wikimedia.org/r/238471

I missed that submodules are not checked out in that job.

Well, we do not even need to explicitly ignore git submodules, since the Jenkins job does not even clone them. :D

https://integration.wikimedia.org/ci/job/bundle-rubocop/1060/console

zfilipin@integration-slave-trusty-1011:/mnt/jenkins-workspace/workspace/bundle-rubocop/modules$ tree -L 1
.
|-- contributions
|-- editor
|-- engine
|-- flow
|-- flow-initialize.js
|-- handlebars.js
|-- messagePoster
|-- notification
|-- styles
`-- tours

Change 238471 merged by Alexandros Kosiaris:
Also check submodules with rubocop

https://gerrit.wikimedia.org/r/238471