ci: Check commits in PRs

[realeinherjar]

Closes #1119.

• Creates commits.yml in .github/workflows/ directory with a new GH Action
  that has 2 jobs:

  1. signed-commits: uses [1Password/check-signed-commits-action(https://github.com/1Password/check-signed-commits-action)
     to check if all commits in the PR are signed.
  2. conventional-commits: uses cocogitto/cocogitto-action
     based on cocogitto
     to check if all commits in the PR follows the conventional commits,
     i.e. they are prefixed with fix, refactor, feat etc.

• Clarifies the "Commit policy" CONTRIBUTING.md:

  • Rust-based CLI tool suggestion to locally check for the commits: cocogitto/cocogitto.
  • Suggestion of two git hooks, see below.

• Two Git Hooks
  to the new directory ci/git-hooks

  • signed-commits.sh hook (a pre-push hook)
    will not allow the user to push to remote if the last commit is not signed.
    This is a good sanity test,if the last commit is signed then there is a high chance
    of any other previous are also signed.
  • conventional-commits.sh hook (a commit-msg hook) will not allow the user to commit
    if the commit message does not satisfy the Conventional Commits template.

Notes to the reviewers

Since this is a big enforcement change,
we can for now not make this CI check mandatory for a PR to be merged.
When appropriate we can turn check mandatory.

Also none of these actions have write permissions.

Changelog notice

Added CI checks for signed commits and conventional commits guidelines.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature (in CONTRIBUTING.md)

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[danielabrozzoni]

Heads up, the conventional commits check is failing.

Also, I like this idea from @evanlinjin: https://discord.com/channels/753336465005608961/999098651383189554/1152079701985210408
Do you think we could just have a few scripts checking if commits are signed and conventional instead, so that the check is easy to reproduce locally?

[realeinherjar]

Heads up, the conventional commits check is failing.

Somehow webiny/action-conventional-commits was not published to the github actions marketplace.
I've edited the commits in the PR to use aevea/commitsar instead, which is published.

[notmandatory]

A few comments:

1. Is there any reason to use the signed-commits github action instead of just relying on the github setting to require signed commmits? I'd rather not add a new CI job and plugin if github can do it without
2. If the recommendation is to use commitlint locally shouldn't we use it for the ci job also? https://commitlint.js.org/#/guides-ci-setup?id=github-actions
3. I think the first commit message that starts with feat(CI) should start with just ci: to be to spec. 🙂

[notmandatory]

Another suggestion, the cocogitto tool may be better for a rust project like ours. It also has instructions for integrating with ci.

[realeinherjar]

Another suggestion, the cocogitto tool may be better for a rust project like ours. It also has instructions for integrating with ci.

That's an amazing suggestion. Yeah I much prefer this one! I followed the instructions at https://github.com/cocogitto/cocogitto-action.
Done in 5647563.

2. If the recommendation is to use commitlint locally shouldn't we use it for the ci job also? https://commitlint.js.org/#/guides-ci-setup?id=github-actions

Let's change to cocogitto. Done in ebff4b5.

3. I think the first commit message that starts with feat(CI) should start with just ci: to be to spec.

Done in ebff4b5.

1. Is there any reason to use the signed-commits github action instead of just relying on the github setting to require signed commmits? I'd rather not add a new CI job and plugin if github can do it without

Yes, but that feature is not automatically copied over to forks.
BDK's enforcement would work in bitcoindevkit/bdk but not in someone/bdk.
Hence, this would not be enforced automatically when people apart the maintainers who can have branches at bitcoindevkit/bdk when they commit in their forks.
This would only happen when we would try to merge the PR.
Here's a recent example: https://github.com/bitcoindevkit/bdk/pull/967/commits
The commits were not signed, but the user was able to not only make the PR but also all CI checks passed.

[realeinherjar]

NOTE to myself: cocogitto might check the WHOLE commit history.
If BDK has not always been conventional commits compliant, we migth get a CI commit check failure.
We need to perform check only since the latest tagged version:

      - name: Conventional commit check
        uses: cocogitto/cocogitto-action@v3
        with:
          check-latest-tag-only: true

[notmandatory]

I think the only way to do this would be to use check-latest-tag-only: true and then we have to wait until after our next tag (should be 1.0.0-alpha.2) to merge this PR. Then from that tag forward all commits will be to be compliant.

[realeinherjar]

We can also merge now and not add conventional commits to the mandatory CI checks for PRs to be merged onto master.
Then, 1.0.0-alpha.2 onwards we can enforce this if we want.

[danielabrozzoni]

Is there any reason to use the signed-commits github action instead of just relying on the github setting to require signed commmits? I'd rather not add a new CI job and plugin if github can do it without

The reason behind a CI job would be to make the requirement for signed commits more explicit. What happened to me way to many times is that I would ack a PR and notice only later that the author didn't sign the commits, and usually this means waiting days/weeks before the author sees the comments and does something about it. Instead, if at the time of opening PR the CI fails because of signed commits, the author can quickly fix.

[danielabrozzoni]

Do you think we could just have a few scripts checking if commits are signed and conventional instead, so that the check is easy to reproduce locally?

Btw, we already use the bitcoin core method for merging PRs, using the github-merge.py script, maybe we should consider to also check the commits validity like Bitcoin Core does? In this way, we would add all the docs to verify the commits.
See:

• On verifying commits: https://github.com/bitcoin/bitcoin/blob/master/contrib/verify-commits/README.md
• The CI job: https://github.com/bitcoin/bitcoin/blob/master/ci/lint/06_script.sh#L37

[notmandatory]

Yes, but that feature is not automatically copied over to forks.

This is a good reason to enable it. I've had to remind new committers (and sometimes not new) to sign commits, having CI on their forks to automatically notify folks would be useful.

[realeinherjar]

@danielabrozzoni, I've added two Git Hooks to the new directory ci/git-hooks.
These are bash scripts that the user can add to the .git/hooks/ directory to automatically enforce conventional and signed commits.
Conventional commits hook (a commit-msg hook) will not allow the user to commit if the commit message does not satisfy the conventional commits template.
Signed commits hook (a pre-push hook) will not allow the user to push to remote if the last commit is not signed.
This is a good sanity test, if the last commit is signed then the chance of the other previous are also signed.

I also added instructions to CONTRIBUTING.md.

Here's an output:

> git commit -m "test hook"
Error: Invalid commit message format. Please use a conventional commit message.
Commit message should match the pattern: ^(build|ci|docs|feat|fix|perf|refactor|style|test|chore|revert)(\([[:alnum:]\-]+\))?:[[:space:]].*.
Please refer to https://www.conventionalcommits.org/en/v1.0.0/ for more details.

> git commit --no-gpg-sign -m "ci: test hook"
[einherjar/ci-commits 205b54c] ci: test hook
 3 files changed, 53 insertions(+), 5 deletions(-)
 create mode 100755 ci/git-hooks/conventional-commits.sh
 create mode 100755 ci/git-hooks/signed-commits.sh
> git push
Error: Last commit (205b54cd7caf6b86d53699177137280e91be7c00) is not signed.
error: failed to push some refs to 'https://github.com/realeinherjar/bdk.git'

[danielabrozzoni]

@danielabrozzoni, I've added two Git Hooks to the new directory ci/git-hooks.

Thanks!

I see that the conventional commits check is failing on the merge commits (for example 59fc1b3), is there a way to configure it to allow them?

[realeinherjar]

I see that the conventional commits check is failing on the merge commits (for example 59fc1b3), is there a way to configure it to allow them?

That is only possible with a cog.toml cocogitto configuration file.
Setting ignore_merge_commits = true will make Merge ... commit messages not fail on CI.

This is documented in the cocogitto docs.
The CI cocogitto action should load the cog.toml configs.

[notmandatory]

@realeinherjar Is this PR still needed or will these checks be rolled into #1165?

[realeinherjar]

This is superseded by #1165.