Skip to content

Conversation

@rbtr
Copy link
Collaborator

@rbtr rbtr commented Dec 15, 2023

Reason for Change:

lowers threshold for stale to 2 weeks
lowers threshold for close to 1 week
deletes branches for closed stale PRs

Issue Fixed:

Requirements:

Notes:

@rbtr rbtr requested a review from a team as a code owner December 15, 2023 17:25
@rbtr rbtr requested review from nddq and ramiro-gamarra December 15, 2023 17:26
@rbtr rbtr self-assigned this Dec 15, 2023
@rbtr rbtr added the ci Infra or tooling. label Dec 15, 2023
@rbtr rbtr enabled auto-merge (squash) December 15, 2023 17:26
exempt-issue-labels: 'blocked,must,should,keep'
days-before-close: 7
days-before-stale: 14
delete-branch: true
Copy link
Contributor

@ramiro-gamarra ramiro-gamarra Dec 15, 2023

Choose a reason for hiding this comment

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

Since the new workflow uses branches from this repo instead of forks, not sure if we want to delete the branch. With forking I always have a remote with my branch, but now all my branches are going to be here and I may not always have a local to push again. Let me know if am misunderstanding this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your understanding is correct. This was not previously enabled because it couldn't delete forked branches anyway.
I think deleting the branch if a PR is closed for being stale is appropriate. It will be 3 weeks of total inactivity - after 2 weeks, the stale label is added, and anyone can remove that to preserve the PR. There's definitely the potential to lose a branch, but there needs to be motivation to not abandon PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are stale branches that much of an issue? I definitely agree with closing PRs to eliminate noise, but they can always be reactivated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could similarly argue that the official Org repo should be for actively contributing and collaborating and stale branches are also noise and don't belong there. It's straightforward enough to re-push a branch from your local, or to work primarily in a fork and only push to the upstream remote when you're ready to actually open the PR and follow through all the way to merging it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that even if branches are deleted, there is a restore option in the PR as well (although not sure how long that remains available after delete). Even if the branch cannot be restored, commits should still be discoverable via the closed PRs, so work won't be totally lost, which is my main concern. I think I'm ok to try this change and see if we have any issues in the next few months. It seems you will still need to add one more permission for this to work though: https://github.com/actions/stale?tab=readme-ov-file#delete-branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added contents: write

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@rbtr rbtr disabled auto-merge December 19, 2023 20:52
@rbtr rbtr merged commit 9f82662 into master Dec 19, 2023
@rbtr rbtr deleted the stale branch December 19, 2023 20:52
paulyufan2 pushed a commit that referenced this pull request Jan 2, 2024
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
matmerr pushed a commit that referenced this pull request Jan 17, 2024
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Infra or tooling.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants