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

Jump to bottom

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

Closed
dmitshur opened this issue Apr 24, 2019 · 3 comments
Closed

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

dmitshur opened this issue Apr 24, 2019 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) Community FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Apr 24, 2019 •
edited

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 dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done. Community labels Apr 24, 2019
@dmitshur dmitshur added this to the Unreleased milestone Apr 24, 2019
@dmitshur dmitshur self-assigned this Apr 24, 2019
@dmitshur dmitshur mentioned this issue Apr 24, 2019
Open
@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 24, 2019 •
edited

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).

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 24, 2019
@stamblerre stamblerre mentioned this issue Apr 8, 2020
Open
@dmitshur dmitshur removed their assignment Apr 10, 2020
@stamblerre stamblerre self-assigned this Apr 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/227977 mentions this issue: cmd/gopherbot: fix reviewer assignment for CLs imported from PRs

@stamblerre
Copy link
Contributor

Just mailed the above CL, which uses your suggestion above to require 2 reviewers.

I tried parsing out the author's name and email from the first comment from GerritBot and matching against that in the reviewers, but then I realized that would require requesting reviewers with detailed account information. It's probably too much effort to do that anyway 😄

@golang golang locked and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) Community FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dmitshur @stamblerre @gopherbot