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): Review stale pull request closure policy #33335

Open
1 task
georeeve opened this issue Feb 7, 2025 · 4 comments
Open
1 task

(meta): Review stale pull request closure policy #33335

georeeve opened this issue Feb 7, 2025 · 4 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. p2

Comments

@georeeve
Copy link

georeeve commented Feb 7, 2025

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

@georeeve georeeve added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 7, 2025
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Feb 7, 2025
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Feb 7, 2025
@ashishdhingra
Copy link
Contributor

Assigning it to team's board for reviewing the process. Will bring this up with team offline as well.

@ashishdhingra ashishdhingra removed the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Feb 7, 2025
@georeeve
Copy link
Author

@aaythapa Thanks for #33346, that's really useful. However, in #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.

@georeeve
Copy link
Author

georeeve commented Feb 10, 2025

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).

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.

@aaythapa
Copy link
Contributor

aaythapa commented Feb 10, 2025

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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. p2
Projects
None yet
Development

No branches or pull requests

3 participants