build: implement a Commit Queue in Actions #34112

mmarchini · 2020-06-29T02:54:05Z

This is a (still experimental) implementation of a Commit Queue on
GitHub Actions, using labels and the scheduler event to land Pull
Requests. It uses node-core-utils to validate Pull Requests and to
prepare the commit message, and then it uses a GitHub personal token to
push changes back to the repository. If the Queue fails to land a Pull
Request, that PR will be removed from the queue and the
node-core-utils output will be pasted in the Pull Request.

An overview of the implementation is provided in
doc/guides/commit-queue.md, as well as current limitations.

Ref: https://github.com/mmarchini-oss/automated-merge-test
Ref: nodejs/build#2201

I've been testing this feature on https://github.com/mmarchini-oss/automated-merge-test and it works well for most general cases. If anyone wants to give it a try, let me know so I can add you to the repository.

Here's an example of the Action landing a Pull Request:

(mmarchini-oss/automated-merge-test#17)

And here's an example of the comment left by the Action when a PR fails to land:

(mmarchini-oss/automated-merge-test#54 (comment))

Requirements to land

Create a Jenkins token for @nodejs-github-bot
Create a GitHub personal token for @nodejs-github-bot
Add GH_USER_NAME, GH_USER_TOKEN and JENKINS_TOKEN secrets to this repository (or to the entire org, if we want this to work on forks such as quic and node-auto-test as well)
Land land: fix --abort exit status code node-core-utils#449, and wait for the next NCU release
Create commit-queue and commit-queue-failed labels in this repository

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

.github/workflows/commit-queue.yml

tools/actions/commit-queue.sh

doc/guides/commit-queue.md

tools/actions/commit-queue.sh

bnoordhuis

Thanks for taking on this thankless, unglamorous job, Matheus.

.github/workflows/commit-queue.yml

doc/guides/commit-queue.md

lundibundi

LGTM after fixing the md.

mmarchini · 2020-07-26T13:04:28Z

LGTM after fixing the md

I thought I addressed all comments yesterday. Did I miss something?

lundibundi · 2020-07-26T13:23:41Z

@mmarchini PTAL at github-actions warnings of doc/guides/commit-queue.md, or just run lint-md locally. IIRC there should be an empty line in order to make nested lists in md. Currently, all sub-elements are concatenated in one line which is visible in GitHub md preview.

mmarchini · 2020-08-07T19:22:24Z

I plan to land this late next week (probably next Friday). Want to wait a little more to see if we learn anything else from the request-ci PR. I'll also update this PR with what we learned so far (make sure this don't run on forks otherwise it'll be a flood of notifications, and make sure we have comprehensive log in case things don't work).

mmarchini · 2020-08-10T08:10:00Z

Might as well land it tomorrow since we might change how the Jenkins starter works (#34707). I still think schedule event is the appropriate one for a commit queue though, if we use pull_request_target and two actions run simultaneously the second one to finish will fail to push. We could implement a wait mechanism so that one action executes at a time, it would be a matter of choosing which complexity we want in the repo.

I'll update the docs in this PR to replace mentions of "pull_request event won't work" with "if we use pull_request_target and two actions run simultaneously the second one to finish will fail to push".

This is a (still experimental) implementation of a Commit Queue on GitHub Actions, using labels and the scheduler event to land Pull Requests. It uses `node-core-utils` to validate Pull Requests and to prepare the commit message, and then it uses a GitHub personal token to push changes back to the repository. If the Queue fails to land a Pull Request, that PR will be removed from the queue and the `node-core-utils` output will be pasted in the Pull Request. An overview of the implementation is provided in doc/guides/commit-queue.md, as well as current limitations. Ref: https://github.com/mmarchini-oss/automated-merge-test Ref: nodejs/build#2201

devsnek · 2020-08-10T20:45:53Z

Does this validate that the cq label was added by someone with commit permissions?

mmarchini · 2020-08-10T20:49:40Z

It works under the assumption that only folks with commit permission can add labels, which was true until a month or so ago when we introduced the triaging team :/

Nice catch, will update with this check.

addaleax · 2020-08-10T20:55:20Z

I actually think it would be helpful if triagers could kick off CQ, it’s just that their reviews shouldn’t count towards the PR being ready, right?

.github/workflows/commit-queue.yml

Co-authored-by: Anna Henningsen <[email protected]>

mcollina · 2020-08-10T22:53:56Z

wow!

codebytere

this is awesome, i'm really excited to see where this goes 🥳

bnb · 2020-08-11T04:29:29Z

this is super awesome @mmarchini. Incredible work, Mary.

mcollina

lgtm

richardlau

Thanks, looking forward to seeing this in action.

Comments are non-blocking.

richardlau · 2020-08-11T10:19:18Z

doc/guides/commit-queue.md

+1. The Pull Request must have only one commit
+2. A CI must've ran and succeeded since the last change on the PR
+3. A Collaborator must have approved the PR since the last change
+4. Only Jenkins CI is checked (Actions, V8 CI and CITGM are ignored)


I think Actions are checked now?

Not yet, I have a PR open for that

richardlau · 2020-08-11T10:23:49Z

.github/workflows/commit-queue.yml

+          # TODO(mmarchini): install from npm after next ncu release is out
+          npm install -g 'https://github.com/mmarchini/node-core-utils#commit-queue-branch'


Can we get a new version of node-core-utils published before landing this?

If we get a new version before I land this I'll update here, otherwise I'll follow up with a PR to update it later

mmarchini · 2020-08-11T23:55:43Z

should triagers be able to start Jenkins CI?

omg I just realized I made a huge mistake on my comment above. Triage team will be able to start Commit Queue, not CI (they already can start CI today :) ). So let me rephrase my question: should the triage team be allowed to start Commit Queue, which means they will be able to land pull requests? As @addaleax mentioned above, triage team will still not count towards reviews, and the Commit Queue runs all the necessary checks before landing (wait time, reviews, CI, etc). This might help offload some landing work from collaborators, which is good. OTOH it gives partial commit permission to a team separate from collaborators. So what do folks think?

addaleax · 2020-08-11T23:56:58Z

@mmarchini I didn’t even notice :) To be clear, I’m very much +1 on both.

This is a (still experimental) implementation of a Commit Queue on GitHub Actions, using labels and the scheduler event to land Pull Requests. It uses `node-core-utils` to validate Pull Requests and to prepare the commit message, and then it uses a GitHub personal token to push changes back to the repository. If the Queue fails to land a Pull Request, that PR will be removed from the queue and the `node-core-utils` output will be pasted in the Pull Request. An overview of the implementation is provided in doc/guides/commit-queue.md, as well as current limitations. Ref: https://github.com/mmarchini-oss/automated-merge-test Ref: nodejs/build#2201 PR-URL: #34112 Refs: https://github.com/mmarchini-oss/automated-merge-test Refs: nodejs/build#2201 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Richard Lau <[email protected]>

mmarchini · 2020-08-14T01:48:03Z

Landed in a84716a 🎉

Trott · 2020-08-14T02:25:07Z

Guess I'll find out when someone uses it, but when this action is used, who/what shows up on the Commit: line when you do something like git log --format=fuller? I'm asking because I use that as one of the metrics I collect on collaborators to see who is super active and who is not so much. (Maybe someone isn't writing much code but is landing a lot of commits and closing PRs.)

This is in no way opposition to this nor am I asking to be accommodated. Just curious what the change will look like. I am fully on Team Commit Queue.

mmarchini · 2020-08-14T04:16:17Z

For now it will show "Node.js GitHub Bot" (https://github.com/nodejs/node/pull/34112/files#diff-21f285c740fafee2a921678194aef7abR39-R40). Totally feasible to change it to the collaborator who added the label though if we decide that adding the label will count as "active metrics".

Another option is to keep committing as the bot but add Commit-Queue-By (or just Commit-Queue) metadata when landing. That's what V8 does.

This is a (still experimental) implementation of a Commit Queue on GitHub Actions, using labels and the scheduler event to land Pull Requests. It uses `node-core-utils` to validate Pull Requests and to prepare the commit message, and then it uses a GitHub personal token to push changes back to the repository. If the Queue fails to land a Pull Request, that PR will be removed from the queue and the `node-core-utils` output will be pasted in the Pull Request. An overview of the implementation is provided in doc/guides/commit-queue.md, as well as current limitations. Ref: https://github.com/mmarchini-oss/automated-merge-test Ref: nodejs/build#2201 PR-URL: #34112 Refs: https://github.com/mmarchini-oss/automated-merge-test Refs: nodejs/build#2201 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Richard Lau <[email protected]>

mmarchini added the blocked PRs that are blocked by other issues or PRs. label Jun 29, 2020

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Jun 29, 2020

devsnek reviewed Jun 29, 2020

View reviewed changes

.github/workflows/commit-queue.yml Show resolved Hide resolved

lundibundi reviewed Jun 29, 2020

View reviewed changes

This was referenced Jul 1, 2020

doc: drop minimum waiting time as hard guideline #33879

Closed

Request permission to create personal and Jenkins tokens for github-bot add Secrets to nodejs/reliability and nodejs/node nodejs/admin#523

Closed

bnoordhuis approved these changes Jul 1, 2020

View reviewed changes

mmarchini force-pushed the commit-queue branch from 89eadf4 to 4882408 Compare July 25, 2020 21:51

lundibundi approved these changes Jul 26, 2020

View reviewed changes

mmarchini mentioned this pull request Jul 27, 2020

Node.js Technical Steering Committee (TSC) Meeting 2020-07-30 nodejs/TSC#896

Closed

mmarchini force-pushed the commit-queue branch from 4882408 to e542b5d Compare July 31, 2020 00:15

mmarchini self-assigned this Jul 31, 2020

mmarchini force-pushed the commit-queue branch from e542b5d to fa9c3fc Compare July 31, 2020 22:21

mmarchini force-pushed the commit-queue branch from fa9c3fc to 09f8a36 Compare August 10, 2020 02:43

mmarchini force-pushed the commit-queue branch from 09f8a36 to d1f4de4 Compare August 10, 2020 19:48

This comment has been minimized.

Sign in to view

addaleax approved these changes Aug 10, 2020

View reviewed changes

.github/workflows/commit-queue.yml Outdated Show resolved Hide resolved

fixup! build: implement a Commit Queue in Actions

52b4a22

Co-authored-by: Anna Henningsen <[email protected]>

jasnell approved these changes Aug 10, 2020

View reviewed changes

codebytere approved these changes Aug 11, 2020

View reviewed changes

mmarchini mentioned this pull request Aug 11, 2020

Node.js Technical Steering Committee (TSC) Meeting 2020-08-13 nodejs/TSC#907

Closed

mcollina approved these changes Aug 11, 2020

View reviewed changes

richardlau approved these changes Aug 11, 2020

View reviewed changes

mmarchini removed the blocked PRs that are blocked by other issues or PRs. label Aug 14, 2020

mmarchini closed this Aug 14, 2020

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

This was referenced Jun 14, 2021

Restrict cq permissions nodejs/node-auto-test#33

Closed

Which teams should be allowed to land pull requests on nodejs/node? nodejs/TSC#1039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: implement a Commit Queue in Actions #34112

build: implement a Commit Queue in Actions #34112

mmarchini commented Jun 29, 2020 •

edited

Loading

bnoordhuis left a comment

lundibundi left a comment

mmarchini commented Jul 26, 2020

lundibundi commented Jul 26, 2020

mmarchini commented Aug 7, 2020

mmarchini commented Aug 10, 2020

devsnek commented Aug 10, 2020

mmarchini commented Aug 10, 2020

addaleax commented Aug 10, 2020

This comment has been minimized.

mcollina commented Aug 10, 2020

codebytere left a comment

bnb commented Aug 11, 2020

mcollina left a comment

richardlau left a comment

richardlau Aug 11, 2020

mmarchini Aug 11, 2020

richardlau Aug 11, 2020

mmarchini Aug 11, 2020

mmarchini commented Aug 11, 2020

addaleax commented Aug 11, 2020

mmarchini commented Aug 14, 2020

Trott commented Aug 14, 2020

mmarchini commented Aug 14, 2020

		# TODO(mmarchini): install from npm after next ncu release is out
		npm install -g 'https://github.com/mmarchini/node-core-utils#commit-queue-branch'

build: implement a Commit Queue in Actions #34112

build: implement a Commit Queue in Actions #34112

Conversation

mmarchini commented Jun 29, 2020 • edited Loading

Requirements to land

Checklist

bnoordhuis left a comment

Choose a reason for hiding this comment

lundibundi left a comment

Choose a reason for hiding this comment

mmarchini commented Jul 26, 2020

lundibundi commented Jul 26, 2020

mmarchini commented Aug 7, 2020

mmarchini commented Aug 10, 2020

devsnek commented Aug 10, 2020

mmarchini commented Aug 10, 2020

addaleax commented Aug 10, 2020

This comment has been minimized.

mcollina commented Aug 10, 2020

codebytere left a comment

Choose a reason for hiding this comment

bnb commented Aug 11, 2020

mcollina left a comment

Choose a reason for hiding this comment

richardlau left a comment

Choose a reason for hiding this comment

richardlau Aug 11, 2020

Choose a reason for hiding this comment

mmarchini Aug 11, 2020

Choose a reason for hiding this comment

richardlau Aug 11, 2020

Choose a reason for hiding this comment

mmarchini Aug 11, 2020

Choose a reason for hiding this comment

mmarchini commented Aug 11, 2020

addaleax commented Aug 11, 2020

mmarchini commented Aug 14, 2020

Trott commented Aug 14, 2020

mmarchini commented Aug 14, 2020

mmarchini commented Jun 29, 2020 •

edited

Loading