aws
(meta): Review stale pull request closure policy
Describe the bug
I have authored two PRs for AWS CDK, #31844 and #31771; both of which have been closed for being stale. On the first PR, I received a comment with:
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.
I then merged in main, but it was still closed three weeks later. Comments are also locked automatically so I cannot raise any problems on the PRs themselves.
What is the benefit of this stale PR policy, especially when these PRs haven't had a single review yet so the author can't make any further changes anyway? I understand that the CDK team is busy, so may not get around to reviewing these PRs, but people have spent time on contributing code which presumably works and their PRs are just being closed for being "stale"? Surely the project is losing valuable contributions and discouraging others from contributing in the future with this policy?
I understand that there may be a benefit for this stale policy where a review has been conducted and the PR hasn't been worked on for a long time, but if a review hasn't been conducted then what work is expected to be conducted on a working but otherwise months old PR?
There's some PRs which are repeatedly closed again by the bot after having been reopened by a maintainer, such as #32404. It does seem like we're losing valuable contributions here and being too bitey to new contributors.
Regression Issue
- [ ] Select this option if this issue appears to be a regression.
Last Known Working CDK Version
No response
Expected Behavior
The stale pull request policy is removed, or is at least paused when there hasn't been a review yet.
Current Behavior
Stale pull requests are closed and comments are locked automatically.
Reproduction Steps
N/A
Possible Solution
No response
Additional Information/Context
No response
CDK CLI Version
N/A
Framework Version
No response
Node.js Version
N/A
OS
N/A
Language
TypeScript
Language Version
No response
Other information
No response
Assigning it to team's board for reviewing the process. Will bring this up with team offline as well.
@aaythapa Thanks for https://github.com/aws/aws-cdk/pull/33346, that's really useful. However, in https://github.com/aws/aws-cdk/pull/31844, I received the closure warning, I then merged in main, but the PR was auto closed 3 weeks later without another warning. I haven't taken a deep look into the closure action, but does the bot only post an initial warning on a PR without warning another closure window is about to commence? If so I think this could be misleading.
I'm trying to avoid only calling out my PRs in this issue, so I did spend some time looking through stale PR closures earlier (from beginner contributors as I think a bad experience for them means we're more at risk of losing future contributions than someone who has contributed several PRs).
- #32484 - PR opened 11 December 2024, reviewed 3 February 2025, closed and locked 10 February 2025.
- #30621 - PR opened 22 June 2024, reviewed 10 January 2025, author responded to the review question on the same day, closed and locked 10 February 2025.
- #30843 - PR opened 12 July 2024, exemption request responded 30 January 2025, author commented
"Could the timer be reset just so I have time to work on this PR in the next few days hopefully? Thank you very much."5 February 2025, closed and locked 7 February 2025.
Again, I'm not entirely sure on all of the reasons behind this policy, and I know this doesn't apply to all of the closure PRs, but do we think 7 days is sufficient to allow an author to pick back up a PR that could be several months old? I think in the interim, we could at least bump up this time window a bit.
Hi @georeeve thanks for opening this issue and the data points, they're really useful and I can bring them up with the team to re-asses our process.
AFAIK our closing stale PRs policy applies to PRs that have failing builds or linter rules. If those aren't passing (and another contributor doesn't review) then the PR will not show up on our radar. I see the PRs you linked had failing builds and that is why they were closed. If you have questions you can manually add the exception-requested and/or clarification-requested labels to the PR. This isn't made clear in the comments left by the bot so I updated the message here https://github.com/aws/aws-cdk/pull/33346.
I'll look at the closing stale PRs workflow when I can to see what changes we can make to make things as transparent as possible (and I agree with you that the time window should be bumped at least). I'll post any updates here and you can leave your thoughts on it.
Thanks @aaythapa, that policy makes more sense. Obviously it's not reasonable for the CDK team to review years old failing PRs. Unfortunately I think there is still a situation where for example, the staleness closure is blocked by an exemption request, a reviewer then reviews this exemption request and the original PR author responds; but then the PR is automatically closed 7 days later without a chance for the conversation to continue. Could we maybe block the staleness closure for longer if the PR author has responded to a comment?
@leonmk-aws It looks like when @aaythapa reopened #31771 (and #31844), the comments lock stayed on. Can you reopen the conversation so I can reply there? Thanks for your review by the way!