Better precommit / prepush hooks

📝 Summary

Lets improve the local checks done before pushing code to the Kibana repo.

Why

Engineers are currently using Elastic's CI as the primary way to validate their code. We have a script in the repo which installs pre-commit git hooks, but this script is not able to catch a significant amount of frequently occurring issues. In addition installation of this hook is optional, and the engineer needs to know it exists. This leads to a substantial amount of early catchable failures in the day to day engineering process. We need to improve this area to try to reduce our usage of build minutes on our CI provider, as well as save costs by catching mistakes earlier. It can be argued that this will also lower Elastic's environmental impact.

Rigor vs speed

We need to strike a balance between the thoroughness of the checks and the time it costs to perform the checks. Currently the checks are simplistic (and in some cases outdated) so the local checks are performed very fast, but in order to catch more issues we need to cast a wider net.

Areas of improvement

Preflight check hook needs to be pre-push, not pre-commit Currently the script is set to pre-commit. We intend to move to pre-push instead. This would allow engineers to organize the work in their branches in a way that makes sense to them and not be slowed down by checks while they are committing. Then, when they are ready to push their work and open a PR, the checks (hereafter referred to as preflight checks) should be performed.

Add Typescript checking Typescript checking is not performed by the script. This check is essential.

Update linters The script still performs a Stylelint check. This is largely deprecated since we moved to emotion over scss. Let's remove it.

Add unit test checking If any of the changed files in the diff include unit tests, we might want to run those.

i18n checking If the git diff includes any changes in localisation identifiers which are not backed by changes in the translation files, we need to run the i18n fix script.

Checking strategy Using a git diff to only check the changed files is a naive way to catch errors. Perhaps we can be smarter in our approach by analysing the changed files for imports to see if those files comply with all linters and checks outlined above. Same goes for files that are importing the file that is analysed in the diff. The question is how many levels deep to go. This strategy will not be 100% flawless and will not catch everything, but it will catch more than it does currently which will save the company time and money.

Thoroughness option We might want to offer the engineer the option to provide a more thorough preflight check which employs further heuristics to determine dependencies or consumers of changed code. This way engineers can decide when they are performing big refactor jobs across multiple plugins to do a wider check.

Add telemetry Let's add telemetry to the prepush hook so we can continually learn:

What errors and types of errors are caught
Which branch, so we can cross reference the results of the local preflight check with results from the more comprehensive CI checks so we can see if there are certain types of issues we can catch with the preflight checks)
How long the preflight checks take, backed with processor architecture so we distinguish Intel from M series chips

Install preflight checks as part of yarn kbn bootstrap Preflight checks are now optionally installed. It is proposed that as we gain more confidence in the changes we intend to make to the checks, we will include installation as mandatory. This can be done by hooking into the yarn kbn bootstrap command. This mechanism also allows an easy mechanism to evolve the preflight checks over time as bootstrap is run by engineers often.

Escape hatch While we propose making installation of the hooks required as they will be part of the Kibana bootstrap process, it is always possible to opt out of checks by pushing with --no-verify.

elastic / kibana