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

meta: merge stalebot and update timings/messages #52712

Closed
wants to merge 11 commits into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Apr 26, 2024

This PR will consolidate the feature-request and pull request stalebots into a unified workflow that targets all issues and all pull requests.

Focusing solely on feature-request issues seems unnecessary, as other issues can also become stale.

Instead of relying on @mhdawson's fork, which has been great, this PR will update the stale-bot to the latest version (v9.0.0).

However, there's a trade-off. Without the fork, the stalebot loses the endDate controlling environment variable. To address this, I've set the stale timeout to a middle ground between the endDate and the stale timeout (8 months).

Previously, the check was for issues older than a year with no updates in the past 6 months. Now, it will check for issues without any updates in 8 months. The middle ground would technically be 9 months, but I think 8 months strikes a good balance in this case.

@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 Apr 26, 2024
@RedYetiDev RedYetiDev added the discuss Issues opened for discussions and feedbacks. label Apr 26, 2024
RedYetiDev and others added 2 commits April 27, 2024 12:41
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@mhdawson
Copy link
Member

I'm have to refresh my memory in terms of how the endDate helpd us meet our requirements. I think it was to meet what we had agreed in terms of handling what to consider stale.

I think https://github.com/nodejs/node/blob/main/doc/contributing/feature-request-management.md will need an update.

@targos
Copy link
Member

targos commented Apr 29, 2024

We should also be careful about the impact. We don't want everyone to suddenly have 500 new notifications.

@mhdawson
Copy link
Member

+1 to what @targos said, I was very careful to avoid that when I introduced it in the first place.

@RedYetiDev
Copy link
Member Author

We should also be careful about the impact. We don't want everyone to suddenly have 500 new notifications.

Yes, that is a lot off issues :-/, WDYT should be done?
https://github.com/nodejs/node/issues?q=is%3Aopen+sort%3Aupdated-desc+updated%3A%3C%3D2023-08-29+-label%3Astale+-label%3Anever-stale+

@mhdawson
Copy link
Member

mhdawson commented May 1, 2024

That in part is why I added the end date. If I remember correctly I very carefully moved that a bit at a time so that we would not get a flood of notifications.

@RedYetiDev
Copy link
Member Author

That in part is why I added the end date. If I remember correctly I very carefully moved that a bit at a time so that we would not get a flood of notifications.

Still, if we look at issues older than a year, it's a large portion of issues, and no matter the end date, floods may still occur.

@RedYetiDev
Copy link
Member Author

After looking at the stalebot docs, there is a parameter https://github.com/actions/stale?tab=readme-ov-file#start-date, so we could only apply this change to newer issues, to prevent the flood of notifications

@RedYetiDev
Copy link
Member Author

RedYetiDev commented May 7, 2024

Also, in v9.0.0, the stale API now uses the Actions Cache, which will decrease the time it takes to run and the amount of API operations it performs.

Based on the issue cache from the help repo's first run of the new stalebot, I estimate that ~209 to ~409 API operations will be spared.

@RedYetiDev RedYetiDev requested a review from mhdawson July 1, 2024 23:10
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 1, 2024

Bumping, as I believe the stale issues in the repository are far more than they could be.

We should also be careful about the impact. We don't want everyone to suddenly have 500 new notifications.

True, but a few days of heavy notifications in exchange for less stale issues might be reasonable.

(Sorry about the review request, didn't mean to do that 😬)

@RedYetiDev RedYetiDev removed the request for review from mhdawson July 1, 2024 23:11
@RedYetiDev RedYetiDev added review wanted PRs that need reviews. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 1, 2024
@RedYetiDev RedYetiDev removed discuss Issues opened for discussions and feedbacks. review wanted PRs that need reviews. labels Jul 11, 2024
@RedYetiDev RedYetiDev requested a review from targos July 11, 2024 19:05
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 11, 2024

(requesting review from previous commenters and reviewers)

@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label Jul 20, 2024
@RedYetiDev
Copy link
Member Author

Hey, this PR hasn't seen any reviews in months, and it has no objections. Given the gravity of this change, and the lack of reviews, would it be okay if the TSC took a look? I don't want to cause them noise via a ping, so I'd like to ask permission first.

@mhdawson
Copy link
Member

mhdawson commented Aug 9, 2024

@RedYetiDev last time when I worked on this, I very carefully managed the issues that were updated to avoid the flood in order to address the concern about a flood of issues.

I think if you had an answer to that people might be more willing to move forward.

If not then you can ask the TSC to override objections on that front but you would need a specific question and then could add the tsc-agenda tag. My suggestion would be to try to come up with something to avoid the flood, but its up to you how you proceed.

@RedYetiDev
Copy link
Member Author

@mhdawson i don't want to "override" objections, your insight is very important.

I've set the operation count to 50, meaning at most, only around 30 issues will be staled a day, does that work?

@mhdawson
Copy link
Member

@RedYetiDev I don't think the operations do what you want to limit the number that will be closed. Otherwise I think I would have used that originally.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Aug 20, 2024

I don't think the operations do what you want to limit the number that will be closed. Otherwise I think I would have used that originally.

It's not what they are intended for, but they do (sorta) work like that. If you look at the nodejs/help repo, the operation count used to be 30, and when it was upped to 500, many more issues were closed.

You can also have it only apply to newer issues (via a different flag).

@mhdawson
Copy link
Member

@RedYetiDev have you tested it on your own repo. I'm pretty sure I looked at that in the original pass and it was not a viable option for limiting the issues closed.

@RedYetiDev
Copy link
Member Author

I'm pretty sure I looked at that in the original pass and it was not a viable option for limiting the issues closed.

I did some testing. it does limit the issues being closed, but its inconsistent, meaning it (with a 50 operations per run) will close ~30-50 issues a day, depending on what's in the cache from the previous day.

(Sorry for the delay in my response, it took multiple days to test)

@RedYetiDev RedYetiDev closed this Aug 31, 2024
@RedYetiDev
Copy link
Member Author

I have a separate idea to achieve this while also limiting the impact of this potential change. Once I do testing and verify it'll work non-disruptively, I'll open a new PR.

@RedYetiDev RedYetiDev mentioned this pull request Aug 31, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants