-
Notifications
You must be signed in to change notification settings - Fork 154
chore: conventional commits #487
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
Conversation
8fc2a49 to
ec7684f
Compare
ec7684f to
9806070
Compare
cjappl
left a comment
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.
LGTM! Very cool.
(1000% outside the scope of this review)
Looking through our ci.yaml and seeing it get more complex, at some point it may be nice to move some of that to a Makefile that devs can run locally.
I have seen situations where the ci becomes some beast that is un-runnable locally and hard to troubleshoot. Stuffing it in a Makefile and having ci be something like make changelog helps squash that a bit.
carlfriedrich
left a comment
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.
@sandr01d Thanks a lot for your work on this! I haven't used Cliff before but like both its simplicity and flexibility.
I have just a few comments concerning the commit parsers, but none of these are blocking.
From my perspective this can be merged. Eventual adjustments can be made later when we have some experience on how all this performs in practice.
I also second the comment from @cjappl, it's a good idea to outsource the individual commands into small scripts that can be run locally, but that can indeed be done in a separate PR.
9806070 to
4ce05c9
Compare
|
Thanks for the quick reviews @cjappl @carlfriedrich. I've updated based on your suggestions. |
4ce05c9 to
9f2ea91
Compare
|
Also one additional change, I've updated Dependabot to use a lower case prefix, which is the common case for conventional commits. |
|
@sandr01d Great, thanks for the update. From my side this is merge-ready.
IMO we don't need any external tool at all. Why not use simple shell scripts for these things? Especially since we already have proper tooling for shell scripts in place. |
|
I'm more pro Either is fine by me, I think it only depends on how many inter-step dependencies we have (more, |
Sure that'd work for me too. |
Check list
Description
As discussed in #479, we'd like to use conventional commits for this project going forward. To help with this, this PR
commitlint.git-cliff.I'm using both of these tools in my dayjob, so I'm already familiar with them.
commitlintseems to be the goto solution for linting against conventional commits and from my research seems to be pretty much the only option. The downside of commitlint is that it is only available throughnpm, which means we have to first installnpmand thencommitlintin the workflow - which is a bit slow. I tried using the setup-node action, as outlined in commitlints docs, but this action seems to assume a Node.js based project and complains about the missing package-lock.json.I decided to go with
git-cliff, because I've had a very good experience using it and because I know it to be very flexible. Another big plus IMO, is that it can also be installed locally, which makes testing/troubleshooting a lot easier. I've created a release on my fork, so you can get an impression about how the changelog would look like. Ignore that most changes are currently listed under "Other". This is only the case because those commits do not follow conventional commits yet.Type of change
Test environment