canonical / pebble

Pebble is a lightweight Linux service manager with layered configuration and an HTTP API.
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
149 stars 54 forks source link

docs: introduce conventional commit guidelines #277

Closed jnsgruk closed 1 year ago

jnsgruk commented 1 year ago

I'm posting this here for discussion. I've grown to quite like the conventional commit specification. I think it naturally brings consistency to commit messages when reading the short log, and also encourages me personally to think carefully when I'm committing as to how I should break things up.

We've actually got some conventions emerging, but because we're not enforcing it, the commit log is starting to look a little untidy to my obsessive eye.

Proposing this as a convention - happy for comments etc. If we can agree on the style, we can introduce a CI check.

Including some examples from the source of this commit:

feat: checks inherit context from services
test: increase unit test stability
feat(daemon): foo the bar correctly in the baz
test(daemon): ensure the foo bars correctly in the baz
ci(snap): upload the snap artefacts to Github
chore(deps): update go.mod dependencies
benhoyt commented 1 year ago

I'm fine with this, though per discussion with @jnsgruk, we just don't want it to mean that we fixate on the "punctuation" of the commit message and ignore the meat. For example "fix(api_files): updates" might pass Conventional Commit style but is a terrible commit message, whereas "fix(api_files): use correct mode in Mkdir calls" is fine.

If we use an automatic commit style checker for this, something we'll have to figure out is whether this will then require us to use rebase on all the commits to get them perfect before merging/applying them to the base branch. Because if we continue to use Squash and Merge, then the merger has to type in the commit message/body manually after pressing the big green button, and presumably there won't be a chance for the automatic commit checker to check it then?

My strong preference is using Squash and Merge to reduce a PR to a single commit on the main branch, 1) because GitHub's review workflow doesn't work with rebases that well, and 2) because it's simpler to just keep committing random fixes (with relatively poor commit messages) to the PR branch, and only think about a nice commit message once when you merge.

barrettj12 commented 1 year ago

@benhoyt: "squash and merge" should take the PR title as the first line of the commit message, so if we want some tooling here, we can just check the PR title. Probably it's not necessary to observe this convention for commits which will later be squashed, although maybe it's good practice.

barrettj12 commented 1 year ago

~Just had a thought - on the Juju team, because of our Jira workflow, we are supposed to prefix our PR titles with JUJU-1234 to link it with the Jira card. You can see I've done this on #256 and #267. Is this incompatible with the convention you are proposing here, @jnsgruk?~

~Personally, I've never been a fan of "[JUJU-XXXX] \<PR title>", and I'd be happy to drop it in favour of conventional commits.~

Edit: just found out that JUJU-XXXX doesn't need to go in the title - it's fine to put it in the PR description instead. False alarm.

jnsgruk commented 1 year ago

@benhoyt :100: agreed -- adhering to the format is the easy part, it still requires some thought!

@barrettj12 - indeed the JUJU-XXXX can just go in the commit body - conventional commits are really targeting the summary line.

Agreed generally on squash and merge - it just requires the person pressing the button to pay attention and ensure the message makes sense. While the convention can be ignored on small commits and tidied up with a squash, I find it helps me (personally) to use the convention throughout to just get into the practice :)

I can definitely see the benefit of rebase & merge in some cases; for example:

feat(cli): add standardised json output for all commands
test(cli): ensure json output is correct and parseable
ci: simplify to use json output in smoke tests

Personally - I'd prefer to see that land as is on the main branch than squash it - but I'm not religious about it :)