Skip to content

tools: add workflow to add label when there are 2+ approvals#64340

Open
aduh95 wants to merge 1 commit into
nodejs:mainfrom
aduh95:approval-label
Open

tools: add workflow to add label when there are 2+ approvals#64340
aduh95 wants to merge 1 commit into
nodejs:mainfrom
aduh95:approval-label

Conversation

@aduh95

@aduh95 aduh95 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

And make the CQ use it to support adding the label on PRs with a single approval which are not 7 days old yet.

I've tested it with nodejs/node-auto-test#45, seems to be working fine. Here are a few caveats:

  • It counts the approvals of the head of the PR, meaning that if the PR has many approval, but a commit gets pushed and it receives a single approval, the CQ will wait for the 7 days (even though the PR could technically land sooner according to our governance rules).
  • the pr_number.txt is user-mutable (if someone opens a PR, they could change the workflow to pass arbitrary data to the file, which would then be consumed by the other workflow if it receives any review (which doesn't have to be positive nor from someone with Write access)), unless I've missed something the worst that could happen is that the label could be added/removed on the wrong PR
  • I'm not using the latestReview JSON field of gh pr view as I find it unreliable at communicating if it's limited to "non-stale" reviews (i.e. did the PR head changed since that review)

Alternative to #64262 and #62498

And make the CQ use it to support adding the label on PRs with a single
approval which are not 7 days old yet.

Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jul 7, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions

@panva

panva commented Jul 7, 2026

Copy link
Copy Markdown
Member

It counts the approvals of the head of the PR, meaning that if the PR has many approval, but a commit gets pushed and it receives a single approval, the CQ will wait for the 7 days (even though the PR could technically land sooner according to our governance rules).

Can we aleviate this drawback? Often times a PR that's ready to land in the 2-7 period would get an approval with a nit, or just requests a small change before approving, you want to fix that nit but now can't land it with CQ even if the one who requested a small change re-approves because now it needs one more existing reviewer to either come back and approve or someone else entirely.

#62498 OTOH just allowed to add the CQ label with one approval and it'd stay in the queue until a 2nd review comes or 7 days pass. No brain teasers.

@aduh95

aduh95 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@panva you can add the Approvals: 2+ PRs that have anough approvals to land before the 7 days mark label manually
Note that there’s no reliable way for the bot to detect a small change, so IMO it makes sense to defer to a more defensive behavior

@panva

panva commented Jul 7, 2026

Copy link
Copy Markdown
Member

@panva you can add the label manually

Which label would that be? Can you explain this automation proposal here better?

@aduh95

aduh95 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

The automation runs every time a review is submitted or dismissed. It will count how many approvals from the team (TSC for semver-major PRs, collaborators for other PRs) on the HEAD (i.e. since the last non-empty commit), if that's more that 2 and there are no changes requested, it adds Approvals: 2+ PRs that have anough approvals to land before the 7 days mark , otherwise it removes it.
What I'm saying is that for the case you present in #64340 (comment), it would IMO makes sense for a human to manually add the label, if it's indeed just a nit it doesn't invalidate the previous reviews.

Can we aleviate this drawback?

To answer this specifically, the answer is: definitely yes, I had to add some logic to check if the review was on the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meta Issues and PRs related to the general management of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants