WIP: include lhcb-hooks in gitlab-ci

This change is part of the simplification process of some scripts that should run before changes are merged to a branch. This is especially important for the master branch.

Up until now, every repository needed to include their own jobs that would contain an implementation of the said scripts. For example, if i wanted to check that the formatting is ok in my files before merging, i would have to create a job that includes a script that does that. And if i wanted to have this in my other repository as well, i would have to write it there too.

We have found a way to make this simpler and with much less duplication. This solution comes in the form of a python package called "pre-commit". I will not go into much detail on what pre-commit is, as you can find more information here (https://pre-commit.com/) and another explanation as well as a guide on exactly how to use it, here (https://gitlab.cern.ch/lhcb-core/pre-commit-hooks).

So, each repository should have its own .pre-commit-config.yaml file, which shows what scripts need to be run before commiting/merging and where to find them. If a repository does not contain a .pre-commit-config.yaml file, an lhcb default .pre-commit-config.yaml is used instead, containing some standard scripts (a check for copyright comment on the start of the file, a check for the formatting of files and a check for a license file in the repository). If all the checks pass, the pipeline continues normally, usually by running the unit tests. If a check fails, the whole pipeline fails as well, indicating that these changes should not be merged.

The logic above, is captured in the included yaml file. That way, a common way to include this kind of scripts is provided, while customization is still available by having your own .pre-commit-config.yaml. This limits duplication while needing to include only 2-3 lines in your gitlab-ci.yaml

To use this yaml file properly, the stage "pre-commit-hooks" needs to be added to the pipeline if other stages are already defined.

@gtsalama MR failed, additional checks needed.

Yes, i know. What is the standard process in these cases? Just make it a WIP? Or write a comment that it is being investigated?

Better to make it a WIP for now. When MR will be ready, just remove WIP and it will be picked up again.

added Build new feature labels

@msaur since this only affects the GitLab CI pipelines, there is no need for testing in the nightlies.

marked as a Work In Progress

resolved all threads

unmarked as a Work In Progress

added 1 commit

• d7293a0c - include pre-commit-hooks in gitlab-ci

Compare with previous version

/ci-test --merge

mentioned in issue Moore#198 (closed)

assigned to @rmatev

resolved all threads

@gtsalama I wonder why the script step takes more than a minute when no files are changed, locally it seems much faster.

Would it make sense to use the CI cache for the venv to speed up the job? I guess in that case we should remove the condition https://gitlab.cern.ch/lhcb-core/pre-commit-hooks/-/blob/c25454936ce05fff5b2efd8b9a8217d2cd8afd4a/run_hooks.sh#L8 ?

For some legacy branches (not here), we'll only run the licence/copyright checks and not the formatting check. It would be good if we can override the default behaviour with a list of hooks to run. We can use variables (as in this example) to pass the information of which hooks we want to run to the included yaml. It can then pass something to run_hooks.sh

I experimented a bit (see master...include-pre-commit-hooks-in-ci-test) and it seems that the job doesn't fail when I expect it to fail

Currently you add the job in the test stage, but I think this name is not very appropriate. I think we always want the hooks to run first, in a their own stage. Can we achieve this by just changing the stage name from test to check (like in Moore)?

i can look into it, sure

stage name changed

apart from the fact that I do not think check or test makes so much a difference, I have the feeling that declaring the stages in the included YML file will conflict with the stages defined in the project .gitlab-ci.yml.

this may be unavoidable, i will check if you can declare it inside the include instead or sth

it seems that we cannot add stage on the include, only on the included yaml file. So, i think we should go with the stage that is most likely to be used by every project while still making sense. That stage is probably test

I'm not sure I completely understand, but if we have to avoid conflicts how about this feature: https://docs.gitlab.com/ee/ci/yaml/#pre-and-post (issue at https://gitlab.com/gitlab-org/gitlab/-/issues/31441)

actually, it sounds like it won't work

Note: A pipeline is not created if all jobs are in .pre or .post stages.

let me try it. If it works, it will be a pretty good solution imo

https://gitlab.com/gitlab-org/gitlab/-/issues/29980 is also of interest. If I understand correctly, defining the stage at all in the included yml is not a good idea. Instead, we should fix a name that doesn't clash (eg pre-commit-hooks) and have every project add this stage explicitly.

i think this is what we want to avoid. But it's not certain that we can

yup, .pre as the only stage did not work

Since to use the new hooks requires a change in the .gitlab-ci.yml, we can decide that the change has to be:

• add the pre-commit-hooks stage
• include the yml fragment

i changed the stage in the yaml file from 'checks' to 'pre-commit-hooks'. Since that is the only job run in this repo, it is not needed to add the stage in this .gitlab-ci.yml, i run the pipeline with no issues

I'd say we have to remove

stages:
  - pre-commit-hooks

from the included yaml as it has the potential to overwrite the locally defined stages

assigned to @gtsalama and unassigned @rmatev

@gtsalama Thanks for the very welcome developments, I can't wait to start using the hooks locally!

@clemenci please chime in on the points I raised and add your own