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

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

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented Feb 15, 2019

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 label Feb 15, 2019

@dmitshur

This comment was marked as resolved.

Copy link
Member

commented Feb 15, 2019

I thought we had a similar issue. It was #27533 and it's closed now. Is this the same problem, or subtly different?

@bradfitz

This comment was marked as resolved.

Copy link
Member Author

commented Feb 15, 2019

That was gopherbot. This is about gerritbot & Gerrit.

@andybons

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

This is due to the inability of a user to upload changes as another person. 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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

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