Using git hooks before pushing a commit

Using git-hooks will prevent us from pushing a bad commit.
it will run npm run lint && npm test before you push the commit,
and we can add there whatever we want.

I used the husky library.

To prevent it, you can add the variable HUSKY_SKIP_HOOKS=1 :
HUSKY_SKIP_HOOKS=1 git push origin branch_name

Link to the PR - https://github.com/theforeman/foreman/pull/7087

TBH I don’t think this is something that should configured for everyone at the project level.
A large portion of PRs doesn’t include changes to webpack code, and running npm lint and test takes about last i checked 5 minutes - which is quite a delay. It will also fail if you have a plugin installed that fails linting, even if your pr has nothing to do with it.
People can configure their own hooks if they wish for what make sense for their environment and workflows, or can opt to run tasks manually before pushing.

5 Likes

Running it locally it took me about 1.5 minute to run the tests and linting,
but maybe we could apply it only when the code in the webpack directory have changed.

I like the idea, but having some mixed feelings about it - especially if it comes by default
I think we still should be able to push even if tests are not passing, I prefer to push a WIP pull request, rather then keep it locally until it will be ready, plus the tests stack becomes larger and by the time it might be annoying on each push.

Can we turn it on by demand instead of vice versa?

2 Likes

I think git hooks can turn very useful. TBH I think running tests/code-linting for each commit is too much to ask.
IMO it would be nice to use a pre-commit hook to verify your commit msg is valid.

Thanks, @amirfefer I agree but anyway you’ll need to run tests on your branch,
what if we added a prompt to let the user decide whether to run the tests before push? with “No” as the default.

@sharvit I really like the pre-commit idea !

The downside of pre-commit is that it’s used for all commits. I think it means that when you do a rebase of multiple commits you also run the hook multiple times. I also often do WIP commits to store work. Overall I dislike mandating hooks, especially if it’s javascript. However, I will admit I often rely on CI to actually run tests.

This is not the first discussion about it, BTW: https://github.com/theforeman/foreman/pull/5707

Lol, maybe in the next year it would get it… kidding…
Closed the PR for now.