ci: Add commitlint to ensure conventional commits

[spetrosi]

Ensure that commits follow conventional commits format Ensure that the PR title follows conventional commits format Ensure that only one commit exists in the PR

[spetrosi]

@richm please review, the action that I am adding here is tested in https://github.com/linux-system-roles/mssql/pull/192. I am using commitlint, that resides in the conventional-changelog organization, so is provided by the same community that does the conventional commits initiative.

In addition to checking if commits meat the format, the action also checks that PR title meats the format, just for consistency. PR title is initially created from the commit, and if the commit was not in the format initially, the PR title might remain unformated.

The action also forces the PR to only have 1 commit, I think this will help us with collecting commits to changelog, wdyt? I think that squashing commits is a rule now too, just not forced by tooling. Hopefully, nobody will have problems with this.

[richm]

@richm please review, the action that I am adding here is tested in linux-system-roles/mssql#192. I am using commitlint, that resides in the conventional-changelog organization, so is provided by the same community that does the conventional commits initiative.

Yes, looks good. Just a few minor comments inline.

In addition to checking if commits meat the format, the action also checks that PR title meats the format, just for consistency. PR title is initially created from the commit, and if the commit was not in the format initially, the PR title might remain unformated.

ok - is it ok if [citest skip] appears in the PR title? Does this mean we can't have PR titles of the form WIP - this is a work in progress, or is there a conventional commits way to do that?

The action also forces the PR to only have 1 commit, I think this will help us with collecting commits to changelog, wdyt? I think that squashing commits is a rule now too, just not forced by tooling. Hopefully, nobody will have problems with this.

I think some of our developers will have a problem with that. It is common when reviewing PRs for developers to submit one or more commits for each PR review comment, then squash them all together when merging. Not sure how to handle this situation. Maybe we simply enforce that all commits have the head and body in the right format. The developer is then responsible for either doing that, or squashing the commits into a single commit with the correct format before merging. Note that there may be some situations where having multiple commits in a single PR, and having all of those commit messages go into the changelog, is desirable.

[spetrosi]

In addition to checking if commits meat the format, the action also checks that PR title meats the format, just for consistency. PR title is initially created from the commit, and if the commit was not in the format initially, the PR title might remain unformated.

ok - is it ok if [citest skip] appears in the PR title? Does this mean we can't have PR titles of the form WIP - this is a work in progress, or is there a conventional commits way to do that?

[citest skip] in the title works fine, tested in https://github.com/linux-system-roles/mssql/pull/192. I add an "edited" PR type trigger to default opened, synchronize, and reopened so that the check runs again after PR title is changed.

WIP - this is a work in progress - this will fail the check indeed, but that's fine to have the check failing until the PR is ready to be merged.

The action also forces the PR to only have 1 commit, I think this will help us with collecting commits to changelog, wdyt? I think that squashing commits is a rule now too, just not forced by tooling. Hopefully, nobody will have problems with this.

I think some of our developers will have a problem with that. It is common when reviewing PRs for developers to submit one or more commits for each PR review comment, then squash them all together when merging. Not sure how to handle this situation. Maybe we simply enforce that all commits have the head and body in the right format. The developer is then responsible for either doing that, or squashing the commits into a single commit with the correct format before merging. Note that there may be some situations where having multiple commits in a single PR, and having all of those commit messages go into the changelog, is desirable.

One concern that I have here is that when you merge commits into one using Squash and merge in the GH UI, users are given a window where they edit their resulting commit themselves, and this resulting message becomes impossible to check because it gets merged right away. Typos in this stage might break the automation. The requirement to have a single commit solves this, but i understand that sometimes you need several commits in a PR.

Okay, I removed the condition to have only 1 commit, the action will check each commit to be in the right format.

[richm]

using github UI to squash and merge allows editing the message, and that is not checked

Hmm - good point - if there really is no way to prevent a bad commit message using Squash&Merge, perhaps we should disable it, and have Rebase&Merge as the only merge option - then devs will need to squash manually and repush in order to squash. Although, it would be nice to have a way for "superusers" to use Squash&Merge with the responsibility of creating a conforming commit message manually - but we already have a way to do that, it's called "go into the repo Settings and change the settings to allow you to do whatever you want before merging" . . .

[spetrosi]

Oh exactly, we should disallow squash merging and instead do Rebase&Merge on all repos. I tend to squash using CLI anyway because some time ago web UI was not very reliable, hopefully it is not a big deal for our devs.

[richm]

Oh exactly, we should disallow squash merging and instead do Rebase&Merge on all repos.

Ok. Please send an email to systemroles@lists.fedorahosted.org to explain the move to using Convention Commit message format, and explain that we will need to disable github UI Squash&Merge because of this.

I tend to squash using CLI anyway because some time ago web UI was not very reliable, hopefully it is not a big deal for our devs.

+1 - the only time UI Squash&Merge is useful is when the developer is unable or unavailable to use the CLI, and it's easier for one of us admins to do it. And, we can temporarily change the repo settings to allow it.

[spetrosi]

I sent an email, there are no replies to it, so I went on and disabled squash and merge on all repos. By the way, network had it disabled already. Let's merge this PR now? My plan is to write some simple scrip to reword commits from the previous tag to comply with conventional commits, so that we can automatically gather changelog for the next release.

[richm]

I sent an email, there are no replies to it, so I went on and disabled squash and merge on all repos. By the way, network had it disabled already. Let's merge this PR now?

Yes.

My plan is to write some simple scrip to reword commits from the previous tag to comply with conventional commits, so that we can automatically gather changelog for the next release.

ok, sounds good.