-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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.
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
tools: avoid fetch extra commits when validating commit messages #39128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (if something goes wrong we can always revert)
Actually, can you force two commits here to demonstrate that it works? |
e661bcb
to
fc2dd1c
Compare
Which merge commit? |
The commit the merges the PR branch into the target branch. I tried to find reference for that in GH Actions docs but couldn't find it. |
What kind of message does that commit have? Do we skip its validation? |
Btw, I don't understand how it can work on a push. |
This workflow only runs on the pull_request event. |
Okay, sorry, the beginning of the file was hidden. In this case I wonder if the plus one is necessary. Because if there is a merge commit, it would not be in the pull request |
It is necessary otherwise we dont validate the commit message of the first commit.
Mary explains it better than me: #39121 (comment) |
Commit Queue failed- Loading data for nodejs/node/pull/39128 ✔ Done loading data for nodejs/node/pull/39128 ----------------------------------- PR info ------------------------------------ Title tools: avoid fetch extra commits when validating commit messages (#39128) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:commit-msg-linter -> nodejs:master Labels meta Commits 2 - tools: avoid fetch extra commits when validating commit messages - fixup! tools: avoid fetch extra commits when validating commit messages Committers 1 - Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/39128 Reviewed-By: Mary Marchini ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/39128 Reviewed-By: Mary Marchini -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - tools: avoid fetch extra commits when validating commit messages ⚠ - fixup! tools: avoid fetch extra commits when validating commit messages ℹ This PR was created on Wed, 23 Jun 2021 14:59:58 GMT ✔ Approvals: 1 ✔ - Mary Marchini (@mmarchini) (TSC): https://github.com/nodejs/node/pull/39128#pullrequestreview-769672595 ✔ Last GitHub Actions successful ℹ Green GitHub Actions CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1402622863 |
Landed in b5aa08d |
PR-URL: nodejs/node#39128 Reviewed-By: Mary Marchini <oss@mmarchini.me>
PR-URL: #39128 Reviewed-By: Mary Marchini <oss@mmarchini.me>
PR-URL: #39128 Reviewed-By: Mary Marchini <oss@mmarchini.me>
We only need the number of commits in the PR, plus one for the merge commit.
I couldn't figure out a way to compute the
+ 1
operation without an extra step, if someone knows a cleaner way please chime in.