Commit lint failure should be a warning

[alanshaw]

Failing CI because of commit linting puts up a significant hurdle for new contributors to overcome. In my experience contributors always fail this rule and have to then figure out how to amend their commit message and then how to force push to a branch.

All this has to be done by the new contributor before they can even see if their change passes in CI. This is not an experience that encourages new contributions and is unnecessary considering commit message formats can be trivially fixed by the maintainer when the PR is squashed+merged.

I vote for lowering the severity of this check from error to warning....and I am more than happy to manually communicate the guideline to the contributor in a comment in the PR. Anyone else agree or have an alternative suggestion?

@ipfs/javascript-team

[jacobheun]

Can we add the commitlint step as an allowed failure in Travis, so it's easier to see? I'm all for lowering the barrier of entry for this. My only concern is that if commitlint is a warning it's solely up to the maintainer to ensure they're correcting the commit on squash as we likely aren't going to see it in CI unless it's an error.

This isn't a huge concern though, I'd rather see this be made easier for contributors and risk the occasional oversight on merge.

[achingbrain]

I'm not a fan of commit linting, it adds little value and increases developer friction.

That said it's pretty easy to add commit linting as a pre-push test and catch this stuff before it makes it to CI.

[hugomrdias]

@achingbrain im in favor of a pre-push hook but this option still has the issue @alanshaw is talking about, even though the error printed to the terminal is very helpful and a link is also printed explaining everything.

what do you think, personally i like this ^^ instead of running it in the CI.

https://github.com/conventional-changelog/commitlint#what-is-commitlint

[jacobheun]

but this option still has the issue @alanshaw is talking about, even though the error printed to the terminal is very helpful and a link is also printed explaining everything.

As you mentioned I think this still has unnecessary friction for contributors. I think it printing as a warning is fine, but it shouldn't block pushes.

[hugomrdias]

so commit lint as a warning

should we run it only on ci, only on git hook or both ?

[vmx]

Could the commit message lint be a separate step on travis that could fail independently? This would be a good indicator for the maintainers, but new contributors would still get feedback from the CI if the actual change is correct.

I would then expand it to a general lint step, with the commit message, dependencies and code linting.

[olizilla]

I'm strongly against commitlint errors and warnings showing up on contributors PRs. It adds friction, and is demoralising. I just experienced that trying to offer up an improvement js-cid.

My feeling is that we should be nurturing PRs from the community, and do everything we can to make that a plesant experience. As a maintainer, I review the PR, and communicate with the contributor about it until I am personally happy to merge it and take responsibility for the changes. That includes squashing the commits when merging a PR and ensuring the squashed commit message meets our guidlines.

That being the case, I'd like to reduce commit linting down to just changes to master. I'd be happy to remove it all together.

[vmx]

@olizilla Would that mean that I'd need to manually check a passing CI run to see if things pass the commit lint?

[hugomrdias]

it means you need to squash and change the commit message to something good for the changelog

[vmx]

it means you need to squash and change the commit message to something good for the changelog

Yes, but I won't have an automatic indicator that the commit message didn't conform.

[jacobheun]

To me it feels like the easiest approach for this is to remove commitlint altogether and move to a squash only merge strategy for anyone who isn't already doing this. Lead maintainers should already be aware of the commitlint rules.

We could leave it on for master to at least be notified we're doing it wrong, but there is no corrective path for mistakes, which we have anyway with squashing. Since modifying commits in master is a no no, the best you get here is a note for next time to squash better.

[hugomrdias]

so regarding this issue we are all in agreement right ?
each of us should change .travis.yml accordingly

[jacobheun]

So to be explicit about the change so we can sign off and close this, we are proposing:

1. Removing commitlint from the travis file, - npx aegir commitlint --travis
2. a PR should be made to the readme here to remove it from the template.
3. Maintainers should always squash PRs and use that as a mechanism to create clean, changelog friendly commit messages.
4. We should update https://github.com/ipfs/community/blob/master/CONTRIBUTING_JS.md to mention this.

[vmx]

I'm still in favour of having a commit lint, just making it OK to fail. Even I sometimes forget to lint things locally, so even for myself it's a good indicator whether things are correct or not.

[jacobheun]

I'm still in favour of having a commit lint, just making it OK to fail. Even I sometimes forget to lint things locally, so even for myself it's a good indicator whether things are correct or not.

I think this is going to vary quite a bit by maintainer. I edit nearly every squash I do. Perhaps updating the travis.yml file in the readme here to show how to add that as an allowed failure would be a good middle ground to eliminating it altogether?