What I like about the hound is getting the comments inline on the PR without having to go into jenkins to figure out what broke the linting. It also saves some of our precious jenkins cycles for other tasks.
If we were to remove it (which i think we shouldn’t, getting an extra notification now and then isn’t such a big deal imho), I would ask that we replace it with some other method of making sure linting passes for both ruby and js before code gets merged, not just turning it off.
In katello at least, you can always run rubocop locally. It takes ~30
seconds I typically run it before pushing up a new PR. To me this was less
annoying than hound, but i can see new developers preferring hound.
Since foreman has relied on hound for so long, I’m not sure if rubocop even
passes properly in ‘develop’. That would probably need to get fixed before
you could rely on jenkins even. +1 to removing hound.
a big +1 from me for removing hound. As most of you know I’m not a big fan to say the least. Mostly for the reasons John mentioned. I believe if we have a separate check in github for this would be great and visible enough.
The problem with just running rubocop locally is that if you don’t gate merges on it and someone forgets to run it you merge code that isn’t linted.
Rubocop should pass in develop because hound runs rubocop on all the changed files, so the only way it wouldn’t pass is if for some reason hound wasn’t run on a pr or someone ignored the hound’s warning and merged code.
With all the recent changes in jenkins, would it be possible to only run tests after linting is green? that would save a lot of cycles when someone pushes code that fails linting only to push again 30 minutes later with the linting fixed. In that case, i do see value in returning the linting to Jenkins and removing hound.
One more benefit of the inline comments from hound that hasn’t been mentioned is that it is much easier for new contributors to understand what they need to fix in their code, without having to figure out the failures deep inside Jenkins.
BTW, if you don’t want to get linting errors in your PRs, you can always add a git hook that runs linting on pre-push.
All that being said, I understand several people quite dislike the current status, so lets have a poll and see (calling @core to state their opinion) :
Also worth mentioning hound check for css lints as well.
I personally would like to keep hound as I find it’s a useful service both
for new developers who don’t know the project linting rules and for
maintainers who knows that a pr is not ready yet.
Hound is helpful, but the comments can be disrupting. GitHub recently added “Checks” which should address this issue. Hound is apparently already working on moving to this API. Comments by our pr-bot could also be updated to use checks rather than comments.
We should definitely lint at some point in our PR/CI pipeline, having Hound take care of that, means Jenkins doesn’t need to. However linting before tests run means that the build is cancelled sooner and not via an eventual push with linting issues fixed.
To avoid linting issues being pushed it might also be good to advice on using linting before pushing any code. A good practice is to either have it enabled in the editor or which is even better have it lint before running tests locally. A third option is to use a pre commit hook. There are tools like lint-staged that can help with that.
I think we can all agree that linting is useful and I don’t want to get rid of it. The problem with hound is, that we don’t have any control. Hound updates the rubocop rules when they please and just run the rules on changes files. That’s why rubocop can still fail on develop even though every PR checks out green.
Ideally, Jenkins could use the new checks api to mark linting failures inline.
That’s why rubocop can still fail on develop even though every PR checks out green.
In my experience Hound also can pass while rubocop can fail because hound only checks updated code. This can cause it to miss rules. Thats why on Katello we never took Hound as the final authority and still run rubocop in jenkins.
This may have gotten better recently, but it looks like at least one test (block length check) is failing on foreman develop.
I’m not going to vote, since I don’t actually write core code these days - but to me the choice seems to be between “Run our own linting” vs “Use a 3rd party linter that we can’t control”. Given we have the resources, the latter seems far less preferable to the former
Notice we don’t have jobs in Jenkins for CSS, JS, linting, which Hound does:
Also and far more important IMO: when a new contributor submits a PR, Hound comments directly on the lines that are breaking the linting. It’s immediately clear what to fix.
Most people are going to be very confused at Jenkins’ job output, or at least what we had before. You have to do a couple of clicks to get to the info (if you know where it is) and we have to maintain it.
In Foreman Ansible, I do run rubocop after the tests pass. It’s confusing, you see on GitHub that Jenkins is red, with “0 failures”. I know where to look at in the console output, but most new contributors don’t.