Skip to content

[SPARK-30173][INFRA] Automatically close stale PRs #26877

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.

Sign up for GitHub

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

Jump to bottom
Closed
nchammas wants to merge 3 commits into from
Closed

[SPARK-30173][INFRA] Automatically close stale PRs #26877

nchammas wants to merge 3 commits into from

Conversation

@nchammas
Copy link
Contributor

@nchammas nchammas commented Dec 13, 2019 •
edited
Loading

What changes were proposed in this pull request?

This PR adds a GitHub workflow to automatically close stale PRs.

Why are the changes needed?

This will help cut down the number of open but stale PRs and keep the PR queue manageable.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I'm not sure how to test this PR without impacting real PRs on the repo.

See: actions/stale#32

@nchammas
Copy link
Contributor Author

nchammas commented Dec 13, 2019

@HyukjinKwon and @srowen - I've put in some dummy values and prose to give you something to look at. Let me know what you think the stale timeout and message should be. I've linked to the action doc in the PR description.

@SparkQA
Copy link

SparkQA commented Dec 13, 2019

Test build #115281 has finished for PR 26877 at commit 1f3a66d.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen requested changes Dec 13, 2019
View reviewed changes
.github/workflows/stale.yml Outdated
a way of keeping the PR queue manageable.

If you'd like to revive this PR, please reopen it!
days-before-stale: 3650
Copy link
Member

@srowen srowen Dec 13, 2019

Choose a reason for hiding this comment

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

This would be 10 years? 6 months is OK, or maybe less. 1000 days? I also don't think it needs to warn.

Copy link
Contributor Author

@nchammas nchammas Dec 13, 2019

Choose a reason for hiding this comment

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

I also don't think it needs to warn.

Are you talking about days-before-close?

Copy link
Member

@srowen srowen Dec 13, 2019

Choose a reason for hiding this comment

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

Both. I think this number has to be lower. Oops, I meant 100 days not 1000 BTW. Also I don't think we have to warn before closing.

nchammas
nchammas commented Dec 13, 2019
View reviewed changes
.github/workflows/stale.yml
days-before-stale: 100
# Setting this to 0 is the same as setting it to 1.
# See: https://github.com/actions/stale/issues/28
days-before-close: 0
Copy link
Contributor Author

@nchammas nchammas Dec 13, 2019

Choose a reason for hiding this comment

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

@srowen - I've set days-before-close to 0, which in theory means the PR will be closed immediately after it crosses the stale threshold. However, due to the linked issue, this will in practice close the PR 1 day after tagging it as stale and posting the message.

@SparkQA
Copy link

SparkQA commented Dec 13, 2019

Test build #115308 has finished for PR 26877 at commit 686d069.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 13, 2019

Test build #115311 has finished for PR 26877 at commit 424d40f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-30173] Automatically close stale PRs [SPARK-30173][INFRA] Automatically close stale PRs Dec 13, 2019
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Dec 13, 2019

I also saw the dev email, but do we have a vote for this?

@nchammas
Copy link
Contributor Author

nchammas commented Dec 13, 2019

Does it need a vote? Feel free to call one if so.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Dec 14, 2019

I believe it's up to you, @nchammas .
BTW, I'm +1 for your proposal. :)

@srowen
Copy link
Member

srowen commented Dec 14, 2019

I don't particularly think this needs a vote. It's a minor point of process.

@nchammas
Copy link
Contributor Author

nchammas commented Dec 14, 2019

I agree with Sean, but as a non-committer I don't feel I have the authority to make a call one way or the other.

@dongjoon-hyun - Would it be sufficient to just update the original email thread on dev about this and point people to this PR?

@srowen - Are you happy with the PR as it stands now?

srowen
srowen reviewed Dec 14, 2019
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks good; is there any way to do a test run just to make sure it does what we think?

@nchammas
Copy link
Contributor Author

nchammas commented Dec 14, 2019

Unfortunately not. To be safe, we can merge this in with a more lax timeout, or comment out the stale message (which disables actually closing any PRs), and see how it works before reverting the settings to what we ultimately want them to be.

@srowen
Copy link
Member

srowen commented Dec 14, 2019

Yeah, I guess I'm just concerned it might somehow close a bunch of new PRs. Well, I can make a list of them and if it all goes wrong can reopen them. I guess the default of operations-per-run = 30 helps limit what it will do per run. OK, I think we can give it a go.

@nchammas
Copy link
Contributor Author

nchammas commented Dec 14, 2019

Agreed. The action will mark stale PRs with a tag, so they should be easy to find.

@srowen srowen closed this in 58b2939 Dec 15, 2019
@srowen
Copy link
Member

srowen commented Dec 15, 2019

Merge to master

@nchammas nchammas deleted the SPARK-30173-stale-prs branch December 15, 2019 17:04
@nchammas
Copy link
Contributor Author

nchammas commented Dec 16, 2019 •
edited
Loading

@srowen - Thanks for kicking off a run of this workflow. It looks like AMP Lab Jenkins underwent some kind of reboot on September 16 and updated many PRs with a perfunctory "Can one of the admins verify this patch?" comment. We'll have to wait until Christmas for the 100 day timeout to expire and the initial wave of PRs to be marked as stale and closed.

@nchammas
Copy link
Contributor Author

nchammas commented Jan 3, 2020

Looks like the workflow is working. It's hitting the rate limit we set, but nonetheless working its way through our backlog of stale PRs.

@nchammas nchammas mentioned this pull request Jan 7, 2020
Closed
srowen pushed a commit that referenced this pull request Jan 7, 2020
@nchammas @srowen
[SPARK-30173] Tweak stale PR message
f399d65 
Follow-on to #26877.

### What changes were proposed in this pull request?

This PR tweaks the stale PR message to [clarify](#24457 (comment)) the procedure for reopening a PR after it has been marked as stale.

### Why are the changes needed?

This change should clarify the reopening process for contributors.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

N/A

Closes #27114 from nchammas/SPARK-30173-stale-tweaks.

Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srowen srowen srowen requested changes

Assignees

No one assigned

Labels

PROJECT INFRA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nchammas @SparkQA @dongjoon-hyun @srowen