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

Bots to manage PR state in labels #7306

Open
pradyunsg opened this issue Nov 6, 2019 · 5 comments
Open

Bots to manage PR state in labels #7306

pradyunsg opened this issue Nov 6, 2019 · 5 comments
Labels
C: automation Automated checks, CI etc state: needs discussion This needs some more discussion

Comments

@pradyunsg
Copy link
Member

If PRs automatically get a label describing what state they're in, IMO it'll make it easier to handle PR reviews. To copy from CPython, I think we'd benefit from these labels:

  • S: awaiting response (exists)
  • S: awaiting review
  • S: awaiting merge

I think the approximate flow would be:

  • new PR: apply S: awaiting review once CI passes (or on open?)
  • has "changes requested" (red review): switch to S: awaiting response
  • new commit, or comment in "awaiting response" state: switch to S: awaiting review
  • approved and has no "changes requested": switch to S: awaiting merge

We'd still be able to manually apply these labels and if the above is the behavior of the bots, I don't think they'd fight us. Do others think this would be a good idea?

@pradyunsg pradyunsg added C: automation Automated checks, CI etc state: needs discussion This needs some more discussion labels Nov 6, 2019
@pfmoore
Copy link
Member

pfmoore commented Nov 6, 2019

LGTM.

new PR: apply S: awaiting review once CI passes (or on open?)

When CI passes. If someone wants a review before they have a clean CI, they can ask for one (with an explanation if needed) but I think it's good that we take the stance that CI passing is the basic criterion we'd expect.

@chrahunt
Copy link
Member

chrahunt commented Nov 10, 2019

I would apply S: awaiting review once required CI passes - we don't want experiments with CI checks to mess up our workflow. I think this would be an improvement over the red X that currently shows on the PR overview page since an S: awaiting review with red X would indicate that some secondary check has failed, not the main test suite/lints and it's probably worth taking a look.

I don't know if I would find S: awaiting merge useful as-is - if I approve something and don't merge it immediately it is because:

  1. the change is nontrivial and another maintainer may have an opinion in this area but I don't think time-wise they've had enough opportunity to review it (maybe a 24 hour window depending on time zones and whether people are active on other issues)
  2. another maintainer has commented (possibly as "comment" not just "changes requested") and there hasn't been that much time since the latest change that it's reasonable to assume they've reviewed it
  3. the PR was submitted by a maintainer and they can merge at their discretion i.e. nothing immediately depends on the changes in that PR

in the first two cases keeping S: awaiting review would indicate that someone should take a look and if that someone thinks it's OK to merge then they can do it at that point.

@pradyunsg
Copy link
Member Author

My goal with awaiting merge is to avoid PRs from lingering. Any suggestions on how we could do that? Maybe some combination of delays or something else entirely?

@chrahunt
Copy link
Member

chrahunt commented Nov 10, 2019

Maybe I'm misunderstanding. If the transition to S: awaiting merge requires a maintainer action (the last person with "changes requested" approves) and it means that it can be merged, wouldn't that maintainer merge it?

Also what phase are we concerned about PRs lingering in? Awaiting review or response or approved but not merged?

@pradyunsg
Copy link
Member Author

Also what phase are we concerned about PRs lingering in?

Approved and not merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: automation Automated checks, CI etc state: needs discussion This needs some more discussion
Projects
None yet
Development

No branches or pull requests

3 participants