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

Support adding the commit-queue label before the wait time is over #45405

Closed
targos opened this issue Nov 10, 2022 · 5 comments · Fixed by #45407
Closed

Support adding the commit-queue label before the wait time is over #45405

targos opened this issue Nov 10, 2022 · 5 comments · Fixed by #45407
Labels
meta Issues and PRs related to the general management of the project.

Comments

@targos
Copy link
Member

targos commented Nov 10, 2022

Maybe this needs to be implemented mostly in node-core-utils, but I'd like to open the discussion here first for visibility.

I think we should be able to add the commit-queue label to a PR when the only missing requirement for it to land is the wait time. It is always annoying to have to calculate the remaining time and then remember to come back to a pull request just to add this label.

Is it desirable and implementable?

@nodejs/actions

@targos targos added the meta Issues and PRs related to the general management of the project. label Nov 10, 2022
@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2022

Having the commit-queue Add this label to land a pull request using GitHub Actions. label added before a PR is actually ready to land forces the scheduled GHA to do a lot more of work (it now has to fetch the full commit history, install ncu, etc.) which can delay the launch of regular CI runs on other PRs if we've reached the max number of active runners. For example, it's already possible to add the label while the CI is still running, but I've recommended against doing that because of that (and also we never know if the CI will be successful or not).

If there was a way of doing that without the need of fetching the entire commit history, that could be interesting. Otherwise, there's always the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label.

@MoLow
Copy link
Member

MoLow commented Nov 10, 2022

why do we need the full commit history?

@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2022

# Needs the whole git history for ncu to work
# See https://github.com/nodejs/node-core-utils/pull/486
fetch-depth: 0

@MoLow
Copy link
Member

MoLow commented Nov 10, 2022

@aduh95 my PR filters pr prior to this checkout step

nodejs-github-bot pushed a commit that referenced this issue Nov 12, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
ruyadorno pushed a commit that referenced this issue Nov 21, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@Jimimaku

This comment was marked as off-topic.

danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
PR-URL: #45407
Fixes: #45405
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
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 a pull request may close this issue.

4 participants