Can we get rid of hound?

I’m not sure about others, but I don’t find hound commenting on PRs that helpful, and it can create a lot of unnecessary notifications for others if you forget to lint your code before pushing up.

Are there any objections from removing hound from commenting on pull requests?

3 Likes

I’m all in for this. Especially because multiple times in big PRs rubocop/eslint warnings got merged. I’d propose to run the rubocop rake task in Ruby tests and add eslint to JS tests.

As a plus, we can use nay rubocop version we want and use eslint plugins.

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.

1 Like

That’s what I meant with

+1 I didn’t realize this doesn’t exist on foreman already when I started this thread.

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.

Justin

1 Like

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) :

  • Leave hound in place
  • Replace hound with linting in Jenkins
  • Either option works for me
  • I love messy code, let’s not lint at all

0 voters

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[1]. 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[2] that can help with that.

[1] https://github.com/houndci/hound/issues/1536
[2] https://github.com/okonet/lint-staged

1 Like

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.

2 Likes

That would be the ideal solution.

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 seeing a slight preference for Jenkins/Travis right now, any more votes from core devs? :slight_smile:

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 :slight_smile:

I tend to disagree, and it seems like currently its 54% in favor of leaving it as it is.

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.

2 Likes

Sorry, it’s exactly like Greg says. We can’t control:

Thus, when using hound, we limit ourselves to the set, that hound defines.

If this is really the problem, we need more documentation on that. Having i.e. eslint and rubocop is normal for all kind of projects - and most don’t use hound.

Agreed.