ci: Enforce Conventional Commit PR titles

[DiamondJoseph]

Closes #254

Enforces conventional commit messages on this repository, to allow greater automation of change logs, releases etc.

[GDYendell]

Is it possible to do this as a pre-commit check?

[DiamondJoseph]

Is it possible to do this as a pre-commit check?

Looks like yes, but do we want to enforce that every commit is a conventional commit, or just the commits that get added to main? I've made both of my commits in this PR as conventional commits, but I didn't need to if this PR gets squash-and-merged, as the PR title will become the commit message.

[callumforrester]

@DiamondJoseph some teams prefer to rebase and merge than squash and merge, so for them all commits will make it to main, so yes I think enforce per-commit. The copier template is not opinionated about it.

[GDYendell]

I've made both of my commits in this PR as conventional commits, but I didn't need to if this PR gets squash-and-merged, as the PR title will become the commit message.

Does that work? You will have to click merge when the CI is failing if not all commits are valid.

To be clear, will merging this apply it only to this repo or also to the template itself?

[DiamondJoseph]

@DiamondJoseph some teams prefer to rebase and merge than squash and merge, so for them all commits will make it to main, so yes I think enforce per-commit. The copier template is not opinionated about it.

This PR is not enforcing conventional commits for all repositories using the template, just for the template repository itself, where I think we should be opinionated about squashing PRs: so we can have this repository as automated as possible so it is as unbiased by individual human opinions as possible.

[DiamondJoseph]

Does that work? You will have to click merge when the CI is failing if not all commits are valid. To be clear, will merging this apply it only to this repo or also to the template itself?

The action only checks the PR title, not the contents of each commit and so it won't be failing if the PR title is correct even if the commits are incorrect. Merging this will only apply it to this repo and not repositories making use of the template: it isn't symlinked into the template.

[DiamondJoseph]

I've added a demonstrative non-Conventional commit to this PR to show that the action remains passing.

[joeshannon]

Presumably someone could still change the commit message when squash merging? I've been curious to know if there's a solution to that.

[DiamondJoseph]

Yes, you can still reword during squashing- would need to at least add something (to the PR template?) to explain not to do that,

[callumforrester]

Presumably someone could still change the commit message when squash merging? I've been curious to know if there's a solution to that.

Feels like at that point it's more malicious than accidental though?

[joeshannon]

Presumably someone could still change the commit message when squash merging? I've been curious to know if there's a solution to that.

Feels like at that point it's more malicious than accidental though?

Yes pretty much, unless they have different workflows between projects and get mixed up etc.

This seems like a good compromise; maybe a merge bot would solve this problem but it is getting quite involved at that point.

[coretl]

Bear in mind that (in the copier template repo) we only allow PRs to be merged with a merge commit, not a squash or a rebase. This is because we encourage people to demonstrate their branch is adopted in another project before merging into copier template, and if we allowed squash or rebase then they would have to sort merge conflicts when doing this.

Does this affect the PR at all?

[GDYendell]

if we allowed squash or rebase then they would have to sort merge conflicts when doing this.

Why would there be conflicts?

[callumforrester]

@GDYendell If the changes are adopted in another project as a stack of, say, 3 commits, which are then squashed into 1 commit, that will cause a conflict the next time that project is updated. That said, that sounds like a good reason not to require that changes have been demonstrated in a production project in that way. I would prefer people trial features they want in projects, but not necessarily link the commits, e.g. DiamondLightSource/blueapi#929

That said, what is the envisioned use of conventional commits for this repo? The impression I had was that they were good for projects with a frequent release cycle and a high impetus to get changes into production quickly. The copier template is relatively stable and releases are quite conservative at the moment. If we want that to change I would support adopting CC, but do we?

[DiamondJoseph]

@callumforrester you're currently trying to get automated copier updates with renovate, being consistent and picky with versioning means adoption of those updates can be more trustworthy

[DiamondJoseph]

I have additionally added a new ADR for supporting NEP 0029 as discussed on #237, #240.
When dropping support for python 3.9 (#240, 2.2.0, this was a minor release but if we are to adopt SemVer, it should* have been a major release. When dropping support for python 3.10, this was a major release.

• I believe. I wasn't 100% certain, so I gathered opinions and looked around online. If it were a library people seem mostly convinced it's a breaking change, but for an application I would personally only make a patch version. This is in effect a library (?)

[DiamondJoseph]

@coretl I'd like (as of this change) to switch to default being "Squash and Merge" so that the PR title is the commit title and the conventional commits make it to the release notes- I cannot Squash and Merge

[GDYendell]

I don't feel strongly about this, but it is not obvious to me that dropping support for a python version should be a major version bump.

[DiamondJoseph]

[...] it is not obvious to me that dropping support for a python version should be a major version bump.

It also wasn't obvious to me, so I deliberated for a while, asked other people in the office and looked at what others have said and done online. A general consensus, which wasn't a particularly strong one, was that for libraries it could/should be a major bump, as if an application was tied to a particular version it is a breaking change to drop support for that version. In that respect, the copier template is very library-like. For an application it could just be a patch change like any other dependency, because the application gets to choose what versions it supports and if you need to install a new version of python to run it, it doesn't break any of the expectations of the application itself.

Our CI pipelines use the supported versions of python to perform linting- when we dropped support for python 3.9 in the past, the linting started failing because it was opinionated that when you weren't supporting <=3.9 Union[X, Y] Should be replaced with X | Y. As we're automating more, refactoring all of the Python type system for a "patch" or "feature" release seems wrong, and being too cautious with versioning risks people tuning out where being less cautious risks breaking changes making it into production.

I think because it isn't something that is not immediately clear or obvious it should be documented to enforce consistency. I 100% agree that it isn't obvious, but I also don't think it's obviously wrong.

[GDYendell]

Makes sense. It is good that the discussion has been had. I don't think I have anything specific to add for or against. That the template is opinionated such that the linting standards change with the minimum supported version seems like a pretty good reason to me.

[callumforrester]

My $0.02: It is a major release. This template is opinionated but also completely customisable, which means most changes will break someone's workflow somewhere. Semantic versioning is meant to be done against a defined API, but there's not really any such thing here.

[coretl]

@DiamondJoseph feel free to squash merge away...