Enforce Conventional Commits

[wemeetagain]

There are some possible benefits to using/enforcing conventional commits. Pls discuss

Github action to enforce?

[dapplion]

In meetings it was agreed to not enforce at the commit level. Maybe at the PR title level (see #1812), but with our regular bi-weekly release schedule and not yet following semver's breaking policy it's not necessary

[philknows]

Reopened based on our Standup call from March 28. We would like to have better and more human-readable updates to our releases. A manual release summary and enforcing conventional commits on our monorepo should better organize our changelog and make it more useful for users.

We should:

• Add a Github action to enforce
• Update our contributions.md to reflect this change

Quick examples

• feat: new feature
• fix(scope): bug in scope
• feat!: breaking change / feat(scope)!: rework API
• chore(deps): update dependencies

Commit types

• build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
• ci: Changes to CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
• chore: Changes which doesn't change source code or tests e.g. changes to the build process, auxiliary tools, libraries
• docs: Documentation only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that neither fixes a bug nor adds a feature
• revert: Revert something
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
• test: Adding missing tests or correcting existing tests

More information on the spec here: https://www.conventionalcommits.org/en/v1.0.0/#specification

[maschad]

Thanks for reopening this @philknows ! It will really help with standardizing our changes. A few points:

• I think we can distinguish deps a bit more to have it as it's own commit types i.e. deps (dev): or deps!: if upgrading the dep introduces a breaking change.
• We should ensure we outline all aspects of the specification that we want to enforce.

We can setup a husky pre-commit hook to enforce this once we agree upon it.

[nflaig]

deps!: if upgrading the dep introduces a breaking change.

Just using ! for breaking change should not be allowed at all and not sure to be honest why this is even allowed as per spec, we should enforce that commits that introduce breaking changes must include BREAKING CHANGE in the description with a thorough explanation of why it is breaking and what changes are required to make it work again. Adding the ! in addition is good though because you can spot breaking commits just based on the message.

As for production dependencies, I would just use fix(deps). I would also not define fix as a bug fix, sometimes it is just a fix/improvement, it is not always related to a bug.

We can setup a husky pre-commit hook to enforce this once we agree upon it.

Would not enforce this on commit level, we anyhow squash-merge. Need to enforce it on PR title which is also great because it gives contributors more freedom + git hooks are kinda annoying

[maschad]

Just using ! for breaking change should not be allowed at all and not sure to be honest why this is even allowed as per spec, we should enforce that commits that introduce breaking changes must include BREAKING CHANGE in the description with a thorough explanation of why it is breaking and what changes are required to make it work again.

This is outlined in the spec I was not suggesting that we solely use the ! symbol.

Adding the ! in addition is good though because you can spot breaking commits just based on the message.

That's why I included it in my example.

I would also not define fix as a bug fix, sometimes it is just a fix/improvement, it is not always related to a bug.

Give an example of this. A fix that is related to a bug rather an improvement could be classified as a refactor or perf so I think fix relating specifically to bugs is appropriate.

Would not enforce this on commit level, we anyhow squash-merge. Need to enforce it on PR title which is also great because it gives contributors more freedom + git hooks are kinda annoying

As @philknows suggested we would have a Github action to enforce it in the PR title, but enforcing it at the commit level is actually to assist in the PR review process, particularly when reviewing larger PRs, having a good commit etiquette can help people to review PRs faster. I agree it may be a deterrent for new contributors though so perhaps it can be optional, it would certainly be beneficial for maintainers though.

[nflaig]

A fix that is related to a bug rather an improvement could be classified as a refactor or perf so I think fix relating specifically to bugs is appropriate.

perf is really specific category, even though in Lodestar there are quite a lot of commits that fit

refactor is code structure only, meaning the behavior must not change (from outside view). This also means refactor type commits are omitted in the release trigger and changelog, as nothing really changed, code is just (hopefully) more maintainable now.

I like to think about conventional commits in combination with semantic versioning which makes it easier to reason about

Commit Types & Release Triggers/Changelog
|
|-- Release Triggers & Included in Changelog
|    |
|    |-- Major Release
|    |    |-- Breaking Changes (breaking: true)
|    |
|    |-- Minor Release
|    |    |-- New Features (type: 'feat')
|    |
|    |-- Patch Release
|         |-- Reverts (revert: true)
|         |-- Bug Fixes (type: 'fix')
|         |-- Performance Improvements (type: 'perf')
|
|-- No Release Trigger & Not included in Changelog
     |
     |-- Build System (type: 'build')
     |-- Continuous Integration (type: 'ci')
     |-- Chores (type: 'chore')
     |-- Documentation (type: 'docs')
     |-- Code Refactoring (type: 'refactor')
     |-- Styles (type: 'style')
     |-- Tests (type: 'test')

Based on this, let's just think about a change in isolation, if there was only one commit, what kind of release should it trigger if at all. This eliminates refactor right there for any sort of improvements that affect users because you would want those to trigger a release.

Now we can of course re-write the rules or add our own rules that fit the project better but I would suggest sticking as close to the standard as possible.

I think fix relating specifically to bugs is appropriate.

Then there won't be a lot of fix commits (hopefully) if we define a bug as an error or flaw in the code that causes the program to behave unexpectedly or incorrectly.
Also noting that feat should really be reserved for actual new features and not for minor improvements so practically there is no type we can use for a lot of commits if we define fix as bug fixes only.

Give an example of this.

Sure, these are few of the recent commits which are not a bug fix but should be of type fix as we would want them to trigger a patch release (if in isolation)

• Enable metrics if monitoring endpoint is configured
• Only preaggregate attestations if there are connected aggregators (or perf)
• Limit preaggregating attestations (or perf)
• Log index if attestations are published per committee
• Always add gossip attestations to forkchoice (or perf)
• Log monitoring errors as warning and reduce verbosity
• Log expected api errors as warning instead of error

but enforcing it at the commit level is actually to assist in the PR review process, particularly when reviewing larger PRs, having a good commit etiquette can help people to review PRs faster

In theory I would agree but you can do good commits without following conventional commits and you can do bad commits even when following it. I would prefer to keep a feature branch more like a "playground" with loose restrictions.

As @philknows suggested we would have a Github action to enforce it in the PR title

sounds like the ideal solution to me, we get the benefits from using conventional commits without introducing too much overhead

[maschad]

A fix that is related to a bug rather an improvement could be classified as a refactor or perf so I think fix relating specifically to bugs is appropriate.

I think there was a grammatical error here that caused confusion, it should say A fix that is NOT related to a bug BUT rather an improvement could be classified as a refactor or perf so I think fix relating specifically to bugs is appropriate.

Based on this, let's just think about a change in isolation, if there was only one commit, what kind of release should it trigger if at all. This eliminates refactor right there for any sort of improvements that affect users because you would want those to trigger a release.

A changelog should contain all changes in order to provide visibility and improvements of a codebase (not just to users but also other developers and potential contributors), that would include refactors, CI changes, documentation changes etc.

In the case of a refactor or a series of refactors, we probably would not do a release, but that is independent of what should be included in the changelog. In practice it is common to have multiple commits before a release and so whether we are doing a patch, minor or major release will be dependent on bug fixes, features, performance improvements and reverts and the refactors would more than likely be included alongside those changes.

Then there won't be a lot of fix commits (hopefully) if we define a bug as an error or flaw in the code that causes the program to behave unexpectedly or incorrectly.
Also noting that feat should really be reserved for actual new features and not for minor improvements so practically there is no type we can use for a lot of commits if we define fix as bug fixes only.

I don't see this as a problem. I agree in that we would not want to have a lot of bug fixes but there is a possibility at some points we may. I agree that feat should be for any type of new feature but I am not sure what you mean when you say "minor improvements" , those could be be classified as feat as well regardless of how drastic it is.

Sure, these are few of the recent commits which are not a bug fix but should be of type fix as we would want them to trigger a patch release (if in isolation)

All of the listed examples can be articulated as bug fixes or performance improvements (I wouldn't have labelled them refactors so I don't think it's a relevant example) since they are behaviours that were unintended but are now changed, or are improvements in the performance of the software, and so in those cases would trigger a release.

In theory I would agree but you can do good commits without following conventional commits and you can do bad commits even when following it. I would prefer to keep a feature branch more like a "playground" with loose restrictions.

The rationale behind conventional commits is standardize commits so that we have good commits and to reduce the possibility of a bad commit. We agree on what those standards are and therefore determine what is bad and good.

[nflaig]

A changelog should contain all changes in order to provide visibility and improvements of a codebase

Well I guess we just disagree on what a changelog is supposed to do and who the audience should be. IMO it is for consumers of the software and not developers of the software. Therefore adding things not relevant to a consumer is just noise.
Developers can look at the commit history and PRs.

I have used semantic-release for few years and have been involved in that ecosystem for quite a while, so I have rather strong opinions here, let's get input from others, will not come to a consent on this :)

those could be be classified as feat as well regardless of how drastic it is

Hard to define objective rules on how to classify feat but usually looking at a specific PR is should be quite clear, see comment above what I consider fix. A clear feat commit would be adding Lodestar prover for execution api.

All of the listed examples can be articulated as bug fixes

Happy to disagree here as well, I guess we just have different definitions of what a "bug" is, see other comment on how I define it, pretty much like here.

We agree on what those standards are and therefore determine what is bad and good.

Just because you tell developers to prefix their commits with type(scope): doesn't mean all commits magically become good. I have worked with a lot of JR devs while using conventional commits, trust me on this :)

[maschad]

Well I guess we just disagree on what a changelog is supposed to do and who the audience should be. IMO it is for consumers of the software and not developers of the software. Therefore adding things not relevant to a consumer is just noise.
Developers can look at the commit history and PRs.

I think you are conflating release notes with a changelog - (which is understandable as they have been used interchangeably in some cases)

If you examine Google's release-please - which we could actually use to automate changelog generation and release version bumps if we do decide to use Conventional Commit messages and Semantic versioning - there are changelogs which include refactors and build system changes for instance, so I am not sure if the idea that they should be excluded from changelogs is industry standard.

Happy to disagree here as well, I guess we just have different definitions of what a "bug" is, see other comment on how I define it, pretty much like here.

The definition that you posted here (which I also had used to derive my own explanation) states: A software bug is an error, flaw or fault in the design, development, or operation of computer software that causes it to produce an incorrect or unexpected result, or to behave in unintended ways. which was the explanation I had given in stating the following:

since they are behaviours that were unintended but are now changed

so I would say those examples can be articulated as bugs or performance improvements, regardless they should be in a changelog and potential triggers for release.

Just because you tell developers to prefix their commits with type(scope): doesn't mean all commits magically become good. I have worked with a lot of JR devs while using conventional commits, trust me on this :)

The reason behind us standardizing commits is to determine which commits are good as right now there is no codified consensus within the team on what a good commit is. I am not implying that there will not be bad commits, but rather we are reducing that possibility by standardization. If there is more leniency in commit etiquette then there a greater variance in commit standards and subsequently a greater possibility of bad commits. That doesn't mean we have to enforce commit hooks for instance, but we have to be open to the opportunity cost of not doing so.

[wemeetagain]

Yall are covering a lot of ground here XD... will throw in my thoughts:

• RE conv commits on commit level vs PR title level - I favor just enforcing conventional commits on PR titles
  • Reason: We squash all PRs into a single commit, there's not a guarantee we'll maintain all original underlying commit messages for a PR. This will be only a small change to our existing workflow, which will be minimally disruptive for us, but its also convenient for newcomer contributors who aren't familiar with conv commits (we can just edit their PR title once we approve their work).
• RE triggering releases based on specific conv commit types - IMO useful for lower level component libraries, but probably won't be necessary for this repo for a while.
  • Given the rigor of testing required for our releases, I'd recommend just sticking with a biweekly release schedule based on manually curated commits off unstable.
• RE including all changes in the changelog/release notes - I favor including everything, but tailoring the release notes for consumers
  • I love to see the paragraph that @philknows writes for the lodestar-announcements channel as the 'intro' to the release
  • we can add the "unimportant" changes under a dropdown at the bottom
• RE good vs bad commits / PR titles - IMO conv commit titles won't be a silver bullet. We should also separately be conscientious about making titles succinct but also descriptive enough as a summary of the feat/fix/etc. I think this is just very subjective and hard to get right.

Hoping that by using conventional commit PR titles, it will get us more comfortable with conventional commits in general, give us more tools for organizing our releases, and act as a foundation for future release process changes, if / when we need / want to change.

[nflaig]

RE @maschad

I am not sure if the idea that they should be excluded from changelogs is industry standard.

Like I said before my opinions are shaped by semantic-release, whether or not this can be considered an industry standard, I leave up for you to decide

I think you are conflating release notes with a changelog

I don't care what name we use for it, I just care how we can improve what we output here https://github.com/ChainSafe/lodestar/releases (or in a CHANGELOG.md file for that matter) and see if conventional commits are helpful to improve this and how we need to tailor it for it to be valuable

so I would say those examples can be articulated as bugs

the commit Log monitoring errors as warning and reduce verbosity is clearly not a bug, it does not produce an incorrect or unexpected result, and it behaved as intended. Just changed my mind on how this should be logged later. This was a quite opinionated change because I think it improves log UX, fixing a bug is not opinionated because you fix something that is not working as intended and you just make it work as it was supposed to in the first place.

---

RE @wemeetagain

RE triggering releases based on specific conv commit types

definitely, was just giving a theoretical example how conv commit are often used for clarification

I favor including everything, but tailoring the release notes for consumers

Maybe if we have different sections in the changelog this will be bearable and people will dare to read the changelog.

Technically, this also easy to realize because we basically only need conv commits to generate release notes, could just use https://github.com/semantic-release/release-notes-generator out of the box and define our own preset (see options), or just write a script ourselves, sounds simple enough.

[maschad]

Like I said before my opinions are shaped by semantic-release, whether or not this can be considered an industry standard, I leave up to you to decide

semantic release is intended to generate release notes as it states in it's description, which is distinct from the changelog.

I don't care what name we use for it, I just care how we can improve what we output here ChainSafe/lodestar/releases (or in a CHANGELOG.md file for that matter) and see if conventional commits are helpful to improve this and how we need to tailor it for it to be valuable

We intentionally make a distinction between the two, as demonstrated in our last release as @wemeetagain mentioned the release notes help the consumers see what is relevant to them.

the commit Log monitoring errors as warning and reduce verbosity is clearly not a bug, it does not produce an incorrect or unexpected result, and it behaved as intended. Just changed my mind on how this should be logged later. This was a quite opinionated change because I think it improves log UX, fixing a bug is not opinionated because you fix something that is not working as intended and you just make it work as it was supposed to in the first place.

And one could make an argument that the way it ought to have worked in the first place was to have been less noisy and thus it was a UX/DX bug fix, regardless this is an unnecessary point to dwell on and has been discussed ad nauseam I think we can agree that bug fixes and features should be highlighted in release notes.

[nflaig]

I think we can agree that bug fixes and features should be highlighted in release notes.

agreed

@wemeetagain mentioned the release notes help the consumers see what is relevant to them.

I think @wemeetagain meant both the title/introducing aka lodestar-announcement done by @philknows + the Changelog (to be generated from conv commits) should be tailored towards consumers.

Nothing further to add, like I said just care about final/combined result of https://github.com/ChainSafe/lodestar/releases

semantic release is intended to generate release notes as it states in it's description, which is distinct from the changelog.

I leave it at that

[nflaig]

Regarding the tooling, even though we could use semantic-release with just the release notes/changelog generator to generate the Changelog, it seems suboptimal to me as customization is not as easy.

Something like standard-version (or even conventional-changelog) seems to do just what we require and is much leaner. However, there is a deprecation warning and the author suggests to use release-please which @maschad also mentioned already, seems straight forward to customize as well.

[matthewkeil]

Regarding the tooling, even though we could use semantic-release with just the release notes/changelog generator to generate the Changelog, it seems suboptimal to me as customization is not as easy.

It was a bit hard to read the output for semantic-release if I'll be honest (was part of the rush.js monorepo tool used in prior project). It also kinda just gave what it gave when we played with making the output better.

I def agree that release-please looks like a better option

[philknows]

Closed via #5342