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.

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: commit-queue wait 2 days automatically #45407

Merged
merged 1 commit into from
Nov 12, 2022

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Nov 10, 2022

Fixes: #45405

not sure how we can test this

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 10, 2022
@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

CC @targos

@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2022

This would force us to land fast-track PRs that do not need to wait for 48 hours to land. PRs manually, I'm not sure it's desirable.

@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2022

not sure how we can test this

You can use https://github.com/nodejs/node-auto-test for this kind of things.

@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

This would force us to land fast-track PRs that do not need to wait for 48 hours to land. PRs manually, I'm not sure it's desirable.

I guess we can merge the result of two queries

@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

@aduh95 added support for fast-track PRs that do not need to wait for 48 hours to land.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

LGTM. I loved the idea!

.github/workflows/commit-queue.yml Outdated Show resolved Hide resolved
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

@panva @aduh95 tested:
current result of https://github.com/nodejs/node-auto-test/issues?q=is%3Aopen+label%3Acommit-queue+sort%3Aupdated-desc is:

image

the commit queue resulted in attempting to commit PRs 31 (commit-queue & < 2 days) and 50 (commit-queue & fast track):
https://github.com/nodejs/node-auto-test/actions/runs/3439433893/jobs/5736726139

@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

amazing @panva :) too bad I cannot use this on this PR yet :/

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2022
@panva
Copy link
Member

panva commented Nov 10, 2022

If we could query for number of approvals we could also add minimum 2 to the 2 day query and add a third query for minimum 1 and 7 days ;)

@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

If we could query for number of approvals we could also add minimum 2 to the 2 day query and add a third query for minimum 1 and 7 days ;)

ncu already does that

@panva
Copy link
Member

panva commented Nov 10, 2022

If we could query for number of approvals we could also add minimum 2 to the 2 day query and add a third query for minimum 1 and 7 days ;)

ncu already does that

by then the label is removed and ncu will fail the commit queue.

@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022

Yeah 😐

@panva
Copy link
Member

panva commented Nov 10, 2022

But it's mostly crypto and cpp PRs that don't manage to get at least two approvals in 2 days.

This is still a great feature. Add CQ label as soon as you have two or enough fast track thumbs. Then stop worrying about the PR.

@panva panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 10, 2022
Comment on lines +52 to 53
numbers=$(echo $prs' '$fast_track_prs | jq -r -s 'unique | join(" ")')
echo "numbers=$numbers" >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
numbers=$(echo $prs' '$fast_track_prs | jq -r -s 'unique | join(" ")')
echo "numbers=$numbers" >> $GITHUB_OUTPUT
echo "numbers=$(echo $prs' '$fast_track_prs | jq -r -s 'unique | join(" ")')" >> $GITHUB_OUTPUT

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to minimize changes after I completed testing

@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2022

Note: the commit message should start with a verb after the subsystem (and the subsystem should probably be tools: instead of meta:), e.g. tools: do not run CQ on non-fast-tracked PRs open for less than 2 days.

@MoLow MoLow changed the title meta: commit-queue wait 2 days automatically tools: commit-queue wait 2 days automatically Nov 10, 2022
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

lgtm

@MoLow MoLow added tools Issues and PRs related to the tools directory. and removed meta Issues and PRs related to the general management of the project. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 10, 2022
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Thank you!

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2022
@nodejs-github-bot nodejs-github-bot merged commit 119559a into nodejs:main Nov 12, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 119559a

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MoLow MoLow deleted the commit-queue-auto-wait branch April 2, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding the commit-queue label before the wait time is over
6 participants