Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gitlint cannot be run easily via pre-commit inside a CI without pre-commit configuration for the commit stage #191

Open
guillaumelambert opened this issue Jun 3, 2021 · 4 comments

Comments

@guillaumelambert
Copy link
Contributor

guillaumelambert commented Jun 3, 2021

Some projects maintainers require that all linters must be installed via pre-commit
ant its YAML configuration, even when they are called from a CI.
Typically they want to use a tox profile simply calling " pre-commit run --all-files --show-diff-on-failure".

This is not possible with gitlint since it only proposes a pre-commit configuration
for the commit-msg stage with different defaults than the one recommended in
a CI, mainly because a .git/COMMIT_EDITMSG file is needed.

When running pre-commit in a CI system the COMMIT_EDITMSG does not
(normally) get created as that is an artifact of editing the commit
message. If the file does not exist then gitlint will skip which makes
it possible for pre-commit checks that should fail, to pass.

A workaround is proposed by the global-jjb project at
https://gerrit.linuxfoundation.org/infra/c/releng/global-jjb/+/67641
to generate such a file but it had several drawbacks such as:

  • the necessity to pass the HOME variable to virtualenv even locally
  • /bin/sh as whitelist_externals
  • conditional scripting that might have unexpected side-effects in
    another situation.

Indeed, the issue should better be fixed in gitlint uptream repository
itself so that gitlint can be run at the commit stage from pre-commit.
Since gitlint embeds its own hook solution for this stage, this
configuration is not (yet?) proposed in gitlint official repository.
https://jorisroovers.com/gitlint/#using-gitlint-through-pre-commit

Also the pre-commit hook provided up to now has a different behavior
than standalone gitlint defauts settings that are advised in a CI
context (i.e. "gitlint lint" or simply "gitlint").
https://jorisroovers.com/gitlint/#using-gitlint-in-a-ci-environment

The parameters --stage and --msg-filename options are passed and they
appear to be the root cause of the problems in the CI since they add
new requirements i.e. a local git user configuration and the presence of
an extra file argument.
https://github.com/guillaumelambert/gitlint/blob/main/.pre-commit-hooks.yaml
introduced in commit 9de1f89

pre-commit hook YAML upstream configuration does not allow to use
different options depending on the stage inside a same id block nor with
two blocks using the same id.
Only options from the last block appears to be used in that case.
As a consequence, a new id "gitlint0" must be declared but it still can
use the same name "gilint" displayed at runtime.

guillaumelambert added a commit to guillaumelambert/gitlint that referenced this issue Jun 3, 2021
guillaumelambert added a commit to guillaumelambert/gitlint that referenced this issue Jan 31, 2022
guillaumelambert added a commit to guillaumelambert/gitlint that referenced this issue Aug 21, 2022
jorisroovers pushed a commit to guillaumelambert/gitlint that referenced this issue Nov 10, 2022
jorisroovers pushed a commit that referenced this issue Nov 11, 2022
jorisroovers added a commit that referenced this issue Nov 16, 2022
- Python 3.11 support
- Last release to support Python 3.6
- Behavior Change: In a future release, gitlint will be switching to use
  `re.search` instead of `re.match` semantics for all rules. (#254)
- gitlint no longer uses the `sh` library by default in an attempt to reduce
  external dependencies.
- `--commits` now also accepts a comma-separated list of commit hashes,
  making it possible to lint a list of non-contiguous commits without invoking
  gitlint multiple times (#283)
- Improved handling of branches that have no commits (#188)
- Support for `GITLINT_CONFIG` env variable (#189)
- Added a new `gitlint-ci` pre-commit hook, making it easier to run gitlint
  through pre-commit in CI (#191)
- Contrib Rules:
  - New `contrib-disallow-cleanup-commits` rule (#312)
  - New `contrib-allowed-authors` rule (#358)
- User Defined rules:
  - Gitlint now recognizes `fixup=amend` commits, available as
    `commit.is_fixup_amend_commit=True`
  - Gitlint now parses diff **stat** information, available in
    `commit.changed_files_stats` (#314)
- Bugfixes:
  - Use correct encoding when using `--msg-filename` parameter (#310)
  - Various documentation fixes (#244) (#263) (#266) (#294) (#295) (#347) (#364)
- Under-the-hood:
  - Dependencies updated
  - Moved to blacked for formatting
  - Fixed nasty CI issue (#298)
  - Unit tests fix (#256)
  - Vagrant box removed in favor of github dev containers (#348)
  - Removed a few lingering references to the `master` branch in favor of `main`
  - Moved roadmap and project planning to github projects

Full Release details in CHANGELOG.md.
dupuy added a commit to dupuy/gitlint that referenced this issue Jan 21, 2024
Relates to jorisroovers#191, and specifically with making it easy to use gitlint with the https://pre-commit.ci CI service.
@dupuy
Copy link

dupuy commented Jan 21, 2024

I'm using the https://pre-commit.ci CI service to run gitlint as a pre-commit check, but had to explicitly add stages: [manual, pre-commit] to get it to run there.

The .pre-commit-hooks.yaml added in commit 973db5d has stages: [manual] which doesn't add the manual stage, but rather replaces the default pre-commit stage with manual.

Since the gitlint hook only runs in the commit-msg stage, and gitlint-ci only runs in the manual stage, adding the gitlint-ci hook has no effect for pre-commit.ci without making other changes – the minimal change is to add stages: [manual, pre-commit] to the hook configuration in my repo's .pre-commit-config.yaml.

@guillaumelambert
Copy link
Contributor Author

Hello
You can find the debate about manual vs pre-commit stages in the related PR comments
#192
Hope this helps
Guillaume

I'm using the https://pre-commit.ci CI service to run gitlint as a pre-commit check, but had to explicitly add stages: [manual, pre-commit] to get it to run there.

The .pre-commit-hooks.yaml added in commit 973db5d has stages: [manual] which doesn't add the manual stage, but rather replaces the default pre-commit stage with manual.

Since the gitlint hook only runs in the commit-msg stage, and gitlint-ci only runs in the manual stage, adding the gitlint-ci hook has no effect for pre-commit.ci without making other changes (the minimal change is to add stages: [manual, pre-commit] to the hook configuration in my repo's .pre-commit-config.yaml.

@dupuy
Copy link

dupuy commented Jan 25, 2024

Adding the pre-commit stage for the gitlint-ci hook to my .pre-commit-config.yaml was a pretty easy fix to get the hook to run on @asottile's excellent pre-commit.ci service. My PR was mostly to ease the way for others who might want to run this check in that environment.

But it turns out that for the pre-commit.ci service, there is a larger problem I couldn't resolve. As a low latency, low overhead CI service, pre-commit.ci avoids extra overhead and cost by checking out the Git repo for a PR as a shallow (depth=1) copy, and I could not find any way to deepen the repo, despite several different attempts. Even if the full branch commit history wasn't present, I thought there might be some benefit in checking the final (and only) commit message. However, even that minimal check was stymied, since the final commit in the shallow copy at pre-commit.ci is a merge commit between the branch and trunk, with a standard machine generated commit message that isn't worth checking. At that point I decided to abandon the effort, and added the gitlint-ci hook to the set of pre-commit hooks skipped by pre-commit.ci.

@guillaumelambert
Copy link
Contributor Author

Adding the pre-commit stage for the gitlint-ci hook to my .pre-commit-config.yaml was a pretty easy fix to get the hook to run on @asottile's excellent pre-commit.ci service. My PR was mostly to ease the way for others who might want to run this check in that environment.

But it turns out that for the pre-commit.ci service, there is a larger problem I couldn't resolve. As a low latency, low overhead CI service, pre-commit.ci avoids extra overhead and cost by checking out the Git repo for a PR as a shallow (depth=1) copy, and I could not find any way to deepen the repo, despite several different attempts. Even if the full branch commit history wasn't present, I thought there might be some benefit in checking the final (and only) commit message. However, even that minimal check was stymied, since the final commit in the shallow copy at pre-commit.ci is a merge commit between the branch and trunk, with a standard machine generated commit message that isn't worth checking. At that point I decided to abandon the effort, and added the gitlint-ci hook to the set of pre-commit hooks skipped by pre-commit.ci.

AFAIK, shalow (depth=1) is gitlint default config and fits well in Gerrit because each commit is reviewed separately.
With github and gitlab, the PR can contains several commits, some projects CI use git shallow option to purge the git history but this is normally configurable.
If this is not the case, did you try anything like " pre-commit run --all-files --show-diff-on-failure --from-ref deadbeef123 --to-ref HEAD~" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants