-
Notifications
You must be signed in to change notification settings - Fork 511
ci: add stale PR management workflow #853
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
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/stale-pr.yml
Outdated
| days-before-stale: 14 | ||
| days-before-close: 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should up these numbers to 30 days for stale and 60 days for close, though I'm open to other feedback here.
Ideally things are merged much faster than that, but we should account for large changes that take a while to deliberate and for things like people going on vacations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should up these numbers to 30 days for stale and 60 days for close, though I'm open to other feedback here.
Ideally things are merged much faster than that, but we should account for large changes that take a while to deliberate and for things like people going on vacations.
I feel like 30 days grace period for making it stale is good and closing should be at 35th or 40th day this way we dont have large numbers of pr gathered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more comfortable with giving people a two week period to prevent their work from being closed. How do you feel about closing after 44/45 days?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more comfortable with giving people a two week period to prevent their work from being closed. How do you feel about closing after 44/45 days?
I feel like 45 days are enough time for closing
.github/workflows/stale-pr.yml
Outdated
| days-before-issue-stale: -1 | ||
| days-before-issue-close: -1 | ||
|
|
||
| remove-stale-when-updated: true No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not super necessary, but we should consider changing this to remove-pr-stale-when-updated.
.github/workflows/stale-pr.yml
Outdated
|
|
||
| The PR will be closed in 30 days if there's no activity. | ||
|
|
||
| If you need help or have questions, feel free to ask! We appreciate your contribution. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we remove this emoji
.github/workflows/stale-pr.yml
Outdated
| stale-pr-message: | | ||
| This pull request has been inactive for 2 weeks and will be marked as stale. | ||
|
|
||
| If you're still working on this, please: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make it clear that they only need to do one of the following list?
.github/workflows/stale-pr.yml
Outdated
| close-pr-message: | | ||
| This pull request was automatically closed due to inactivity. | ||
|
|
||
| If you'd like to continue this work, feel free to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, only one option is necessary. Though they should always feel free to comment if they need assistance :)
|
Thanks for working on this @Ronitsabhaya75, I think this will be really helpful for us to have! Could you update the PR description to match that this workflow is only for PRs? Since the repo is set to squash commits when we merge PRs, the PR description becomes the squashed commit's message, so we want to make sure we keep that accurate. Thanks! |
|
for sure katie I can
For sure Katie I'll update the description and update reviewed changes asap. Thank you so much for review |
| stale-pr-label: 'stale' | ||
|
|
||
| exempt-pr-labels: 'security,critical,dependencies' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a process for using labels like these today, so I think we should remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I'll def remove those labels
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@realrajaryan pointed out to me that there's an issue on actions/stale where issues: write was required even when only dealing with PRs here. The last comment on that issue mentions that someone was still hitting it in 2024, so it's possible that this is no longer an issue.
We should find a way to test this flow without needing the PR to be merged. That could look like changing the trigger on the flow and using only-labels to target a specific test label that we can create so we only comment on/close the test PR. Let me know if you want to try this and I can make the test label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@realrajaryan pointed out to me that there's an issue on actions/stale where
issues: writewas required even when only dealing with PRs here. The last comment on that issue mentions that someone was still hitting it in 2024, so it's possible that this is no longer an issue.We should find a way to test this flow without needing the PR to be merged. That could look like changing the trigger on the flow and using only-labels to target a specific test label that we can create so we only comment on/close the test PR. Let me know if you want to try this and I can make the test label.
sure we can give it a try
| stale-pr-label: 'stale' | ||
|
|
||
| exempt-pr-labels: 'security,critical,dependencies' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these labels don't exist
Type of Change
Motivation and Context
stalewith friendly reminderTesting