Skip to content

chore: validate conventional commits #4122

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

Open
steve-chavez opened this issue Jun 2, 2025 · 7 comments · May be fixed by #4128
Open

chore: validate conventional commits #4122

steve-chavez opened this issue Jun 2, 2025 · 7 comments · May be fixed by #4128
Assignees
Labels
hygiene cleanup or refactoring

Comments

@steve-chavez
Copy link
Member

steve-chavez commented Jun 2, 2025

Problem

Sometimes commits can be merged without following conventional commits and the commit messages can slip review.

Recent examples:

Solution

Enforce commits follow conventional commits.

We can do this with a git hook https://github.com/tapsellorg/conventional-commits-git-hook.

Also there's https://github.com/amannn/action-semantic-pull-request for enforcing PR titles -- although is not strictly needed IMO.

Notes

@taimoorzaeem
Copy link
Collaborator

This is a great idea. I guess for people with write access, a git-hook would do. For contributors, we can use a custom github action/workflow to validate the commit message(s). For instance a GH action running commitlint:

if [ "${{ github.event_name }}" = "pull_request" ]; then
      # For pull requests, check all commits in the PR
      git log --format="%H" ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }} | while read commit; do
        echo "Validating commit: $commit"
        git log --format="%B" -n 1 $commit | npx commitlint --verbose
      done
    else
      # For push events, check the last commit
      echo "Validating last commit"
      git log --format="%B" -n 1 HEAD | npx commitlint --verbose
    fi

We can ignore this check for situations, where we squash merge with a re-written commit message.

@steve-chavez
Copy link
Member Author

Sounds like a good idea. We should encapsulate that logic into a nix script so we can use it locally too and CI is kept simple.

For instance a GH action running commitlint
https://github.com/conventional-changelog/commitlint

This one looks like a big dependency though, I don't think we need all the functionality.

The bash script on the git hook looked really simple https://github.com/tapsellorg/conventional-commits-git-hook/blob/master/commit-msg-hook.sh.

Couldn't we do the same in a python script? To keep it simple, this python script should just process the commit message, it doesn't really have to callgit. Then we can pipe the git log to the python script as shown above.

WDYT?

@taimoorzaeem
Copy link
Collaborator

Couldn't we do the same in a python script? To keep it simple, this python script should just process the commit message, it doesn't really have to callgit. Then we can pipe the git log to the python script as shown above.

Yes, this should be doable with a simple python script. I'll try to work on this.

@taimoorzaeem taimoorzaeem self-assigned this Jun 4, 2025
@taimoorzaeem taimoorzaeem linked a pull request Jun 5, 2025 that will close this issue
@wolfgangwalther
Copy link
Member

Couldn't we do the same in a python script?

Not sure why we'd roll our own when commitlint can do it and much more as well.

I think we should just set up commitlint.

@steve-chavez
Copy link
Member Author

Not sure why we'd roll our own when commitlint can do it and much more as well.

https://github.com/conventional-changelog/commitlint

Do we need that much more? Why so much stuff for parsing a commit message of 72 characters max?

A big dependency like that will bring more renovate PRs. I think it's always a good policy to keep dependencies lean and lower upkeep.

@steve-chavez
Copy link
Member Author

If the commitlint from nixpkgs works fine (not sure NixOS/nixpkgs#250767), then no opposition from my side.

@wolfgangwalther
Copy link
Member

Examples of rules that could be interesting:

  • body-leading-blank: Nothing is more annoying than badly-written commit messages with body, where the first line is not empty. They break many UIs.
  • {body,footer,header,subject}-max-line-length, if we want to.
  • subject-case to force consistent casing after :.
  • subject-full-stop to disallow ending with a . there
  • subject-exclamation-mark, if we want to disallow that and always use the more verbose BREAKING CHANGE in the body.
  • type-enum, scope-enum obviously

I had previously also written an internal plugin to use imperative mood to start the subject line. See https://www.git-basics.com/docs/git-commit/commit-message-rules for example for rationale behind this.

A big dependency like that will bring more renovate PRs. I think it's always a good policy to keep dependencies lean and lower upkeep.

We should use commitlint from nixpkgs, then it will not cause any more effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hygiene cleanup or refactoring
Development

Successfully merging a pull request may close this issue.

3 participants