Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: introduce conventional commit guidelines #277

Merged
merged 1 commit into from
Aug 16, 2023
Merged

docs: introduce conventional commit guidelines #277

merged 1 commit into from
Aug 16, 2023

Conversation

jnsgruk
Copy link
Member

@jnsgruk jnsgruk commented Aug 15, 2023

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

Copy link
Contributor

@barrettj12 barrettj12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it :) what do you think about adding some "linting" to ensure commits follow this style? Cause I'm definitely gonna forget the first few times.

HACKING.md Outdated Show resolved Hide resolved
HACKING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. We will duplicate this on our side.

@benhoyt
Copy link
Contributor

benhoyt commented Aug 16, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

barrettj12 commented Aug 16, 2023

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
Copy link
Member Author

jnsgruk commented Aug 16, 2023

@benhoyt 💯 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 :)

@jnsgruk jnsgruk merged commit 55fdf50 into canonical:master Aug 16, 2023
13 checks passed
@jnsgruk jnsgruk deleted the conv-commits branch August 16, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants