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

Jump to bottom

tools: commit-queue wait 2 days automatically #45407

Merged
merged 1 commit into from Nov 12, 2022
Merged

tools: commit-queue wait 2 days automatically #45407

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.

anonrig
anonrig approved these changes Nov 10, 2022
View reviewed changes
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!

panva
panva requested changes Nov 10, 2022
View reviewed changes
.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
test nodejs/node#45407
a7d9443
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
e3f6d8a
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
592be55
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
d669bb3
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
9d5ebd9
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
06d59da
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
d7ca51b
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
45f7320
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
20159b7
MoLow added a commit to nodejs/node-auto-test that referenced this pull request Nov 10, 2022
@MoLow
test nodejs/node#45407
7dbc0ae
@MoLow
Copy link
Member Author

MoLow commented Nov 10, 2022 •
edited

@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 •
edited

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
aduh95
aduh95 reviewed Nov 10, 2022
View reviewed changes
.github/workflows/commit-queue.yml
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
anonrig
anonrig approved these changes Nov 10, 2022
View reviewed changes
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
targos
targos approved these changes Nov 11, 2022
View reviewed changes
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
@MoLow @ruyadorno
tools: do not run CQ on non-fast-tracked PRs open for less than 2 days
bb36acf 
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
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@MoLow @danielleadams
tools: do not run CQ on non-fast-tracked PRs open for less than 2 days
ac6c114 
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 danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@MoLow @danielleadams
tools: do not run CQ on non-fast-tracked PRs open for less than 2 days
ddca994 
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
@MoLow @danielleadams
tools: do not run CQ on non-fast-tracked PRs open for less than 2 days
f5a4c52 
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
@MoLow @danielleadams
tools: do not run CQ on non-fast-tracked PRs open for less than 2 days
4776518 
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
Reviewers

@panva panva panva approved these changes

@anonrig anonrig anonrig approved these changes

@targos targos targos approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support adding the commit-queue label before the wait time is over
6 participants
@MoLow @nodejs-github-bot @aduh95 @panva @anonrig @targos