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: opt-out changes from Gerrit approvers list from automatically adding reviewers #34362

Open
mdempsky opened this issue Sep 18, 2019 · 15 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 18, 2019

I'd rather gopherbot stopped automatically adding reviewers to my CLs.

I appreciate the intent, and I think it's a great idea for people who are new to contributing to Go. But I'm comfortable using Gerrit and finding reviewers, and I don't want my reviewers confused because Gopherbot asked them to look at my CLs out of order.

@gopherbot gopherbot added this to the Unreleased milestone Sep 18, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 18, 2019

One workaround is to use git mail -r mdempsky. Or whichever reviewers you want to specify.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 18, 2019

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Sep 18, 2019

One workaround is to use git mail -r mdempsky.

To clarify, the workaround you're suggesting here is to use the -r flag to specify reviewers at upload time, or to add myself as the reviewer specifically? (I only use git-codereview for the commit hooks; I'm not familiar with its workflow for uploading patches.)

Or whichever reviewers you want to specify.

I usually prefer to wait until I've confirmed that my CL passes trybots before sending them to reviewers.

Also, part of the problem is when I upload a patch series, I want to mail out the patches in order and at a pace that I feel confident my reviewers can keep up with. I don't want to mail them all out right away.

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 18, 2019

I usually prefer to wait until I've confirmed that my CL passes trybots before sending them to reviewers.

Have you considered marking the CL as a WIP until it's ready for review? I don't think git-codereview mail has such a flag, but we could add it. That would also clearly signal to humans that you're still testing.

@thanm
Copy link
Member

@thanm thanm commented Sep 18, 2019

At the moment with gerrit if you mark a change WIP, you can't kick off the trybots (instead of the usual "reply" button you get a "start review" button). Example:

Screenshot from 2019-09-18 09-26-34

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2019

@mvdan @thanm: note that GopherBot already understands DO NOT REVIEW in commit messages, and should not assign reviewers to a change whose commit message includes that string.

https://github.com/golang/build/blob/0d7c512d74a9ba24ab5599489480266084f0acc8/cmd/gopherbot/gopherbot.go#L1735

@andybons
Copy link
Member

@andybons andybons commented Sep 18, 2019

Stepping back, are we actually getting value out of the auto-assignment of CLs?

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 18, 2019

@andybons I do think there's some value in making sure that CLs don't go unnoticed in between all the activity. But that only applies to CLs that go unnoticed to begin with. Maybe gopherbot could simply wait 48h to assign reviewers if there are none; that's plenty of time for an author to run the tests, or assign reviewers, or whatever they decide to do.

@andybons
Copy link
Member

@andybons andybons commented Sep 18, 2019

@mvdan assigning after 48hrs automatically could create confusion (do we add messaging for every change letting people know? Only to new contributors?).

I'd rather us move to a pull model where one could find owners via a Gerrit UI plugin and assign themselves. It would be straightforward to implement as dev.golang.org/owners supports CORS requests.

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2019

I do think it's important that the bot give contributors some idea of how to proceed with their CL and when to expect that it will be reviewed. (I know I'm pretty terrible at keeping up with cmd/go reviews.)

I don't feel strongly about whether that should take the form of auto-assignment or a nudge toward some self-service page or tool.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 18, 2019

What about instead removing it for all google.com and golang.org users?

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 18, 2019

(to slightly amend Brad's idea, I think using the approvers list would be slightly better, so that non-googlers can also benefit)

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 18, 2019

Oh, what @mvdan says would be even better.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 18, 2019

Change https://golang.org/cl/196138 mentions this issue: cmd/gopherbot: add opt-out from automatic reviewers

gopherbot pushed a commit to golang/build that referenced this issue Sep 18, 2019
Updates golang/go#34362.

Change-Id: I2a1861d7ebb903d0231a7670597467b02d53ac96
Reviewed-on: https://go-review.googlesource.com/c/build/+/196138
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@andybons andybons changed the title x/build/cmd/gopherbot: opt-out from automatically adding reviewers x/build/cmd/gopherbot: opt-out changes from Gerrit approvers list from automatically adding reviewers Sep 18, 2019
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Sep 18, 2019

Opting-out for all approvers sounds reasonable to me. I just didn't want to be presumptive about whether others feel the same as I do about Gopherbot automatically adding reviewers to my CLs. (E.g., I know that as someone who pushes CLs directly to Gerrit using git push rather than any git-codereview-based workflow already makes me an atypical go-review.googlesource.com user.)

codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
Updates golang/go#34362.

Change-Id: I2a1861d7ebb903d0231a7670597467b02d53ac96
Reviewed-on: https://go-review.googlesource.com/c/build/+/196138
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants