feat: add pre-commit for conventional commit

[davidspek]

This PR adds a pre-commit hook that ensures commits adhere to Conventional Commit. The installation of this hook is optional since I haven't found a good solution that doesn't require tools such as Python or Node be available on the developer's device. Because of this (and because commits can be created with the --no-verify flag), a GitHub action is added that also validates all commits in PRs follow Conventional Commit.

[Jamstah]

To link it up, this pr is related to automated release: #4025

[Jamstah]

I think this is a sensible addition to the workflow.

[Jamstah]

Should we add an example, or a list of the tags?

[milosgajdos]

I'm all up for this, the only thing I'm worried/wondering about is if this creates a friction for contributing 🤔

[davidspek]

I'm guessing you mean the enforcing of Conventional Commit messaging. It does create some friction which I also don't really like. However, having proper commit messages is definitely a best practice and you get used to it quite quickly (there's even a CLI tool to automatically create proper commit messages for you). It's also the only way to have proper versioning and change logs. It should also help ensure new features aren't accidentally introduced into maintenance branches that should only be receiving fixes or breaking changes getting into branches they shouldn't. Strictly speaking not every commit would need to adhere to the Conventional Commit standard, but those commits are ignored for deciding if a release should be cut and what the version of the release needs to be and could cause rouge commits in branches they shouldn't be.

[Jamstah]

What metrics would we use to determine if it affects contribution?

It doesn't stop you raising a pr or stop the tests running, it's not much different to us asking for a squash or that commits are signed off. Signing off commits seems like a similar hurdle too - rewrite the commit message

If we notice ill effects we can always pick a new process.

[milosgajdos]

I'm guessing you mean the enforcing of Conventional Commit messaging. It does create some friction which I also don't really like.

Yes, that's what I meant. There are already so many other barriers to get over (the ease of running tests, etc, etc.) that I feel like adding another obstruction could (would?) just make things even harder

However, having proper commit messages is definitely a best practice

I'd wager this is relative 😄 but don't get me wrong, I do get the benefits of having them. Mind you the mere requirement of the hassle of having to install some tool can be a put off

[milosgajdos]

What metrics would we use to determine if it affects contribution?

This is a good question. I think right now the only guiding light we have is the number of PRs opened within a certain time period 🤔

Signing off commits seems like a similar hurdle too - rewrite the commit message

Yeah I dont like that either and it's been a major pain for me personally getting people signing off their commits

I'm looking at https://github.com/goharbor/harbor/pulls and it seems it's a bit of a mixed bag - some conventional commit messages, some "random"

Curious what other @distribution/maintainers think.

I want to reiterate: I am a fan of this, but as I said my worry is a new point of friction (on top of git itself, DCO, etc.)

[davidspek]

I'm also definitely not a fan of having to install some tool for the pre-commit checks. Not only because it requires people to install a tool manually, but it isn't something we can centrally enforce. So that adds to the issue where people create commits, open a PR and only then see that the commits didn't follow the convention. Having to rewrite commit messages is a huge pain for contributors and adds a lot of friction. For that reason I've discussed an alternative approach with @milosgajdos over slack a couple days back.

Instead of requiring all commits to follow the convention, we change the GitHub setting to use the PR title as the commit title when a PR is merged. Then we have a check that verifies the PR titles follow Conventional Commit. This way, using conventional commit for each commit message becomes optional and something we can nudge people to do. However, we will have a strict policy of PRs following the convention so that everything is picked up in the release automation properly. What's nice about the PR title approach is that maintainers (and reviewers) can easily change the PR titles for people so it doesn't add any burden on contributors.

I'll be doing a full write-up on this in a doc as part of the release automation PR.

[milosgajdos]

question: should this be distribution/distribution? I'm reading this but I might be missing something https://github.com/compilerla/conventional-pre-commit#usage

[davidspek]

No, that's the repo where pre-commit is getting the source code for conventional-pre-commit. You can also use pre-commit to manage all sorts of other git hooks and their versions.

[milosgajdos]

ah, so that's basically saying: code in repo X will make the checks. Well that's not clear from their README 🫠

[davidspek]

I'm not really a huge fan of this solution for managing git hooks. But all the other ones I've found that look better are really focused on Node based projects and I'm not sure they'd integrate nicely. I did find a way it could be done using Go, but either we'd add additional dependencies to the project or I'd need to create a new tool that we can then use (which I might down the line).