Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

chore: adopt conventional commits standard #666

Merged
merged 3 commits into from
Aug 28, 2017
Merged

chore: adopt conventional commits standard #666

merged 3 commits into from
Aug 28, 2017

Conversation

ChristianMurphy
Copy link
Contributor

@ChristianMurphy ChristianMurphy commented Jul 27, 2017

Hey @UW-Madison-DoIT/uportal-home-committers 👋

I've been thinking lately about how to manage releases, while looking at how other open source projects have been approaching the problem, I came across conventional commits. 👀

The goal the conventional commits standard is to simplify management of semantic versioning. Metadata provided by commit messages is used to guide releases as MAJOR, MINOR, or PATCH. As well as give details needed to create CHANGELOGs in a rapid release cycle. ⏩

It seems like the standard could be a good fit with the existing processes and culture at Madison. ⭐
I'd be interested to hear your thoughts on whether adopting conventional commits could be a good idea. 💭

Technical changes in this PR:

  • Add commitlint to check commit messages
  • Add git hook from husky to trigger commitlint on every change
  • Add commitlint config angular, which checks conventional commits, and comes with preset types to use with conventional commits.

Contributor License Agreement adherence:

The goal of adopting the conventional commits standard is to simplify
management of semantic versioning. The metadata provided by commit
messages can guide which releases are MAJOR, MINOR, and PATCH. As well
as give details needed to create CHANGELOGs in a rapid release cycle.
To further aid the process, this adopts the types and formatting
recommendations laid out by the Angular project.
@jimhelwig
Copy link
Contributor

I am not familiar with the details of this but the general philosophy as I understand it sounds good.

@davidmsibley
Copy link
Contributor

We've been interested in things that would allow us to automate (or at least simplify) changelogs. This looks like it's worth some research. However, I hesitate to approve just because of the PR number 😈

@apetro
Copy link
Contributor

apetro commented Jul 27, 2017

How does this relate to my habit of
squash-and-merge such that the commits and commit messages of the feature branch don't end up in master, just a single squash commit? Hard to lint the squash commit commit message since it doesn't exist to be linted until it hits master.

@ChristianMurphy
Copy link
Contributor Author

It would work okay.

I believe "Squash and Commit" allows for editing the message, so the messages could still follow the conventional commits format. 📝
You are right that those commits would not be automatically checked by the commit linter.
If the lack of linter for "squash and merge" is perceived as a high risk, the button can be disabled from the repository settings. ⚙️
I personally don't see that as a high risk. ✅

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Jul 27, 2017

Also to be clear.
I don't think conventional commits is the only way to handle this.
Nor do I know if it is the best way.
It is just a way that several open source projects have used successfully.

Another could be adding a CHANGELOG.md file and requesting contributors add notes as they add fixes and features.
Keep a Changelog, advocates for this approach: http://keepachangelog.com/en/1.0.0/
Projects adopting that technique have also been successful.

@apetro
Copy link
Contributor

apetro commented Jul 28, 2017

So, the label the pull requests solution, that's not sufficiently simplifying figuring out patch vs minor vs major in releasing. Because we're not doing it. That practice isn't documented in this repo, and it's not reminded of in the contributing guidance or the pull request template. So. Would that labeling meet the need if we doubled down on consistently doing it?

@vertein
Copy link
Contributor

vertein commented Jul 28, 2017

In practice, we haven't been labeling our PR's, and I'm fine with that. It's been easy enough to manually go back and take a look and appropriately version the product.

My thought process is - I like the idea and like the thought of automating some change logs. A little bit of thinking of the type of changes we make is healthy.

I also don't like automated change logs and prefer the human touch in release notes and I'm hesitant to add a check that a developer might have to look into and parse.

So far, I'm at the point where I'm either a +1 or a no-vote on this. I would +1 the addition of a changelog with updated documentation stating our intentions.

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Jul 28, 2017

🤔 @apetro It would help.
The challenge with pull requests, is they don't correlate to versions as cleanly as commits.
GitHub doesn't provide a UI to see which release a PR is in, you need to know to look for the merge commit, and check the included tags on that.

However, If both labels, and a milestone for the next release are used, that could work out nicely. 👍

The way I look at it there are two differentiating factors between: conventional commits, keep a changelog, and tag + milestone.

  1. Is the Contributor or the Maintainer responsible for adding the data?
  2. Is the data optional or required?

For Conventional Commits:

  1. Contributor
  2. Required

For Keep a Changelog:

  1. Contributor
  2. Optional

For Tag + Milestone:

  1. Maintainer
  2. Optional

I personally lean toward having Contributors add the information, and Maintainers review it.
I'm not the fence on if it should be optional or required (logs for small changes may not add value for and adopter reading the changelog).

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Jul 28, 2017

@vertein

I also don't like automated change logs and prefer the human touch in release notes and I'm hesitant to add a check that a developer might have to look into and parse.

I completely agree that a human touch is needed.
Squashed and well summarized pull requires can help the automated logs to be more readable.
Adding a completely human written summary of the broad strokes in the changelog can help as well.
The Atom editor's change logs have a human written summary at the top, and the automated listing below.
https://github.com/atom/atom/releases/tag/v1.18.0

@ChristianMurphy
Copy link
Contributor Author

ChristianMurphy commented Aug 4, 2017

I also don't like automated change logs and prefer the human touch in release notes and I'm hesitant to add a check that a developer might have to look into and parse.
So far, I'm at the point where I'm either a +1 or a no-vote on this. I would +1 the addition of a changelog with updated documentation stating our intentions.

@vertein I've opened #671 to add a human maintained CHANGELOG.


Even without it being used to create CHANGELOGs automatically.
I still see the metadata included in conventional commits as having merit on its own.

For example picking at one of my own commit messages. fc86bf6

Cache dependencies to lower download time on build

The message does communicate the intent, reducing build time.
It communicates how, though using caching.
However it does not clarify that it only affects continuous integration builds.
If the message were:

ci(travis): Cache dependencies to lower download time on build

It would effectively communicate the scope of the change.

commitizen helps automatically generate conventional commit messages
@ChristianMurphy
Copy link
Contributor Author

I've also added commitizen, a CLI tool that can help generate conventional commit messages.
Below is a demo of commitzen generating ad96e05

commitizen-demo

@apetro
Copy link
Contributor

apetro commented Aug 10, 2017

In-scope for backlogged MUMMNG-3765, which would track in MyUW Scrum allocating MyUW team attention to engaging with this. This isn't to say this can't naturally go to completion sooner; it is to say that it shouldn't be able to languish many more weeks before Scrum marshals engagement from MyUW team members.

Copy link
Contributor

@vertein vertein left a comment

Choose a reason for hiding this comment

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

Looks good to me. I pulled this down and played with a bit. I'm happy that we added commitzen. From my few tests, it seems pretty easy to work with.

Unfortunately since this was out here for a while, there are some new merge conflicts.

@ChristianMurphy
Copy link
Contributor Author

Resynced with master branch.

@apetro
Copy link
Contributor

apetro commented Sep 6, 2017

uPortal-home is now recognized on https://conventionalcommits.org/ .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants