It’s common that you want to run Rubocop on your plugin. In various plugins I’ve seen the pattern that it defines some rubocop task. However, most rake tasks are executed from the Foreman checkout itself. Standardizing this also makes it easier to run in CI. This is why I opened a PR to run Rubocop in a plugin.
The workflow is:
git clone foreman.git and add my_plugin as a dependency (as you normally would)
Run bundle exec rake plugin:rubocop[my_plugin] from the foreman checkout
It will use .rubocop.yml from the plugin if present. You can also pass another argument to output a JUnit XML file which can be very useful in CI.
Let me know if I missed anything. If it’s merged, I’m proposing to cherry pick it to stable branches so we can update our Jenkins setup to use it.
I don’t believe we should do this, unless we standardize the default RoboCop config.
I’m running RoboCop just by running rubocop cli command locally in the plugin directory and I think we shouldn’t imply the foreman dependency unless we need to do so.
So if we manage to get RoboCop defaults from Foreman, I’m all in, otherwise I’d standardize through the CI directly.
In general this makes sense to me, but a few questions came to mind:
In the past we’ve seen issues when core updated it’s rubocop version and plugins relying on that version broke due to changes e.g. in cop names. Will this change resolve such issues?
What about plugins that run rubocop outside jenkins (e.g. with github actions or travis), or use Hound to do inline linting on PR? How will they be affected by this change?
Can we do something similar for js linting as well?
What is the benefit of running rubocop from a rake task over executing it directly as @ezr-ondrej mentioned?
I see a strong trend where plugin maintainers prefer follow their own styles.
From a community perspective, that does mean that you have to be much more aware of which project you’re working on.
That leads me to think there are generally 3 options, which I’ll explain:
The shared nothing option means everything (rubocop version, rubocop config, maybe even another tool) lives in the plugin. The Jenkins job will simply chdir to the to the plugin, bundle install and run bundle rake $job
The next is the same, but if some marker is not present (not sure what this should be, maybe the presence of a file in the plugin git repo), it falls back to some shared config that lives in Foreman’s repo.
The last is basically the fallback option from the previous situation.
Shared nothing, all in the plugin
Default to plugin, fall back to shared
All from Foreman
0voters
The trade off is control of the plugin author. This freedom does mean a plugin author needs to do the work of maintaining. On the other hand, it does also mean that if the core project decides something, plugin authors do not need to do anything. The full copy may also work better with tools like Hound.
I’d like to ask plugin authors what their preference is.
I’d love this. As I’m very supportive of moving what we can to GH actions, I’d like to keep it simple and cloning foreman just for RoboCop is not that great, but our own gem with just the config sounds great. Btw, it could be just a GH action, with shared config.
Let me make some prototype over the weekend. But gem has more general usability anyway.
I do not contribute the code that much anymore and can live with whatever obscure cops.
However as a plugin maintainer, I’d prefer toto enforce my style or better to say, disable cops I don’t like. Every rubocop update surprises me with some rule, that was never needed before. If an update in core, out of a sudden, breaks my plugin tests on every PR, it will cause a frustration on my end. Especially if it happens just before the release.
Looking at it from the contributor perspective, our rubocop definition is highly customized. I don’t think someone would first look at rubocop.yml and code according to it. They just write the code, push the PR, see what they need to fix, run rubocop to fix it and repush. Therefore I don’t think it’s a big deal if plugins have different definitions.
But if more people prefer to unify this, I’m not against it. I just feel it’s not worth the effort.
It’s very simple solution, doesn’t introduce extra gem dependency, just a internet connectivity would be required, but I don’t feel it should be a blocker.
So the real reason the bootdisk job is failing is that every plugin adds Rubocop to jenkins:unit. That means it’s running the Rubocop job from the other gem in bootdisk.
To solve it, we still need to find a new way in Jenkins jobs. There are only 2 votes, but they both were for Shared nothing, all in the plugin. That suggests we should do bundle install in the plugin directory and run some job. This could be rake rubocop, rake jenkins:rubocop or plain rubocop. We should ensure they write the XML file to a predictable location so Jenkins can parse the results (something we don’t do today). This can be trivially done via plain rubocop, but that makes it harder to configure the job via git. A jenkins:rubocop can always write the file, but requires additional work on each plugin. We can also suggest plugin authors to write the task like this (untested):
require 'rubocop/rake_task'
RuboCop::RakeTask.new(:rubocop) do |task|
if ENV['RUBOCOP_JUNIT']
task.formatters = ['autogenconf', 'junit']
task.options = ['--out', ENV['RUBOCOP_JUNIT']]
end
end
Jenkins would set the env var RUBOCOP_JUNIT.
Once that’s done, all plugins should stop running Rubocop as part of jenkins:unit to avoid duplicate work.
IMHO this is not blocked on any work to make configuration easier. Today most plugins already have some Rubocop config.
I would say we should drop this from jenkins:unit. Most plugins anyways run rubocop using travis/hound/github actions already, and it doesn’t really make sense to have it run with a different config on jenkins. If we want to run rubocop on jenkins we could do it as a separate task from the tests, but I don’t see much value in doing that.
That’s where I was heading as well. The rubocop is perfect candidate to get extracted to github action, as the response is imediate, so it serves instead of annoying hound (notify me in matter of minutes, that i need to reformat and thus I am more likely to do it right away).
That’s why I’m inclined for easy solution. Shared config in foreman core, that offers basic cops, that we think all the plugins should use is what my PR would offer.
The url referenced cop is actually downloaded locally and checked only once in a while (can be include in git), probably not hard failing if no internet connection available.
I agree that versioning of it can be an obstacle, but on the contrary it doesn’t change as often.
Plus it’s not as hard to move to a gem, once we see that the versioning is an real issue.
Does this mean that every cop change requires a gem update? And then it will require changing dependency and fixing code in Foreman core and other plugins?
I don’t think this brings much value. It’s actually the opposite, more confusion, slower workflow. I believe if we must use Rubocop, YAML configuration should live closely with the (independent) code.
Just every change to common cops and I don’t see that happening very often. Gem should be released automatically, you just need to bump the gem version in you gemfile if you wanna get the updated core cops. So I don’t need to follow the core .rubocop file if I want to have the same style as core.
You are still kept with that option, if plugin maintainer don’t want to follow core guidelines, noone forces him to. It would only provide an option for standardized RuboCop rules over the ecosystem.
We should promote this, what we have today is so confusing. Here is a scenario I am seeing every month: a random contributor fixes something in a plugin, small patch, tests pass but Rubocop does not. She tries to run Rubocop in the plugin repo - it either does not work (Rubocop is not present in Gemfile) or what’s worse it runs some different version of Rubocop with a different configuration. This stems from the fact that even us, engineers, don’t know how Rubocop is executed on our CI.
This is super confusing and what I am fighting for here is that we get rid of this completely. I hate when we do something that people do not expect. Like using non Ruby on Rails technologies in RoR application. Or using Rubocop in such way that nobody would ever figure that out. We are wasting our and others time.
Don’t get me wrong, pal. I get your idea. I just think this isn’t getting any better.
