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.

Sign up for GitHub

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 fails to assign reviewers on x/tools CLs (e.g. CL 374434) #50723

Closed
dmitshur opened this issue Jan 20, 2022 · 4 comments
Closed

x/build/cmd/gopherbot: assignReviewersToCLs fails to assign reviewers on x/tools CLs (e.g. CL 374434) #50723

dmitshur opened this issue Jan 20, 2022 · 4 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release Team
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jan 20, 2022 •
edited

From gopherbot logs:

2022/01/20 19:15:16 No reviewers or cc: https://go-review.googlesource.com/c/tools/+/374434
2022/01/20 19:15:16 Setting review on https://go-review.googlesource.com/c/tools/+/374434: {Message: Labels:map[] Tag: Comments:map[] Reviewers:[{Reviewer: State:}]}
2022/01/20 19:15:16 Could not set review for change "tools~374434": HTTP status 400 Bad Request; )]}'
{"reviewers":{"":{"input":"","error":"Account \u0027\u0027 is ambiguous (at most 3 shown):\n54144: Alexandre Perrin \u003calex.perrin@isovalent.com\u003e\n54148: Jamie Parrish \u003cjparrishsc@gmail.com\u003e\n54153: Nooras Saba‎ \u003csaba@golang.org\u003e\n does not identify a registered user or group"}},"error":"error adding reviewer"}

Something's went wrong there. It seems it tried to set the reviewer identified by the empty string. Gerrit reports that the empty string is an ambiguous specification of a reviewer.

The logic for adding reviewers in this task looks like this:

// Assign reviewers.
var review gerrit.ReviewInput
for _, owner := range merged.Primary {
	review.Reviewers = append(review.Reviewers, gerrit.ReviewerInput{Reviewer: owner.GerritEmail})
}
for _, owner := range merged.Secondary {
	review.Reviewers = append(review.Reviewers, gerrit.ReviewerInput{Reviewer: owner.GerritEmail, State: "CC"})
}

It seems owner.GerritEmail must be the empty string.

In x/build/internal/gophers there's an entry:

addPerson("Tools Team", "@golang/tools-team", "e37ba6c9c17684849faf885129b25ef8944419e9")

It's possible the change made in CL 247240 is connected, since it results in the Gerrit field becoming an empty string:

package main

import (
	"fmt"

	"golang.org/x/build/internal/gophers"
)

func main() {
	fmt.Printf("Gerrit = %q\n", gophers.GetPerson("@golang/tools-team").Gerrit)

	// Output now:
	// Gerrit = ""
	// 
	// Output before CL 247240:
	// Gerrit = "1080@62eb7196-b449-3ce5-99f1-c037f21e1705"
}

(Prior to that change, the Gerrit value for that team entry was "1080@62eb7196-b449-3ce5-99f1-c037f21e1705", which matched the "group_id@gerrit_instance" format.)

tools-team Gerrit group id source
[
	{
		"url": "#/admin/groups/uuid-e37ba6c9c17684849faf885129b25ef8944419e9",
		"options": {
			"visible_to_all": true
		},
		"description": "Members of the Go tools team. Publicly visible group so it can be cc\u0027d and assigned CLs. Not an ACL group.",
		"group_id": 1080,
		"owner": "tools-team",
		"owner_id": "e37ba6c9c17684849faf885129b25ef8944419e9",
		"created_on": "2020-04-24 21:02:33.000000000",
		"id": "e37ba6c9c17684849faf885129b25ef8944419e9",
		"name": "tools-team"
	}
]

(Source is https://go-review.googlesource.com/groups/?query=inname:tools-team via https://gerrit-review.googlesource.com/Documentation/rest-api-groups.html#query-groups.)

CC @hyangah.
@dmitshur dmitshur added 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. labels Jan 20, 2022
@dmitshur dmitshur added this to the Unreleased milestone Jan 20, 2022
@dmitshur dmitshur changed the title x/build/cmd/gopherbot: assignReviewersToCLs fails to assign reviewers on CL 374434 x/build/cmd/gopherbot: assignReviewersToCLs fails to assign reviewers on x/tools CLs (e.g. CL 374434) Jan 20, 2022
@gopherbot
Copy link

Change https://go.dev/cl/386754 mentions this issue: cmd/gopherbot: drop non-Gerrit owners for review assignment

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 18, 2022
gopherbot pushed a commit to golang/build that referenced this issue Feb 18, 2022
@prattmic
cmd/gopherbot: drop non-Gerrit owners for review assignment
b8f574f 
Owners may include users that do not have an associated Gerrit account,
notably GitHub Teams. When determining reviewers to add a CL, drop all
these users from the potential reviewer set. Otherwise Gerrit chokes on
attempting to set reviewer to "".

Dropping these users from owners may result in an empty primary reviewer
set (e.g., on packages where a GitHub team is the only primary owner).
In this case, upgrade the secondary owners to primary so they end up as
reviewers rather than just CC'd. If both lists are empty, a 'no-owners'
tag is added to the CL by assignReviewersToCLs.

For golang/go#50723.
For golang/go#28320.

Change-Id: I99ac5426e080bf32c46817c09debeac507282e01
Reviewed-on: https://go-review.googlesource.com/c/build/+/386754
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@prattmic
Copy link
Member

https://go.dev/cl/386754 should fix the symptom in the title: "assignReviewersToCLs fails to assign reviewers on x/tools CLs".

But the problem that @dmitshur describes above with Gerrit group UUIDs not resulting in an owner GerritEmail still exists, so leaving this open.

@gopherbot
Copy link

Change https://go.dev/cl/386794 mentions this issue: internal/gophers: remove unused Gerrit UUID

gopherbot pushed a commit to golang/build that referenced this issue Feb 18, 2022
@dmitshur
internal/gophers: remove unused Gerrit group UUID
e7fda18 
Prior to CL 247240, the addPerson line for Tools Team included:

	"1080@62eb7196-b449-3ce5-99f1-c037f21e1705"

This was a "<account ID>@<instance ID>" format that isn't used much,
but is documented as one of the formats that gophers.GetPerson accepts.
It was interpreted as an email because of the '@' in the middle.

That CL meant to replace the email with the UUID of a Gerrit group,
but there's no explicit support for such entries, and it currently
gets misinterpreted as the Tools Team's name.

We're not yet sure how to best handle teams on the Gerrit side,
but remove the UUID for now since it's neither supported nor used,
to reduce confusion.

Add test cases to help future team entry changes have the intended
effect.

For golang/go#50723.
For golang/go#27631.

Change-Id: Ice408033cd48fab746a395dc7a49f1dafe14fc55
Reviewed-on: https://go-review.googlesource.com/c/build/+/386794
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
@dmitshur
Copy link
Contributor Author

I filed this issue primarily to share my findings on a problem that was happening previously. I believe CL 386754 and CL 386794 largely resolved that problem, and there isn't anything very actionable left to do here, so I'll close it now. Follow-up work to improve reviewer assignment to teams in Gerrit will be better tracked in new issues.

Thanks to @hyangah and @prattmic for helping with this issue.

@cagedmantis cagedmantis added this to Done in Go Release Team Mar 1, 2022
@golang golang locked and limited conversation to collaborators Feb 19, 2023
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) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release
Archived in project
Go Release Team
Done
Milestone
Unreleased
Development

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