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

x/build/cmd/gopherbot: assignReviewersToCLs doesn't add reviewers if a CL was imported from a PR #31658

Open
dmitshur opened this issue Apr 24, 2019 · 1 comment

Comments

@dmitshur
Copy link
Member

commented Apr 24, 2019

Gopherbot has a task to assign reviewers to CLs based on owners:

// assignReviewersToCLs looks for CLs with no humans in the reviewer or cc fields
// that have been open for a short amount of time (enough of a signal that the
// author does not intend to add anyone to the review), then assigns reviewers/ccs
// using the golang.org/s/owners API.
func (b *gopherbot) assignReviewersToCLs(ctx context.Context) error

It doesn't work when someone makes a Pull Request, that then gets imported as a Gerrit CL by gerritbot.

For example, CL 173577 was imported from a PR and has had no activity but gopherbot did not assign a reviewer. /cc @stamblerre

This is a bug in gopherbot, and I suspect it happens because of issue #30265 (which is in turn due to #23964). In that CL, GerritBot added the author of the PR as a reviewer of the CL:

image

That makes the following check in assignReviewersToCLs return early:

if b.humanReviewersOnChange(ctx, gc, cl) {
	return nil
}

If not for that, gopherbot would've taken the right action:

2019/04/24 13:12:18 No reviewers or cc: https://go-review.googlesource.com/c/tools/+/173577
2019/04/24 13:12:18 [dry run] Would set review on https://go-review.googlesource.com/c/tools/+/173577: {Message: Labels:map[] Comments:map[] Reviewers:[{Reviewer:rstambler@golang.org State:} {Reviewer:iancottrell@google.com State:}]}

Ideally this should be fixed by resolving the root cause issue #30265. However, that might take much longer to resolve, so we can look into adding a workaround for it now by modifying humanReviewersOnChange to not consider authors of the CL as reviewers.

/cc @bradfitz @andybons

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

we can look into adding a workaround for it now by modifying humanReviewersOnChange to not consider authors of the CL as reviewers.

Doing that is proving to be difficult because of issue #23964.

The reviewers are all specified in Gerrit IDs, like:

Reviewer: Gerrit User 31858 <31858@62eb7196-b449-3ce5-99f1-c037f21e1705>
Reviewer: Gerrit User 16140 <16140@62eb7196-b449-3ce5-99f1-c037f21e1705>

To skip the author CL, we need to know their Gerrit account ID. But for CLs that were imported from PRs, the owner ID is always "Gerrit User 12446 <12446@62eb7196-b449-3ce5-99f1-c037f21e1705>", i.e., "GerritBot" itself.

This means it may be required to make some additional network requests to Gerrit API; data in maintner may not be enough to skip authors from reviewers.

It may be possible to use a heuristic though: if the CL author is GerritBot, then we should require 2 or more human reviewers (since one of them will always be the CL author).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.