Fixes #3809 - Add rubocop to check Foreman code #1666
Conversation
daviddavis
changed the title
|
Should be no need to package it, it's only for tests. If you can copy what's in Katello for |
daviddavis
changed the title
|
@domcleal does the task |
|
Sure, we already have the jenkins:* namespace in lib/tasks/jenkins.rake, so it can be added there. |
|
Right. Thanks. |
|
I got the Jenkins task added and working. I forgot that I need to upgrade Katello to the same version. Will open a PR for that and link it here. |
|
This should be ready to go now. |
|
I added rubocop to the Jenkins job configuration and there were still five warnings on this branch. [test] should show this shortly. Note that the job will show "SUCCESS" as it's configured to fail quietly for now, since this isn't yet merged. |
|
@domcleal thanks. I'll take a look into the failures. |
|
@domcleal should be good to go now. |
|
Should :rubocop task be executed automatically before tests? Probably worth adding something similar to smart-proxy too. |
|
@witlessbird I suppose we could eventually look into running it automatically before tests. However, right now, rubocop doesn't support Ruby 1.8 so we're only running it with the 1.9+ jobs in Jenkins. Regarding smart-proxy, I'd be happy to look into adding rubocop there too. |
|
Do we not also want a generic rake rubocop task? I am also thinking of this in light of the fact that it would be nice to do what we have done with some other tasks and make them usable by plugins, so that plugins don't have to carry one-off rake tasks for common operations (and we could converge on the same cops being used and thus style). For example, |
|
@ehelms I can see that being helpful in the future (once we're closer to needing it). I'm tempted to open an issue for it (it'll also be easier to implement once we drop 1.8.7). Thoughts? EDIT: Added a basic rubocop rake task and opened http://projects.theforeman.org/issues/7196 for Katello. |
daviddavis
force-pushed
the
temp/20140807164439
branch
4 times, most recently
from
8c35eb4
to
1b59ad1
Compare
|
@daviddavis I can live with that for now, I do think you'll end up coming back to edit the generic task to allow plugins to easily call it in the future but I can live with that being in the future. |
daviddavis
force-pushed
the
temp/20140807164439
branch
3 times, most recently
from
846188b
to
51f000a
Compare
daviddavis
force-pushed
the
temp/20140807164439
branch
2 times, most recently
from
89224ac
to
5bde678
Compare
|
Seems it's failing silently... I'm a bit worried that the exit code would appear to be zero, as Jenkins should have failed the job if it was non-zero, but hey. + 1408718339.583267410 /tmp/hudson7452188239192663296.sh : 43 > bundle exec rake jenkins:rubocop TESTOPTS=-v Your Gemfile lists the gem facter (>= 0) more than once. You should probably keep only one of them. While it's not a problem now, it could cause errors if you change the version of just one of them later. Your Gemfile lists the gem gettext_i18n_rails_js (>= 0.0.8) more than once. You should probably keep only one of them. While it's not a problem now, it could cause errors if you change the version of just one of them later. Your Gemfile lists the gem gettext (~> 2.0) more than once. You should probably keep only one of them. While it's not a problem now, it could cause errors if you change the version of just one of them later. Your Gemfile lists the gem locale (<= 2.0.9) more than once. You should probably keep only one of them. While it's not a problem now, it could cause errors if you change the version of just one of them later. Your Gemfile lists the gem minitest (~> 4.7) more than once. You should probably keep only one of them. While it's not a problem now, it could cause errors if you change the version of just one of them later. uninitialized constant Rubocop /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:70:in `const_get' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:70:in `block in custom_formatter_class' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:69:in `each' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:69:in `reduce' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:69:in `custom_formatter_class' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/formatter/formatter_set.rb:34:in `add_formatter' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/runner.rb:131:in `block in formatter_set' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/runner.rb:130:in `each' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/runner.rb:130:in `formatter_set' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/runner.rb:23:in `run' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/lib/rubocop/cli.rb:24:in `run' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/bin/rubocop:14:in `block in ' /usr/local/rvm/rubies/ruby-1.9.3-p392/lib/ruby/1.9.1/benchmark.rb:295:in `realtime' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/gems/rubocop-0.24.1/bin/rubocop:13:in `' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/bin/rubocop:23:in `load' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/bin/rubocop:23:in `' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/bin/ruby_executable_hooks:15:in `eval' /usr/local/rvm/gems/ruby-1.9.3-p392@test_develop_pr_rubocop-2/bin/ruby_executable_hooks:15:in `' |
daviddavis
force-pushed
the
temp/20140807164439
branch
3 times, most recently
from
f07c5e2
to
8b1df2f
Compare
|
@domcleal thanks for reviewing. I updated the code. |
daviddavis
force-pushed
the
temp/20140807164439
branch
4 times, most recently
from
d293408
to
6be08c6
Compare
| @@ -14,6 +14,14 @@ begin | |||
| end | |||
| task :minitest => [:pre_ci, "ci:setup:minitest"] | |||
| end | |||
|
|
|||
| task :rubocop do | |||
| system("bundle exec rubocop \ | |||
There was a problem hiding this comment.
Should we add -R for Rails cops too?
There was a problem hiding this comment.
I turned on the RunRailsCops config instead.
There was a problem hiding this comment.
Ah, that's even better. Nice that it's all config-driven.
daviddavis
force-pushed
the
temp/20140807164439
branch
from
6be08c6
to
a5154b2
Compare
daviddavis
force-pushed
the
temp/20140807164439
branch
from
a5154b2
to
0b13f37
Compare
|
Thanks @daviddavis, merged as 7ebd35e for Foreman 1.7. |
This basically disables all the failing rules in Foreman and puts them in
.rubocop_todo.yml. Over time, we'll need to address each cop by fixing the code or adding it to.rubocop.yml(if we don't want to enforce it). Then when all the cops are removed, we'll remove.rubocop_todo.yml.Katello PR: Katello/katello#4565
If you want to look up the cops to see what they do, visit http://rubydoc.info/gems/rubocop/0.24.1/frames