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/gerritbot: Gerrit asks PR author to review their own change #30265

Open
bradfitz opened this issue Feb 15, 2019 · 8 comments
Open

x/build/cmd/gerritbot: Gerrit asks PR author to review their own change #30265

bradfitz opened this issue Feb 15, 2019 · 8 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

In https://go-review.googlesource.com/c/oauth2/+/162937 I wrote:

I'm going to delete your code review vote because it's misleading. Reviewing your own code doesn't really count in terms of our code review requirements.

PR author @nwidger then replied:

My bad! I wasn't sure if doing that was appropriate. The email from Gerrit Bot said 'Gerrit Bot would like Niels Widger to review this change' so I thought maybe that's what it wanted me to do.

This is a tracking bug to understand why that happened and ideally prevent that message.

/cc @andybons @dmitshur
@gopherbot gopherbot added this to the Unreleased milestone Feb 15, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 15, 2019
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 15, 2019
@dmitshur

This comment has been minimized.

Sign in to view
@bradfitz

This comment has been minimized.

Sign in to view
@andybons
Copy link
Member

andybons commented Feb 15, 2019 •
edited by dmitshur

This is due to the inability of a user to upload changes as another person (which is issue #23964). Ideally the owner of the change is the original author, but in the case where gerritbot uploads a change to Gerrit, it automatically adds the author of the commit as a reviewer. I remember asking the Gerrit team about impersonating another user, but that requires engineering work on the Gerrit side as it’s a new feature. I’ll ask around if there’s anything that can be done to make this less confusing.

@bradfitz
Copy link
Contributor Author

work on the Gerrit side as it’s a new feature

"it'd be" a new feature or "it's a" new feature? That is, they didn't just add part of what we need for impersonation recently did they?

@andybons
Copy link
Member

It would be a new feature. They have not added the ability to do this for us, yet, and they have no plans to do so in the near future.

@dmitshur dmitshur mentioned this issue Apr 24, 2019
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Apr 24, 2019 •
edited

This issue is also causing #31658. /cc @stamblerre

@andybons Is the latest status still as before? In that case, I plan to work around it on our side by writing extra code in humanReviewersOnChange to filter out the CL author, so that we can resolve #31658 sooner.

@andybons
Copy link
Member

@dmitshur no status update on Gerrit's side that I've heard

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/build that referenced this issue May 28, 2020
@stamblerre
cmd/gopherbot: fix reviewer assignment for CLs imported from PRs
bdb4c1f 
golang/go#30265 causes Gerrit to add the authors of PRs as reviewers on
the imported CLs. Then, when Gopherbot assigns reviewers to the PR, it
seems a human reviewer already listed, meaning that a lot of CLs never
get assigned reviewers and are then never replied to.

As a work-around for these issues, require two reviewers on all PRs.

Fixes golang/go#31658

Change-Id: I7b3f62194450cba61493d8c4c0ad14320443e641
Reviewed-on: https://go-review.googlesource.com/c/build/+/227977
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) 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
4 participants
@bradfitz @andybons @dmitshur @gopherbot