GeezerVison is showing github.com — Cron job to close stale PRs · Issue #93 · python/core-workflow
Cron job to close stale PRs #93DescriptionI'm classifying "stale" as someone who created a PR but has not signed the CLA or someone who received a code review requesting changes and and never followed up with more commits or review comments. These two scenarios specifically make sure that the PR submitter is the hold up and not any core dev(s). (A PR that can't be merged probably shouldn't be classified as such since they shouldn't have to perpetually maintain a PR if they don't get a review from a core dev).
It would probably be nice to receive a warning that a PR will be closed within a week, but re-opening a PR I don't think is hard either so it should be a big deal to just close either if it's too much work to give the warning (that would need to be verified, though, that a PR submitter and re-open their own PR before making a decision about this). Activitymatrixise commented on May 24, 2017 +1. on today, there are 17 open PR where the CLA is not signed. souravsingh commented on Oct 4, 2017 @Mariatta I am interested in working on this. Mariatta commented on Oct 4, 2017 @souravsingh Sure! I think this is not a straight forward task, and it has some fun challenges :)
My initial thoughts about this:
A PR is considered "stale" if:
- the last activity on it has a timestamp older than X days (1 month? 2 months? 3 months?)
and
- it has
CLA not signed label, or
- it has
awaiting changes label
The workflow:
- bedevere applies the
stale label to PRs with above criteria
- bedevere leaves a comment saying that this PR will be closed unless changes are made.
- if a PR has the
stale label and there has been no change 7 days after the label has been applied, then bedevere will just close the PR. Not sure how to detect this. Perhaps by retrieving all the comments from the PR, and see the timestamp when the comment from step 2 was made?
Side task, I think it can be a separate task. It will be a webhook instead of a cron.
- Remove the
stale label when awaiting changes or CLA not signed label has been removed.
I think the code can be added to bedevere, so we don't need to create any new bot. brettcannon commented on Oct 5, 2017 My one worry with adding this to Bedevere is GitHub quota. Going through 453 issues to find out details about them is going to eat through quota rather quickly and might be too much with Bedevere's current quota usage. Mariatta commented on Oct 5, 2017 Hm, the rate limit is something I haven't considered.
If we were to create a new app/bot for this, will it also be at risk of hitting the rate limit? brettcannon commented on Oct 5, 2017 Quite possibly. We might not know until we do the queries and see what the count comes out to. This might require smearing the work across the day, e.g. start at midnight, find out how many pages of issues there are, and then do a page per hour (where hopefully there are less than 24 pages 😉 ). Mariatta commented on Oct 6, 2017 brettcannon commented on Oct 6, 2017 Nice catch on the GET arguments! So it looks like 40 issues in total ATM are either labeled "CLA not signed" or "awaiting changes". But the problem is that out of 19 pages worth of issues, only the first 5 have any "awaiting" label really applied to them. So that means the 17 issues that are awaiting changes could be extrapolated to be about 68 (although possibly more since those PRs are probably/hopefully languishing for a reason), which puts it at 90 issues or more (aside: maybe we should write a script that we run once that goes through and does a back-fill of all issues without an "awaiting" label? We could have a custom message when we set "awaiting changes" saying "we're totally guessing here, so please say 'I didn't expect the Spanish Inquisition' if we're wrong" or something).
So let's worst-case it to 200 issues. If a PR is now considered stale/inactive, then there will be a comment and an added label (so 2 requests). If a PR has passed it's one week grace period, then it will be a comment and closure (2 requests). Add in pagination on the issue list and we're looking at worst-case 400+ requests out of our (I believe) 5000 request quota, so we're approaching 10%.
I have opened python/bedevere#64 so that we can see what kind of margin we currently have so we don't have to keep guessing. chetankm-cs commented on Oct 8, 2017 Hi,
I am working on this, I have added the code for labelling stale pr.
I need help on setting a cron for this. brettcannon commented on Oct 8, 2017 @chetankm-cs it will be however you set up a cron job on Heroku. hugovk commented on Jan 10, 2022 The two test PRs:
First cron run:
Both steps ran and processed the two PRs. No actions taken because it's less than 1 day since there was activity. Will check back tomorrow! hugovk commented on Jan 10, 2022 Test restarted:
We updated the PR to avoid double negatives ("has 'CLA signed'?" instead of "does not have 'CLA not signed'?" python/cpython#30500 (comment)) and I've updated the test repo and PRs to match, reseting the inactivity counters. hugovk commented on Jan 11, 2022 First run with new test:
Looks good so far, as last time:
Both steps ran and processed the two PRs. No actions taken because it's less than 1 day since there was activity. Will check back tomorrow!
erlend-aasland commented on Jan 13, 2022 @ambv:
Question: if there is activity on the PR after the label is applied, or the label is subsequently removed, will the PR still be closed? If not, the current suggested message is misleading.
No, it will not be closed. See python/cpython#30500 (comment) hugovk commented on Jan 15, 2022
The two test PRs:
First cron run:
Both steps ran and processed the two PRs. No actions taken because it's less than 1 day since there was activity. Will check back tomorrow!
Test complete, both worked as expected! 👍 erlend-aasland commented on Feb 17, 2022 Reacted by Hugo van Kemenade hugovk commented on Feb 17, 2022 Reacted by Erlend E. Aasland hugovk commented on Feb 18, 2022 The new cron ran and we now have 15 stale+CLA not signed open:
https://github.com/python/cpython/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Astale+label%3A%22CLA+not+signed%22
I expect, if no other activity before the cron:
- the top one to have
stale removed (it's had activity today): #29918
✅
- at least 17 to be closed (all last updated more than
days-before-stale: 30 + days-before-close: 14 days ago):
Only 4 were closed:
#29264 #29084 #28904 #28110
13 still open:
#22696 #27848 #27552 #27341 #25744 #26930 #26458 #24556 #24942 #24474 #23998 #22695 #22671
- The other four still open or possibly closed depending on when the
stale label was actually applied: #27256 #29865 #28987 #29900
1 closed, 1 merged 🎉, 2 still open.
So why didn't we close all the expected ones? Because the cron job action doesn't run on all the PRs.
The last run: https://github.com/python/cpython/actions/runs/1861772715
From "Check PRs with 'CLA not signed' label":
Warning: No more operations left! Exiting...
Warning: If you think that not enough issues were processed you could try to increase the quantity related to the operations-per-run ([https://github.com/actions/stale#operations-per-run)](https://github.com/actions/stale#operations-per-run%E2%80%8B)) option which is currently set to 30
Statistics:
Processed PRs: 383
New stale PRs: 1
Closed PRs: 6
Added PRs labels: 1
Added PRs comments: 7
Fetched items: 400
Fetched items events: 7
Fetched items comments: 7
Operations performed: 32
That warning:
Warning: If you think that not enough issues were processed you could try to increase the quantity related to the operations-per-run (https://github.com/actions/stale#operations-per-run)) option which is currently set to 30
https://github.com/actions/stale#operations-per-run explains how the action is rate limited, and we can increase it but "if reached will block these API calls for one hour (or API calls from other actions using the same user (a.k.a.: the github-token from the repo-token option)".
The limit:
When using GITHUB_TOKEN, the rate limit is 1,000 requests per hour per repository.
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#requests-from-github-actions
That sounds pretty high, I expect we can raise it. If operations-per-run: 30 dealt with 400 PRs, shall we try x4: operations-per-run: 120 to deal with the 1,592 open PRs?
Note we have two steps running in the action, one for adding the stale label, one for closing stale+no-CLA.
Also:
We can change the order PRs are fetched, from default ascending: false (get newest first) to ascending: true (get oldest first).
https://github.com/actions/stale#ascending
Does it make sense to deal with the older PRs first? They're more likely to be stale/abandoned. At least for the closing step. erlend-aasland commented on Feb 18, 2022
If operations-per-run: 30 dealt with 400 PRs, shall we try x4: operations-per-run: 120 to deal with the 1,592 open PRs?
Sounds good to me :)
Does it make sense to deal with the older PRs first?
+1 hugovk commented on Feb 18, 2022 Reacted by Erlend E. Aasland hugovk commented on Feb 19, 2022 That was merged and ran last night, and is looking good.
https://github.com/python/cpython/runs/5255121854?check_suite_focus=true
Some more detail.
From "Check PRs with 'CLA not signed' label":
...
Warning: No more operations left! Exiting...
Warning: If you think that not enough issues were processed you could try to increase the quantity related to the operations-per-run ([https://github.com/actions/stale#operations-per-run)](https://github.com/actions/stale#operations-per-run%E2%80%8B)) option which is currently set to 120
Statistics:
Processed PRs: 37
New stale PRs: 30
Added PRs labels: 30
Added PRs comments: 30
Fetched items: 100
Fetched items events: 31
Fetched items comments: 31
Operations performed: 123
-> Switching to processing oldest first has helped, it's labelled some old PRs from 2017.
From "Check PRs with 'CLA not signed' label":
...
Batch #16 processed.
No more issues found to process. Exiting...
Statistics:
Processed PRs: 1587
New stale PRs: 4
No longer stale PRs: 4
Closed PRs: 10
Deleted PRs labels: 4
Added PRs labels: 4
Added PRs comments: 14
Fetched items: 1587
Fetched items events: 19
Fetched items comments: 19
Operations performed: 87
->
13 still open:
These 10 are now closed:
#27848 #27552 #27341 #26930 #26458 #24942 #24474 #23998 #22695 #22671
And 3 still open:
#22696 curiously had the stale label removed:
[#24556] Remove the stale label since the pull request has a comment and the workflow should remove the stale label when updated
[#24556] The pull request is no longer stale. Removing the stale label...
[#24556] Removing the label "stale" from this pull request...
[#24556] The label "stale" was removed
[#24556] Skipping the process since the pull request is now un-stale
[#24556] 2 operations consumed for this pull request
So what happened? The label was applied some time ago, comments were then made, the label should have been removed but the PR dropped off the list of processed PRs until now. So that's okay.
Same story for the other two: #25744 #24556 Reacted by Erlend E. Aasland erlend-aasland commented on Feb 19, 2022 Looks good. Thanks for going through all the details for us, @hugovk! Reacted by Hugo van Kemenade hugovk commented on Feb 19, 2022 Reacted by Erlend E. Aasland and Łukasz Langa hugovk commented on Mar 10, 2022 Okay, things are looking good here, let's close this :)
Currently 1 open CLA not signed + stale, and 37 closed. Reacted by Erlend E. Aasland
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|
