yari icon indicating copy to clipboard operation
yari copied to clipboard

feat(pre-commit): enforce commit signing

Open caugner opened this issue 2 years ago • 4 comments

Summary

Problem

From time to time, new contributors open PRs on yari that include unsigned commits, and it is not trivial to sign the existing commits.

Solution

Prevent committing unsigned commits by verifiying that commit signing is enabled in the pre-commit hook.


Screenshots

Before

$ git config commit.gpgSign 0
$ git commit --allow-empty -m 'test'
yarn install v1.22.19
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
success Already up-to-date.
✨  Done in 0.26s.
yarn run v1.22.19
$ /Users/claas/github/mdn/yari/node_modules/.bin/lint-staged
→ No staged files found.
✨  Done in 0.33s.
[main 4dff57424] test

After

$ git config commit.gpgSign 0
$ git commit --allow-empty -m 'test'                                                                                                                                                                                                                                       
ERROR: Commit signing is NOT enabled!
Please note that we require that all commits are signed.
Please see the documentation about signed commits and how to sign yours on GitHub:
- https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification
- https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
husky - pre-commit hook exited with code 1 (error)

How did you test this change?

See above:

  1. Disabled commit signing: git config commit.gpgSign 0
  2. Tried to commit: git commit --allow-empty -m 'test'

caugner avatar Oct 13 '23 11:10 caugner

I'm a bit on the fence about this one: as I see it, the point of code signing is to ensure that code that claims to be committed by e.g. me, Leo McArdle, is actually committed by me, and can be trusted.

But if an anonymous contributor opens a PR, I don't see value in that commit being signed - what surely matters more is that whoever reviewed that code has some kind of verifiable cryptographic signature, and it's easier IMO to squash a set of commits before merging, signing them with your own key, than taking a contributor through the steps of setting up PGP.

LeoMcA avatar Oct 13 '23 12:10 LeoMcA

In the past, there were a handful of cases where new contributors opened a PR with unsigned commits, and these were rarely trivial to resolve, mostly requiring rounds of back and forth. This PR avoids those situations by making new contributors set up commit signing before they do their first commit.

@LeoMcA Are you suggesting an alternative, to keep the status quo as is, or lifting the signed commits requirement?

caugner avatar Oct 13 '23 12:10 caugner

these were rarely trivial to resolve, mostly requiring rounds of back and forth

My suggestion is that if a new contributor submits a PR with unsigned commits, we just sign it on their behalf, which requires no back and fourth.

This PR avoids those situations by making new contributors set up commit signing before they do their first commit.

I'm pretty sure I've seen a fair few PRs which weren't prettier-ed correctly, so I'm not sure this guarantees that commits will end up signed. I'm also not sure we want to block code contributions on the sometimes complex setup of GPG.

I'm on the fence here, but I think what would put me in the "yeah why not" camp would be the ability to turn this check off with the presence of a .nogpg file or something.

@LeoMcA Are you suggesting an alternative, to keep the status quo as is, or lifting the signed commits requirement?

Keep the signed commits requirement, but in cases where it would be easier, just sign the commits for a contributor - as our signature as reviewer is far more meaningful than there's as a first/second time contributor (past a handful of PRs it's probably worth guiding them through the process of setting it up - as their signature then attaches some meaning)

LeoMcA avatar Oct 16 '23 10:10 LeoMcA

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Jan 29 '24 13:01 github-actions[bot]