Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: use Actions to validate commit message #32417

Closed
wants to merge 1 commit into from
Closed

build: use Actions to validate commit message #32417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/commit-lint-problem-matcher.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"problemMatcher": [
{
"owner": "core-validate-commit",
"pattern": [
{
"regexp": "^not ok \\d+ (.*)$",
"message": 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not entirely clear on whether it works this way or whether the JSON values all have to refer to regexp capture groups, but could we use something like "severity": "warning" (or "severity": "notice"? not sure if that would work either) so that this does not appear like a failed build, given that this is ultimately not essential for PR authors to address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The severity only works for annotations, the Check status can only be Success or Failure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s unfortunate :/ Any ideas for how else we could make clear that this is non-essential? I’d rather not have explicit failures because of this…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could prefix the workflow name with "[not a blocker]" or hijack the annotation to have this as a message. Unfortunately I don't think there's anything we can do using GitHub's features (the best thing would be for the Action leave a comment, but we can't do that with Actions for PRs coming from forks), and I would rather not have the check on CI than mark as "allowed to fail", because if we do so it'll be misleading for newcomers (the check will always be green).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to warning instead of error on the annotation is probably still worth though. I'll update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, reading the Checks API docs again it seems there a "neutral" status.

https://developer.github.com/v3/checks/runs/

I never seen it, and I'm not sure we can set it without directly calling the API (which is not possible in the Action), but it's worth a shot.

(There's also a "Action Required" status, but it's visually worse than "Failure" IMO)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Actions has no way of setting the "neutral" status on runs without calling the Checks API directly (which needs a write-permission token, which means it wouldn't work on Pull Requests from non-collaborators). So, we can either:

  1. Have the check fail
  2. Set it to continue-on-error
  3. Don't check the commit message on CI

I'd rather not set it to continue-on-error since any failures will fly under everyone's radar. I'm fine not landing this change though, if folks think failing the CI because the commit messages don't follow our guidelines is unwelcoming. Personally I think the annotations are helpful for folks who want to make sure their commits are following the guidelines. Maybe we could force one annotation with a "not required" message, but I'm not sure how useful and clear that would be.

On the other hand we could also use this check to verify Signed-off-by metadata on commits once #31976 lands, or have another Action doing the same verification. Either way the CI would fail if the commit is not signed, so maybe it's a good thing to introduce this checks now instead of later.

One alternative I've seen in the past is keeping the check "pending" when things like Signed-off-by are missing, but then again, I don't think this is possible with the current Actions permission model.

}
]
}
]
}
21 changes: 21 additions & 0 deletions .github/workflows/commit-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: "Commit messages adheres to guidelines at https://goo.gl/p2fr5Q"

on: [pull_request]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
env:
NODE_VERSION: 14.x

jobs:
lint-commit-message:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
# Last 100 commits should be enough for a PR
fetch-depth: 100
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if this syntax is valid YAML, but you can (and should IMO) use the actual number of commits.

Suggested change
# Last 100 commits should be enough for a PR
fetch-depth: 100
fetch-depth: ${{ github.event.pull_request.commits }} + 1

- name: Use Node.js 12
uses: actions/setup-node@v1
with:
node-version: 12.x
Comment on lines +14 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Use Node.js 12
uses: actions/setup-node@v1
with:
node-version: 12.x
- name: Use Node.js ${{ env.NODE_VERSION }}
uses: actions/setup-node@v2
with:
node-version: ${{ env.NODE_VERSION }}

- name: Validate commit messages
run: |
echo "::add-matcher::.github/workflows/commit-lint-problem-matcher.json"
git log --oneline ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }} | grep -v -e fixup -e squash | awk '{ print $1 }' | xargs npx -q core-validate-commit --no-validate-metadata --tap