ci: Add Conventional Commit Linter

[arielvalentin]

I have made too many mistakes approving and merging PRs that lack conventional commits.

This Action should mitigate those mistakes going forward.

Failure

Success

[scbjans]

fix, feat and perf are missing (of which the first two should be included by default; but it might be nice to be explicit).
Since include-commits: true, I expect squash should also be included here.

Lastly, I believe a token is needed to validate the commits associated to the PR

[arielvalentin]

Makes sense. I did was not certain that it was necessary to make a distinction between perf and fix and I am on the fence about whether refactor and test are useful to make distinctions from CI or fix.

Do we gain any additional insights supporting the additional types?

Since include-commits: true, I expect squash should also be included here.

Fair. I use squash all the time and probably do not need it to fail the linter check; though I do want it to be more strict when merging to main

Lastly, I believe a token is needed to validate the commits associated to the PR

For some reason I thought it used a default token! Thanks for the pointer.

[scbjans]

Do we gain any additional insights supporting the additional types?

I'm not sure; but I do think that since these are conventional, that we should support them.
Especially since we encourage people to use them, CI shouldn't fail on those.

[kaylareopelle]

Especially since we encourage people to use them, CI shouldn't fail on those.

Great catch. I'm comfortable limiting the list to just those included in our CONTRIBUTING documentation. However, I could see ci and release being very helpful for maintainers, but not for contributors.

[Kevin-de-Jong]

By default, the Convention Commits specification does not impose any restrictions on the type - it only requires feat and fix as these have a special purpose (in relation to SemVer).

Therefore, CommitMe will not impose any restrictions on the type unless you provide an additional list of types to restrict to.

Typically, projects are using the Angular definition of types;

• build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
• ci: Changes to our CI configuration files and scripts (examples: CircleCi, SauceLabs)
• docs: Documentation only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that neither fixes a bug nor adds a feature
• test: Adding missing tests or correcting existing tests

(taken from: https://github.com/angular/angular/blob/68a6a07/CONTRIBUTING.md#-commit-message-format)

Note

The same principle true for the scopes configuration parameter (no restrictions, unless configured)

[arielvalentin]

@kaylareopelle @scbjans Seems like default merge commit messages are not allowed because only Squash Commits are allowed.

It also means revert commits will need to be formatted differently.

That will mean we have to rebase whenever we sync branches. Do you all think that is OK?

https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/7483619858/job/20369149719

[scbjans]

@kaylareopelle @scbjans Seems like default merge commit messages are not allowed because only Squash Commits are allowed.

It also means revert commits will need to be formatted differently.

That will mean we have to rebase whenever we sync branches. Do you all think that is OK?

@arielvalentin Apparently Merge commits are allowed, but with our current configuration we cannot set include-commits: true (because of the merge commits with a non-conventional commit message).

Would it be possible to update the repository and configure the merge_commit_message (sorry, can't deeplink) to BLANK?

IIUC a rebasing merge strategy would mean (potential) contributors need to reset their local branch upon each branch sync, that does not seem desirable to me.

[arielvalentin]

I believe the docs you're referencing are for the defaults values used when merging a PR to a target branch but I'm not aware of how to set the default merge commit message when syncing a fork or branch.

I feel like we have limited options here. I am a fan of rebasing but also am sympathetic to how tricky it can get when dealing with conflicts.

I want to scan commits because it's what the release tooling uses at the moment but we also squash and merge to main so maybe that's enough?

I'll disable commit scanning and see how this works purely with PR title scans.

[kaylareopelle]

1. style and revert are both listed in ci: Add Conventional Commit Lint #821, but aren't present here. Do we want to include them?

2. Does this get around merge commit validations, or do we need to add those too?

3. Last thing, test is in this list and it's also in the Conventional Commits Angular documentation, but our CONTRIBUTING.md file currently lists tests, plural.

Looking through our commits, it seems like we've used both forms, but test is more common. I prefer to go with test and update our contributing docs.

[arielvalentin]

style and revert are both listed in #821, but aren't present here. Do we want to include them?

👍🏼

Does this get around merge commit validations, or do we need to add those too?

Yes! Where it did not support it in my initial tests, it does now. https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/7614598575/job/20737243103

Last thing, test is in this list and it's also in the Conventional Commits Angular documentation, but our CONTRIBUTING.md file currently lists tests, plural.

👍🏼

[kaylareopelle]

🎉

[scbjans]

Been thinking about this a little longer. Functionally we only need PR title validation for the release tooling?
If so, maybe we should not include commits at this stage since it might break contributor workflows?
We can enable that setting later on if we want?

[arielvalentin]

Been thinking about this a little longer. Functionally we only need PR title validation for the release tooling? If so, maybe we should not include commits at this stage since it might break contributor workflows? We can enable that setting later on if we want?

I brought this up during the SIG. I do not think folks had a strong opinion either way, however during the SIG @mwear also shared a preference to using client-side git-hooks to validate individual commit messages. This would reduce friction for contributors, who would get an error message earlier in the process and avoid generating non-conventional commits.

It would not enforce commits coming from say the PR UI suggestions but as you stated, perhaps all we need is PR title validation. That would at least mitigate issues with maintainers (mostly me) end up merging bad squashed commits.

[scbjans]

using client-side git-hooks to validate individual commit messages.

That sounds like a great idea!

[scbjans]

LGTM

In addition to that, I would like to follow up on @mwear suggestion to install client side git-hooks before enabling commit validation in the action.

Let's implement that in a separate PR.