oss-review-toolkit / ort

A suite of tools to automate software compliance checks.
https://oss-review-toolkit.org
Apache License 2.0
1.59k stars 308 forks source link

Replace commitlint with a more generic and less fragile solution #6668

Open sschuberth opened 1 year ago

sschuberth commented 1 year ago

Since the introduction of conventional commits and commitlint to enforce them we've faced various issues with commitlint, resulting in work-arounds like

https://github.com/oss-review-toolkit/ort/blob/2ce43220f4e8dce5d6421b4527c7ab513dd8345d/.commitlintrc.yml#L5-L11

Also the configuration syntax sucks quite frankly, like in

https://github.com/oss-review-toolkit/ort/blob/727fc5bd73d7fd6c4047093db72a6390f77b1419/.commitlintrc.yml#L13-L15

what the heck is "2" supposed to mean? Let alone the bugs.

As conventional commit can easily be checked via regexes, I propose to instead switch to a generic tool like https://github.com/GsActions/commit-message-checker that can also be configured to check for some of our other non-conventional-commits requirements.

Opinions, @oss-review-toolkit/core-devs?

Edit: Collecting ideas for alternatives

MarcelBochtler commented 1 year ago

While I get the issues with commitlint, continue using it has some advantages:

My vote is to continue using commitlint.

sschuberth commented 1 year ago

I can use editor plugins (e.g. for vim / IntelliJ) that already verify the format of the commit message.

I'm not sure I can follow here. How's that related to commitlint specifically vs. any other conventional commits checker?

sschuberth commented 1 year ago

I can use the commitlint CLI to create a post commit hook.

Ok, if it's about CLI support, how about using https://github.com/cocogitto/cocogitto instead? That's at least written in Rust instead of TypeScript... 😉 Its accompanying https://github.com/cocogitto/cocogitto-bot also comments about failures instead of hiding the cause in logs.

mnonnenmacher commented 1 year ago

I'm open to alternatives, but they must be as easy to run locally as commitlint (e.g. npx commitlint --from HEAD~2). Ideally, it should be possible to integrate it with Gradle to not depend on more tools (like currently npx).

MarcelBochtler commented 1 year ago

I can use editor plugins (e.g. for vim / IntelliJ) that already verify the format of the commit message.

I'm not sure I can follow here. How's that related to commitlint specifically vs. any other conventional commits checker?

I'm mostly talking about CLI tools, but the commitlint ecosystem also offers plugins for multiple editors: For instance IntelliJ or VSCode, and they might even be smart enough to use the .commitlintrc.yml to apply the repositories commitlint settings.

But more important for me is that a CLI tool exists that can use something like the .commitlintrc.yml to ensure locally that the last commit is correct and complies with the repositories commitlint settings.

As conventional commit can easily be checked via regexes, I propose to instead switch to a generic tool like https://github.com/GsActions/commit-message-checker that can also be configured to check for some of our other non-conventional-commits requirements.

IIUC this would rely on regexes written in the CI definition files, which couldn't be easily reused by any local tools.

sschuberth commented 1 year ago

Ideally, it should be possible to integrate it with Gradle to not depend on more tools (like currently npx).

Maybe https://github.com/nicolasfara/conventional-commits?

mnonnenmacher commented 1 year ago

The plugin does not seem to be very configurable. To me we could just close this issue as I hadn't had any issues with commitlint for a while and am using it in multiple repositories now.

sschuberth commented 1 year ago

The plugin does not seem to be very configurable.

What other configuration beyond the available options would you expected from a tool that is specialized on conventional commits (not commit syntax in general)?

To me we could just close this issue as I hadn't had any issues with commitlint for a while

Once you configure around all the quirks in commitlint it somewhat works, yes. But my point is that there are too many quirks. IMO it's a badly written tool. So I'd like to keep the issue open for a while more.

mnonnenmacher commented 1 year ago

What other configuration beyond the available options would you expected from a tool that is specialized on conventional commits (not commit syntax in general)?

I would like the tool to verify not just the conventional commit types and scopes but also the whole commit syntax indeed. So line length, casing, and so on. Also, it seems the linked Gradle plugin does only work as a commit hook, I would not like that.

sschuberth commented 1 year ago

hadn't had any issues with commitlint for a while

Here's again a bogus error I'm running into. I'd really still like to get rid of commitlint.

mnonnenmacher commented 1 year ago

Here's again bogus error I'm running into.

Issue references are part of the footer so it's complaining because there is no empty line before "Fixes #7366". The error message is not helpful though.

sschuberth commented 1 year ago

Issue references are part of the footer

What? No, why? Only my Signed-off-by is a footer line. I believe this is simply https://github.com/conventional-changelog/commitlint/issues/896.

sschuberth commented 1 year ago

Issue references are part of the footer

What? No, why? Only my Signed-off-by is a footer line. I believe this is simply conventional-changelog/commitlint#896.

Ah, you might be confused by yet another bug which adds blank lines in the error output where there are none: https://github.com/conventional-changelog/commitlint/issues/3129

mnonnenmacher commented 1 year ago

Commitlint defaults to the conventional-commits-parser: https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/parse#api

And this interprets issue references to be part of the footer: https://github.com/conventional-changelog/conventional-changelog/blob/master/packages/conventional-commits-parser/README.md#usage

The issue will go away if you add an empty line before "Fixes #7366" (like we usually format it anyway).

Ah, you might be confused by yet another bug which adds blank lines in the error output where there are none: https://github.com/conventional-changelog/commitlint/issues/3129

This bug makes it worse because the actual issue is not visible in the error output because of it.

IMO it's a badly written tool.

I do agree with that, it seems to be overly complex for the problem it's trying to solve and the documentation is terrible.

I'd really still like to get rid of commitlint.

If there was an alternative that fulfills the requirements mentioned in the messages above I would be happy to switch.