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

Adding GitHub Actions workflow to close stale PRs (Lombiq Technologies: OCORE-154) #15710

Merged
merged 14 commits into from
Apr 17, 2024

Conversation

Piedone
Copy link
Member

@Piedone Piedone commented Apr 9, 2024

Part of #15029.

@Piedone Piedone mentioned this pull request Apr 9, 2024
13 tasks
@Piedone Piedone requested a review from MikeAlhayek April 16, 2024 20:29
@MikeAlhayek
Copy link
Member

Hopefully a stale PR is one with 0 approval. Maybe we should exclude those with "need-triage" label as well.

@Piedone
Copy link
Member Author

Piedone commented Apr 16, 2024

We can exclude those (this action is super configurable) but do we want to? A PR will be marked as stale (label and comment added) after 60 days of inactivity (even a comment, or adding a label counts as activity), and closed 15 days after that. If nobody cared about a PR for 75 days then there's not much else to talk about.

@MikeAlhayek
Copy link
Member

Well if a PR has at least one approval, then it should not be subject to be stale. If someone marked it with need-triage, then they think it is worth looking into. So we should not mark it stale. If we do the triage correctly, then these would be reviews with in a week. At that time, either we merge or provide feedback and remove the need-triage label. If we provide feedback, and don't get a response after 60 days then sure stale and close it. But as long as it is awaiting triage, we should not stale it.

@Piedone
Copy link
Member Author

Piedone commented Apr 16, 2024

The 75 days starts at the last activity, and any activity resets it (if it's after the PR is marked stale, then you also need to remove the label).

If a PR awaiting triage wasn't touched for 60 days, then upon the stale comment being added the reviewer adding "needs triage" has the chance to do something. Bring it up at the meeting, merge from main, or just comment "I'll bring this up in the next few weeks, pinky promise.". Then the 60 days start anew. However, something really should happen. I don't think we can hope for a PR to be ever picked up if nobody cared about it for 2,5 months.

When going through all the PRs, I found that a PR being dropped by a reviewer (or not reviewed by anyone), or dropped by the author after a review, were soft "no"s. This isn't how we should operate, because this results in almost 200 PRs piling up, at least a quarter of which were silent quit like this. Instead, we should provide clear feedback on each PR, including if we won't take it (which sucks, but is inevitable). If nobody from the team gives this clear feedback (which also is an issue), then auto-closing it can at least prevent the PR from just sitting in limbo forever.

(A PR receiving approval that the reviewer doesn't deem requiring a second opinion should just be merged right away.)

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good with this. If we find that this is causing issue we can then modify as needed.

@Piedone Piedone merged commit 58a98d9 into OrchardCMS:main Apr 17, 2024
4 checks passed
@hishamco
Copy link
Member

I'm afraid that another GC collector is coming on the way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants