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/internal/gophers: first email must be Gerrit email #27517

Closed
andybons opened this Issue Sep 5, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@andybons
Member

andybons commented Sep 5, 2018

x/build/cmd/devapp/owners uses the first email returned by gophers.GetPerson as the Gerrit email. Perhaps we should be more specific about this in the gophers package, but for now we should ensure that the first email is the Gerrit email. You cannot add a person to a review using their Gerrit ID email, for instance.

@gopherbot gopherbot added this to the Unreleased milestone Sep 5, 2018

@gopherbot gopherbot added the Builders label Sep 5, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 6, 2018

Member

Am I understanding this bug correctly?

p := gophers.GetPerson("@FiloSottile")
got := p.Gerrit // "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
want := "filippo@golang.org"

@andybons Do you know how many people, other than Filippo, are affected? Is it a part of resolving this issue to find that out?

I think it can be checked in bulk by iterating over all the people in gophers and seeing if the Gerrit server at https://go-review.googlesource.com recognizes the p.Gerrit email value.

Member

dmitshur commented Sep 6, 2018

Am I understanding this bug correctly?

p := gophers.GetPerson("@FiloSottile")
got := p.Gerrit // "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
want := "filippo@golang.org"

@andybons Do you know how many people, other than Filippo, are affected? Is it a part of resolving this issue to find that out?

I think it can be checked in bulk by iterating over all the people in gophers and seeing if the Gerrit server at https://go-review.googlesource.com recognizes the p.Gerrit email value.

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Sep 6, 2018

Member

yes that's correct.

as for others affected, that's part of this bug, yes.

Member

andybons commented Sep 6, 2018

yes that's correct.

as for others affected, that's part of this bug, yes.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 11, 2018

Member

You cannot add a person to a review using their Gerrit ID email, for instance.

@andybons Could you please provide more information about that. I'd like to get a better understanding of the complete high-level picture here, so I can find the most effective way to resolve the underlying issue. Answering the "What did you do? / What did you expect to see? / What did you see instead?" questions (from the perspective of x/build/cmd/devapp/owners) would be very helpful.

I want to be able to test my changes, and I might want to use Gerrit in the same way to verify whether my fix is complete.

Member

dmitshur commented Sep 11, 2018

You cannot add a person to a review using their Gerrit ID email, for instance.

@andybons Could you please provide more information about that. I'd like to get a better understanding of the complete high-level picture here, so I can find the most effective way to resolve the underlying issue. Answering the "What did you do? / What did you expect to see? / What did you see instead?" questions (from the perspective of x/build/cmd/devapp/owners) would be very helpful.

I want to be able to test my changes, and I might want to use Gerrit in the same way to verify whether my fix is complete.

@andybons

This comment has been minimized.

Show comment
Hide comment
@andybons

andybons Sep 11, 2018

Member

A Gerrit user has an email associated with their account (https://go-review.googlesource.com/settings/#Profile). When assigning a reviewer, the email used must match this (or another email that the user has entered in settings).

When I say Gerrit user ID emails, I'm referring to those that look like 6195@62eb7196-b449-3ce5-99f1-c037f21e1705. These are used internally by Gerrit in the refs/meta data model and cannot be used to assign reviews to someone. Meaning I can't send a review using git mail -r 6195@62eb7196-b449-3ce5-99f1-c037f21e1705.

To start, we should at least be treating the Gerrit user ID "emails" as definitely not elligible when specifying what someone's Gerrit email is (the one others should be using to assign a review to someone).

You could theoretically use https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html to check whether an ID is a valid one to determine which email to associate with an individual as well.

Member

andybons commented Sep 11, 2018

A Gerrit user has an email associated with their account (https://go-review.googlesource.com/settings/#Profile). When assigning a reviewer, the email used must match this (or another email that the user has entered in settings).

When I say Gerrit user ID emails, I'm referring to those that look like 6195@62eb7196-b449-3ce5-99f1-c037f21e1705. These are used internally by Gerrit in the refs/meta data model and cannot be used to assign reviews to someone. Meaning I can't send a review using git mail -r 6195@62eb7196-b449-3ce5-99f1-c037f21e1705.

To start, we should at least be treating the Gerrit user ID "emails" as definitely not elligible when specifying what someone's Gerrit email is (the one others should be using to assign a review to someone).

You could theoretically use https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html to check whether an ID is a valid one to determine which email to associate with an individual as well.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 12, 2018

Member

I understand the issue fully now. I have a fix in mind that I think will be good, and will send a CL (with a commit message that explains everything) soon, probably after issue #27282.

I also want to follow up by improving the gophers package, I filed issue #27631.

Member

dmitshur commented Sep 12, 2018

I understand the issue fully now. I have a fix in mind that I think will be good, and will send a CL (with a commit message that explains everything) soon, probably after issue #27282.

I also want to follow up by improving the gophers package, I filed issue #27631.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 14, 2018

Change https://golang.org/cl/135456 mentions this issue: internal/gophers: restore valid Gerrit emails

gopherbot commented Sep 14, 2018

Change https://golang.org/cl/135456 mentions this issue: internal/gophers: restore valid Gerrit emails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment