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

Bors policy question: Auto-reassignment on r+ #59489

Closed
Centril opened this issue Mar 28, 2019 · 20 comments
Closed

Bors policy question: Auto-reassignment on r+ #59489

Centril opened this issue Mar 28, 2019 · 20 comments
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Mar 28, 2019

Currently, the behavior of @bors is that whenever you @bors r+ or @bors r=UserFooBar, the reviewer doesn't change. However, this makes it less clear who the current reviewer is. For example, the assignee might not have even looked at the PR before someone else has r+ed it. I believe having an up to date assignee more frequently would simplify triage. Also, as @pnkfelix noted:

if one steals review action, it certainly seems like one should take on the burden of assignment as well.

While one could r? @Centril @bors r+ and such all the time, people often forget this and it instead causes churn to do so manually. As an example, there's #59468.

Instead, I propose that:

  • Whenever you r+ a PR, you get r? as well.
  • If you r=UserFooBar, and @UserFooBar is not assigned, then they will be assigned.

The tagged teams are those that typically use @bors in this repo or perform triage and who will therefore be affected by this policy.

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Mar 28, 2019
@Centril Centril self-assigned this Mar 28, 2019
@Centril Centril changed the title [WIP] Bors policy question: Auto-reassignment on r+ Bors policy question: Auto-reassignment on r+ Apr 1, 2019
@Centril
Copy link
Contributor Author

Centril commented Apr 1, 2019

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 1, 2019

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 1, 2019
@estebank
Copy link
Contributor

estebank commented Apr 1, 2019

@rfcbot reviewed

@pietroalbini
Copy link
Member

What should happen when the PR author approves the PR themselves (like when users are delegated and forget to r=reviewer, doing r+ instead)? I think we should skip reassigning if the person that'd be reassigned is the PR author.

Also, sometimes we do r=reviewer1,reviewer2, do we want to assign both in that case?

@kennytm
Copy link
Member

kennytm commented Apr 1, 2019

Would the original reviewer be unassigned?

If we do @bors delegate=«user» would it assign the PR to «user» too?

@Centril
Copy link
Contributor Author

Centril commented Apr 1, 2019

I think we should skip reassigning if the person that'd be reassigned is the PR author.

Sure; seems fine... that's an implementation detail imo ;)

Also, sometimes we do r=reviewer1,reviewer2, do we want to assign both in that case?

Seems reasonable -- otherwise pick the first?

If we do @bors delegate=«user» would it assign the PR to «user» too?

Seems like @pietroalbini's note... so we'd skip in that case?

@shepmaster
Copy link
Member

We cannot assign to users that are not members of the repo, correct?

@pietroalbini
Copy link
Member

Nope, to be able to be assigned an user needs at least to have explicit read access to the repo.

@petrochenkov
Copy link
Contributor

We should probably conservatively do nothing on @bors r=anything, anything not being already assigned is not a common situation.

@petrochenkov
Copy link
Contributor

@Centril
Meta: I don't think you'll ever get a complete FCP from this many people.
(2/3 or 60% are used as a consensus sometimes.)

@Centril
Copy link
Contributor Author

Centril commented Apr 1, 2019

@petrochenkov hehe; maybe not... let's see ;) there have been similar situations that have worked.

As for r=foo, I think it would be fine to do nothing; I mostly care about r+.

@petrochenkov
Copy link
Contributor

@Centril
There were also situations when similar changes were made with much less process overhead.

@kennytm
Copy link
Member

kennytm commented Apr 1, 2019

Last time in #56951 the list has 26 people. For this there are 42 people. Let's see :)


IIRC r+ and r=foo share the same code path. Additionally, due to delegate=«user», the «user» sending r+ will have the same issue anyway. But assigning to a user outside the org is no-op anyway, I don't see a big problem as long as the original assignee is kept intact.

@Centril
Copy link
Contributor Author

Centril commented Apr 12, 2019

@BatmanAoD
Copy link
Member

@Centril Sorry, for some reason I mixed this up with your "test, please ignore" issue and thought review hadn't actually been requested from me. My bad!

@Centril
Copy link
Contributor Author

Centril commented May 5, 2019

@Centril
Copy link
Contributor Author

Centril commented May 20, 2019

Since there are only 3 checkboxes remaining and I've tried to reach those involved on many occasions I'm going to take the liberty to tick one of them (under the same "on leave" rationale) so that we can move on...

@Centril
Copy link
Contributor Author

Centril commented May 20, 2019

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 20, 2019
@rfcbot
Copy link

rfcbot commented May 20, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 20, 2019
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 30, 2019
@rfcbot
Copy link

rfcbot commented May 30, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants