Test-infra: mungegithub: blunderbuss: could we use GH reviewers instead of assignees?

[stevekuznetsov]

The mix of terminology between reviewers and assignees is confusing to new users to the system. Could blunderbuss request review from the members that it requires review from? If we think we want to continue allowing anyone to do a review and /lgtm, we should be OK to use reviewers even though anyone can change that list (as opposed to assignees, which are more locked down).

/cc @fejta @cjwagner @apelisse

[cjwagner]

Are you suggesting making blunderbuss request reviews from users instead of assigning them? That sounds fine to me. I don't think its a very important distinction though, they both request that the user looks at a PR.
I'm not sure what you mean by assignees being more locked down than reviewers though or reviewers being changed by anyone. How could anyone change the reviewers list? The list is in an OWNERS file which can't be changed without a PR that is approved by the approvers in the file or a higher level OWNERS file.

[stevekuznetsov]

I don't think its a very important distinction though

Yeah, it's not a huge thing but new users to the robot as we are seeing get pretty confused between the "reviewer" terminology in the OWNERS file and the fact that these members get added as assignees, not as reviewers, in GitHub.

How could anyone change the reviewers list?

Sorry -- this is an example of the confusion -- I meant the list of people in the reviewers bucket on GitHub, not the reviewers: list in OWNERS.

[cjwagner]

Ah I see what you mean. I don't think that the reviewers bucket on github is any more locked down than the assignees bucket though. At least for me when I go to a PR in kubernetes/kubernetes I cannot manually assign a reviewer or approver with the github buckets (I have to use a prow command). This seems like a good change to me though in any case.

[stevekuznetsov]

I'm not super clear on all of the interaction with the reviewer bucket, but I think if you do a review, you get added as a reviewer, regardless of whether a review was requested of you or not. This actually sounds like the current status of /lgtm (we add you as an assignee if you do it). I'll write a PR to see what the change looks like.

[BenTheElder]

This sounds good to me. I think we should try to be as consistent with terminology as possible, k8s has plenty of jargon already :-)

[BenTheElder]

I would like to see this happen but it's a bit outside of what I'm working on ATM and might need more discussion. It should (?) be fairly easy to do if anyone wants to go for it.

[stevekuznetsov]

The alternate proposal from @fejta was that blunderbuss (or the spiritual successor in prow) should tell you how to assign people but not do it itself. I like that approach, too.

[kargakis]

What's the value of the reviewers section in OWNERS files if we require from users to request reviews then?

[stevekuznetsov]

Helps to make it clear to users it's on them to drive the review and approval process for their PR and gives them a starting point for who to ping about the issue -- same as what we currently do to suggest approvers.

[kargakis]

Imagine that as a new contributor I make a change that spans multiple directories. Now I need to determine who is the most appropriate reviewer out of all the directories' OWNERS files or potentially ping people who have no interest in reviewing my changes just because they are not the real owners of the parts I am changing. Not the best experience I would want as a contributor.

[kargakis]

Or the directories I am changing have no OWNERS file so assuming I figure out how OWNERS work, I need to chase files in parent directories. Ouch.

[stevekuznetsov]

Not sure I follow -- same as today for approvals, the comment will contain "when you are ready for review. /assign XXXX"

[cjwagner]

I'm starting work on a Prow version of blunderbuss. I'm planning to make it request reviews instead of assigning users.
Should I have it suggest reviewers with a comment instead of actually requesting the review? I could handle pull_request events with action review_requested or assigned and delete the comment in either case.

[stevekuznetsov]

Seems reasonable -- suggest people who the PR author could /cc and /assign but don't tag them directly, and clean up the comment as the PR actually gets reviews requested (or reviews submitted) and assignees added.

[spiffxp]

Big +1 to requesting reviews instead of assigning

I like auto-assigning or auto-requesting, because we can use the # of pr's assigned or number of reviews requested as a proxy for how large someone's review workload is.

The amount of manual human intervention required to herd a k/k PR is really off-putting to newcomers to the community. I feel like a lot of this is because we're working around the noise that is github notifications. I think our automation could help drive this process? eg: once it sees an lgtm label it could drop requested reviewers and auto assign the suggested approver(s).

[cjwagner]

I like auto-assigning or auto-requesting, because we can use the # of pr's assigned or number of reviews requested as a proxy for how large someone's review workload is.

Wouldn't this number be more accurate if we did not auto-assign or auto-request reviews and instead only suggested reviews? I think requested reviewers are more likely to actually review a PR if the author has explicitly requested a review from them instead of being auto-requested.

The amount of manual human intervention required to herd a k/k PR is really off-putting to newcomers to the community.

Making PR authors request reviews themselves is certainly an extra step compared to auto-requesting if the auto-requested reviewers would have been appropriate. That being said I think this extra step could improve the experience for newcomers specifically, because it would show them how to request reviews themselves while still suggesting a user who can review. I don't feel strongly about this though, its not hugely beneficial for new users to know how to /cc right off the bat especially if we have already requested reviews for them.

[spiffxp]

I think requested reviewers are more likely to actually review a PR if the author has explicitly requested a review from them instead of being auto-requested.

Fair. I get feedback from people who aren't used to having to shepherd PR's through other projects, but I'm open to seeing us try it out, and changing if the data shows this is hurting vs. helping.

It'd be nice to express this in terms of something we can measure. I guess we would see time from open to LGTM conceivably go down here https://devstats.k8s.io/dashboard/db/time-metrics (I couldn't find a board that measures time from assignment to review, just time to first non-author comment)

[kargakis]

We should track command usage from a couple of different groups: first-time contributors, members outside the trusted org, and org members.

[cjwagner]

I'll make the plugin auto-request reviews for now to minimize process change and limit the amount of required human interaction. We can try changing the plugin to suggest the reviewers instead of auto-requesting in a future PR once we have metrics to compare the two options.

[kargakis]

We will need a way to figure out whether a requested reviewer actually did the review.

[spiffxp]

@kargakis devstats attempts to do something like that https://devstats.k8s.io/dashboard/db/suggested-approvers?orgId=1

it's difficult to parse out because /lgtm can mean something different depending on whether you're in OWNERS as an approver or reviewer (this is why I wish we added the extra friction to require an explicit /approve for approval)

[cblecker]

The discussion here is valuable, but this is also a significant workflow change.

The idea of "assigning" PRs to a reviewer puts a degree of obligation on those users to actually do a review. It denotes ownership.. The assignees own the PR and have a duty per our community guidelines to shepherd it though, or unassign themselves/find someone else to assign to. (I realize that isn't always what happens today.)

We would also be disrupting any workflows that reviewers have developed that depend on actually being assigned to PRs.

I'd suggest getting @kubernetes/sig-contributor-experience-proposals involved, and possibly requesting feedback from kubernetes-dev@ as a whole before moving forward with this change.

The amount of manual human intervention required to herd a k/k PR is really off-putting to newcomers to the community.

Absolutely agree with this. However, I can see the possibility of changing workflows on very busy reviewers without consultation/communication may have the opposite effect of easing the issue.

[cblecker]

/reopen
Reopening for discussion.

[k8s-ci-robot]

@cblecker: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/reopen
Reopening for discussion.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kargakis]

it's difficult to parse out because /lgtm can mean something different depending on whether you're in OWNERS as an approver or reviewer (this is why I wish we added the extra friction to require an explicit /approve for approval)

If you are assigned as a reviewer in a PR and add /lgtm, most likely you have reviewed the PR. Not sure there is another way to figure out whether automatic review requests work today.

[cblecker]

@fejta @spiffxp @cjwagner Was consensus on this reached in some other forum than this issue? It was noted in https://github.com/kubernetes/test-infra/pull/5547#issuecomment-346165547 that this behaviour shouldn't have been deployed without consensus/communication, but it looks like https://github.com/kubernetes/test-infra/pull/6321 rolled this out.

[cjwagner]

@cblecker Sorry, thats my mistake. I meant to address this first, but two months had gone by and I completely forgot that there were reservations about process change.

If it is necessary, I can recreate the mungegithub instances and revert to using the munger again (:nauseated_face:) or change the Prow plugin to assign instead of requesting reviews. However, given that there have been no complaints about requesting reviews instead of assigning and no indication that we have broken any custom workflows is it necessary to revert this? Obviously this isn't the correct way to roll out changes, but since it has already happened and has been working fine for a month should we really revert it?

[cblecker]

@cjwagner We need to get communication out to people as they may not even have realized that their workflow has been impacted. The other thing that made me think and come back to this is the discussion around the meaning of /lgtm, and that lgtm still assigns people. And when we are looking for an approver, we also suggest to assign people.

The other datapoint we need to look at, is since this workflow change was put in place, have we seen a drop in responsiveness.

Can we get something out to k-dev indicating that this change was already made and soliciting feedback?

@spiffxp @parispittman @grodrigues3: This also makes me think about the idea of "ownership" of code/process changes. This is a piece where the process is more contribex, but the change was initiated from the test-infra side.

[bgrant0607]

Why does this PR have a ridiculous number of reviewers?
https://github.com/kubernetes/kubernetes/pull/60771

[kargakis]

Why does this PR have a ridiculous number of reviewers?
kubernetes/kubernetes#60771

The algorithm that requests reviews has changed; see https://github.com/kubernetes/test-infra/pull/5960

This was an issue in Openshift for folks so we reverted back to the old algorithm (https://github.com/kubernetes/test-infra/pull/6647 + https://github.com/openshift/release/pull/579) but I haven't seen any formal discussion about this in Kube.

[cblecker]

As for this particular issue, @cjwagner sent a message to k-dev, and we haven't received any negative feedback. I'm fine with leaving this as-is for the time being.

/remove-help
/close