interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

ci: add commit checks for PRs #734

Closed Mirko-von-Leipzig closed 3 years ago

Mirko-von-Leipzig commented 3 years ago

This PR introduces a new github workflow which ensures all commits in a PR conform to our requirements. It checks for the following:

  1. conventional commit messages
  2. used git commit --signoff
  3. no extra merge commits

Conventional commits and --signoff

The first two points are taken care of by the NPM commitlint package. Unfortunately, they provide guides for CircleCI and TravisCI but not for Github Actions (open issue). I managed to get it working after some trial and error.

There is a 3rd party Github Action which looks great, but unfortunately I ran into this bug. It also seemed like a lot of code to audit, considering we don't need much.

Another alternative could be to use git interpret-trailers to check for --signoff, but since this came bundled with commitlint I didn't explore further.

no extra merge commits

This gets checked with some simple bash code walking the PR commits using git log --merges --ancestry-path. Luckily the Github PR event contains the HEAD and number of commits in the PR.

Why a new workflow file?

It has different run on requirements and therefore requires its own file (I think?).

It made sense to me that it should only run on actual PRs. It would otherwise trigger (and probably fail) when wanting to test things quickly.

I also enabled it for PRs into any branch (and not just into master), since its likely the code would eventually make its way into a PR for master, but by that time it may be too late to get the commit requirements fixed.

cargo audit

I've also added in the ignore for the two hyper RUSTSECs.

koivunej commented 3 years ago

Could you add the missing newline in a non-signed off commit, possibly being result of something you merged into this PR branch, later rebase it?

Mirko-von-Leipzig commented 3 years ago

Could you add the missing newline in a non-signed off commit, possibly being result of something you merged into this PR branch, later rebase it?

I'll add a dummy commit πŸ‘ -- done. Should fail CI now (I hope)

koivunej commented 3 years ago

Oki could you now add a merge commit as well?

Mirko-von-Leipzig commented 3 years ago

Oki could you now add a merge commit as well?

mmm. What I did broke something here. It seems the commitlint check is walking the wrong commits now. So the wrong step fails.

--- update --- @koivunej

Turns out HEAD~N doesn't work once there's a merge commit in the mix. Which makes sense. Luckily GH provides the base branches HEAD commit as well. So now the commit range is calculated (and shared between steps using env variables) using:

echo "PR_HEAD=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV
echo "PR_BASE=${{ github.event.pull_request.base.sha }}" >> $GITHUB_ENV

Since the code has changed I'll start the "tests" from the start again.

non-signed commit

β§—   input: feat: unsigned
βœ–   message must be signed off [signed-off-by]

with merge commit

PR contains merge commits:
commit f3489557b0808e1ee5b078f10dfa9afa10c8a2e3 Merge: 8da2d21 dec3383 Author: Mirko von Leipzig <mirko.vonleipzig@gmail.com> Date: Wed Aug 11 11:10:55 2021 +0200 Merge branch 'merge_branch' into gh_action_checks

non-conventional-commit (was also unsigned)

β§—   input: unconventional
βœ–   subject may not be empty [subject-empty]
βœ–   type may not be empty [type-empty]
βœ–   message must be signed off [signed-off-by]
koivunej commented 3 years ago

Thank you @Mirko-von-Leipzig very much!