-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat: add pre-commit for conventional commit #4082
base: main
Are you sure you want to change the base?
Conversation
356b976
to
8d74f74
Compare
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
8d74f74
to
c6e6ada
Compare
To link it up, this pr is related to automated release: #4025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a sensible addition to the workflow.
## Commit Formatting | ||
|
||
This project adheres to [Conventional Commit](https://www.conventionalcommits.org/en/v1.0.0/#specification) for commit formatting. To make adhering to Conventional Commit easier [pre-commit](https://pre-commit.com/) can be used to validate commit messages before they are created. To install the [pre-commit](https://pre-commit.com/) and the git hook please run the following commands: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an example, or a list of the tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all up for this, the only thing I'm worried/wondering about is if this creates a friction for contributing 🤔
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. |
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. |
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
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 |
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 🤔
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 |
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. |
@@ -0,0 +1,7 @@ | |||
repos: | |||
- repo: https://github.com/compilerla/conventional-pre-commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should this be distribution/distribution
? I'm reading this but I might be missing something https://github.com/compilerla/conventional-pre-commit#usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so that's basically saying: code in repo X will make the checks. Well that's not clear from their README 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.