Kibana: [discuss] Changes to pre-commit hook

[tylersmalley]

We are currently enforcing the pre-commit hook by writing to .git/hooks/pre-commit as part of bootstrap. The pre-commit hook aims to identify issues for the staged files to reduce the feedback loop from CI.

As part of the new build tooling, we have identified some friction using our current method, and I would like to understand if contributors would prefer the proposed change. Additionally, while the pre-commit hook is useful, often you would like to bypass it. That can be done with --no-verify; however, that is not always the case as with an interactive rebase.

On CI, we could identify if the pre-commit hook would have prevented a failure and recommend using it.

Proposal:

The pre-commit hook would no longer be forcefully installed on bootstrap. You can manually run the pre-commit hook with node scripts/precommit_hook, or you can have it installed for you with node scripts/register_git_hook.

Issues with the current approach:

• Building/installing the pre-commit hook accounts for 4-5 seconds of the bootstrap.
• Since the pre-commit hook is always written, you can not manually modify the pre-commit hook for the Kibana project.*
• The pre-commit hook can not always be disabled with --no-verify, as some commands like rebase do not support this.

Please:

👍 if you are in favor
👎 if you are against
Comment if you have feedback

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[cjcenizal]

I am not in the 👍 camp but I'm also not against it because I don't know if the proposal will introduce any new friction for me. I already run node scripts/precommit_hook manually, but I also do rely on the automated precommit hook catching mistakes when I forget, so I'll be one of those to install it with node scripts/register_git_hook. I think we should try the proposed approach out and be prepared to back out of it if there's a net increase in suffering.

[tsullivan]

Is there any way that a shell environment variable could be used to flag bootstrap to automatically register the git hook?

I like the pattern that the KBN_OPTIMIZER_THEMES variable introduced, which sets me up automatically to customize optimizer behavior, no matter which repo of Kibana I am working in (e.g a separate clone of Kibana to work in 7.x). I exported it from a shell init script and never had to think about it again... until now, that is :)

[spalger]

@tsullivan Maybe you want to use an alias instead of calling bootstrap directly, I plan to change my kbs alias to:

yarn kbn bootstrap && node scripts/register_git_hook

[tylersmalley]

@mistic and I have also discussed ensuring that the equivelant of the pre-commit hook is run early on CI and the error message to include information on registering the pre-commit hook.

Another thought on allowing the pre-commit hook to be more owned by the developer and not overwritten is that it would allow them to also add things like running Jest with --onlyChanged if they choose to have that overhead.

[dgieselaar]

For those reading along, you can add a precommit file that will be executed by the Kibana-installed precommit hook: https://github.com/elastic/kibana/pull/39784. It doesn't allow you to override the Kibana-installed precommit hook but figure it might be useful for some cases.