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.

Sign up for GitHub

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

doc: add section stating that very stale PRs should be closed #57541

Merged
merged 5 commits into from
Mar 25, 2025
Merged

doc: add section stating that very stale PRs should be closed #57541

merged 5 commits into from
Mar 25, 2025

Conversation

dario-piotrowicz
Copy link
Contributor

I just wanted to open this PR to see if there could be some consensus on closing PRs that
have been state for a significant amount of time (I'm proposing 6 months here, this is very
arbitrary and can definitely be changed/generalized).

The fact is that the number of current PRs in the project, as I am opening this one is 492,
although that's not very problematic it feels rather messy to me (without providing any
noticeable value I think?), having a clear policy on when a PR is so stale that it should probably
be closed could be beneficial in trying to clean outdated and stale PRs without the risk to
generate too much controversy (since right now, as far as I can tell, abandoned PRs just stay
there indefinitely and there isn't a clear procedure on what to do with them).

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 19, 2025
@anonrig anonrig requested review from jasnell, Trott and aduh95 March 19, 2025 00:15
anonrig
anonrig approved these changes Mar 19, 2025
View reviewed changes
@Trott
Copy link
Member

Trott commented Mar 19, 2025

Do we not already have a GitHub Action that closes stalled pull requests automatically? https://github.com/nodejs/node/blob/fe5817e06ca81f0388d448287e60e2ca49c3cba3/.github/workflows/close-stalled.yml (Maybe it's not functioning as expected?)

@Trott
Copy link
Member

Trott commented Mar 19, 2025 •
edited
Loading

Here's a PR from November that was closed by the GitHub Action. #51611 Maybe the Action needs to be adjusted to be less cautious, but I don't think we should add documentation instructing people to do this manually.

@Trott
Copy link
Member

Trott commented Mar 19, 2025

Oh, I see, the GitHub Action requires people to manually add the stalled label. This should probably instruct people to do that rather than having two different official-ish processes. (Or, if we're going to prefer manual closing, than we should probably disable the automation. I prefer the automation myself, but either way is fine. But let's keep it to just one way.)

@dario-piotrowicz
Copy link
Contributor Author

@Trott Thanks for the comment, I wasn't aware of the automation for the stalled label, sounds great, and since it runs every day yes I don't really see any benefit in the manual closing then 🙂👍

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 24, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 24, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/57541
✔  Done loading data for nodejs/node/pull/57541
----------------------------------- PR info ------------------------------------
Title      doc: add section stating that very stale PRs should be closed (#57541)
Author     Dario Piotrowicz <[email protected]> (@dario-piotrowicz)
Branch     dario-piotrowicz:dario/contributing/stale-prs -> nodejs:main
Labels     doc
Commits    3
 - doc: add section stating that very stale PRs should be closed
 - Update doc/contributing/pull-requests.md
 - Update doc/contributing/pull-requests.md
Committers 2
 - Dario Piotrowicz <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/57541
Reviewed-By: Yagiz Nizipli <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57541
Reviewed-By: Yagiz Nizipli <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 19 Mar 2025 00:12:09 GMT
   ✔  Approvals: 1
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/57541#pullrequestreview-2711584587
   ✘  This PR needs to wait 27 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/14045059860

@Trott Trott added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 25, 2025
@Trott
Copy link
Member

Trott commented Mar 25, 2025

I added two trivial edits before approving, so it would be good if someone else could approve (or re-approve) before landing. Otherwise, I think this is ready to merge.

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2025
@nodejs-github-bot nodejs-github-bot merged commit b4280ef into nodejs:main Mar 25, 2025
22 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b4280ef

aduh95 pushed a commit that referenced this pull request Mar 25, 2025
@dario-piotrowicz @aduh95
doc: add section stating that very stale PRs should be closed
ee1c78a 
PR-URL: #57541
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@dario-piotrowicz dario-piotrowicz deleted the dario/contributing/stale-prs branch March 25, 2025 18:46
@bjohansebas
Copy link
Member

In principle, the bot is not working, see #54425. In the meantime, closing the issue/pr would be a manual step.

@dario-piotrowicz
Copy link
Contributor Author

In principle, the bot is not working, see #54425. In the meantime, closing the issue/pr would be a manual step.

It seems like that issue got solved though right? 🙂

pandeykushagra51 pushed a commit to pandeykushagra51/node that referenced this pull request Mar 28, 2025
@dario-piotrowicz @pandeykushagra51
doc: add section stating that very stale PRs should be closed
7a30439 
PR-URL: nodejs#57541
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

@anonrig anonrig anonrig approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dario-piotrowicz @nodejs-github-bot @Trott @bjohansebas @anonrig @aduh95