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/internal/gophers: improve internal package design #27631

Open
dmitshur opened this issue Sep 12, 2018 · 5 comments
Open

x/build/internal/gophers: improve internal package design #27631

dmitshur opened this issue Sep 12, 2018 · 5 comments
Assignees
@dmitshur
Labels
Builders x/build issues (builders, bots, dashboards) Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 12, 2018 •
edited

Problem

Total mess, but a functional mess, and a starting point for the future.
— Commit 891b12dc

The gophers package is currently hard to use and hard to modify. It's not easy to read its documentation and start using it:

// (no documentation)
func GetPerson(id string) *Person

I've used and modified it multiple times, and each time, I had to read its internal code to figure out:

  • what kind of value can "id" be?
  • what is its exact format?
    • is leading '@' required for GH usernames? optional? unneeded?
  • is it case sensitive or not?
  • in what order/what type of information to add to the addPerson(...) lines?

Despite being an internal package, gophers is an important package providing value to 4 other packages, and potentially becoming used in more places. It's no longer just for computing stats, but also for tracking package owners and assigning reviews. Being internal means we can change it easily (even break the API if needed) if we come to agreement on an improved design.

Proposed Solution

I think it can be made easier to use by:

  • documenting it (so its godoc is all you need to use it, no need to read code)

    For example:

    // GetPerson looks up a person by id and returns one if found, or nil otherwise.
//
// The id is case insensitive, and may be one of:
// 	- full name ("Brad Fitzpatrick")
// 	- GitHub username ("@bradfitz")
// 	- Gerrit <account ID>@<instance ID> ("5065@62eb7196-b449-3ce5-99f1-c037f21e1705")
// 	- email ("bradfitz@golang.org")
func GetPerson(id string) *Person

    @bradfitz If you prefer not to be used as an example, let me know, and we can use someone else (I'm happy to volunteer) or use a generic name. But I think a well known real user makes for a better example.

Made easier to modify by:

  • making its internal addPerson logic more explicit rather than implicit

    For example, instead of what we have now:

    addPerson("Filippo Valsorda", "", "6195@62eb7196-b449-3ce5-99f1-c037f21e1705")
addPerson("Filippo Valsorda", "filippo@cloudflare.com")
addPerson("Filippo Valsorda", "filippo@golang.org", "11715@62eb7196-b449-3ce5-99f1-c037f21e1705")
addPerson("Filippo Valsorda", "filippo@golang.org", "hi@filippo.io", "@FiloSottile")

// what kind of changes should be done to modify the end result Person struct?

    It could be something more explicit, along the lines of:

    add(Person{
	Name:      "Filippo Valsorda",
	GitHub:    "FiloSottile",
	Gerrit:    "filippo@golang.org",
	GerritIDs: []int{6195, 11715}, // Gerrit account IDs.
	GitEmails: []string{
		"filippo@golang.org",
		"filippo@cloudflare.com",
		"hi@filippo.io",
	},
	gomote: "valsorda", // Gomote user.
})

    The intention is to make it easy for people to manually add and modify their entries, with predictable results, while still being able to to use code generation (ala gopherstats -mode=find-gerrit-gophers) to add missing entries.

This is just a quick draft proposal, not necessarily the final API design. If the general direction is well received but there are concerns or improvement suggestions, I'm happy to flesh it out and incorporate feedback. I wouldn't send a CL until I have a solid design.

/cc @bradfitz @andybons
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 12, 2018
@gopherbot gopherbot added this to the Unreleased milestone Sep 12, 2018
@dmitshur dmitshur mentioned this issue Sep 12, 2018
Closed
@andybons
Copy link
Member

Given that this API is internal and you're only altering internals, I wouldn't spend a crazy amount of time worrying about implementation details. Do what you think is best for now and you can change it later on.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 13, 2018
@dmitshur dmitshur self-assigned this Sep 13, 2018
@gopherbot
Copy link

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

gopherbot pushed a commit to golang/build that referenced this issue Sep 14, 2018
@dmitshur
internal/gophers: restore valid Gerrit emails
1d8c354 
CL 132995 made a lot of changes to the gophers table based on
the output of gopherstats -mode=find-gerrit-gophers, which is
a new cmd/gopherstats mode added in that very CL.

The current implementation of Person.mergeIDs makes it so that
the first email seen is considered the Gerrit email.

Many of the additions of CL 132995 did not take that into account,
causing the Gerrit field to change for some gophers incorrectly.

Overall, that CL caused the following changes to gophers:

	Github field changes:
	tal@whatexit.org: GitHub="" -> "TomOnTime"
	Carl Henrik Lunde: GitHub="" -> "chlunde"
	Dieter Plaetinck: GitHub="" -> "Dieterbe"
	Diogo Pinela: GitHub="" -> "dpinela"
	Frank Schroeder: GitHub="" -> "magiconair"
	Gregory Man: GitHub="" -> "gregory-m"
	Jan Berktold: GitHub="" -> "JanBerktold"
	Jean de Klerk: GitHub="" -> "jadekler"
	Josselin Costanzi: GitHub="" -> "josselin-c"
	Martin Garton: GitHub="MartinGarton" -> "mjgarton"
	Matt Harden: GitHub="" -> "nerdatmath"
	Michael Darakananda: GitHub="" -> "pongad"
	Mostyn Bramley-Moore: GitHub="" -> "mostynb"
	Nicholas Maniscalco: GitHub="" -> "nicholasmaniscalco"
	Roland Illig: GitHub="" -> "rillig"
	Yasha Bubnov: GitHub="" -> "ybubnov"
	Zheng Xu: GitHub="" -> "Zheng-Xu"
	ltnwgl: GitHub="" -> "gengliangwang"
	oiooj: GitHub="" -> "oiooj"
	thoeni: GitHub="" -> "thoeni"

	Gerrit field changes:
	Andrew Bonventre: Gerrit="andybons@golang.org" -> "365204+andybons@users.noreply.github.com"
	Carl Mastrangelo: Gerrit="notcarl@google.com" -> "carl.mastrangelo@gmail.com"
	Chris McGee: Gerrit="sirnewton_01@yahoo.ca" -> "newton688@gmail.com"
	Eric Lagergren: Gerrit="ericscottlagergren@gmail.com" -> "eric@ericlagergren.com"
	Filippo Valsorda: Gerrit="filippo@golang.org" -> "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
	Guillaume J. Charmes: Gerrit="guillaume@charmes.net" -> "gcharmes@magicleap.com"
	Harshavardhana: Gerrit="hrshvardhana@gmail.com" -> "harsha@minio.io"
	Jean de Klerk: Gerrit="jadekler@gmail.com" -> "deklerk@google.com"
	Joe Tsai: Gerrit="joetsai@google.com" -> "joetsai@digital-static.net"
	Martin Möhrmann: Gerrit="moehrmann@google.com" -> "martisch@uos.de"
	Matthew Dempsky: Gerrit="mdempsky@google.com" -> "matthew@dempsky.org"
	Olivier Poitrey: Gerrit="rs@dailymotion.com" -> "rs@netflix.com"
	Paul Jolly: Gerrit="paul@myitcv.org.uk" -> "paul@myitcv.io"
	Ralph Corderoy: Gerrit="ralph@inputplus.co.uk" -> "ralph.corderoy@gmail.com"
	Raul Silvera: Gerrit="rsilvera@google.com" -> "rauls5382@gmail.com"
	Richard Miller: Gerrit="miller.research@gmail.com" -> "millerresearch@gmail.com"
	Sebastien Binet: Gerrit="seb.binet@gmail.com" -> "binet@cern.ch"
	Tobias Klauser: Gerrit="tobias.klauser@gmail.com" -> "tklauser@distanz.ch"
	Vitor De Mario: Gerrit="vitordemario@gmail.com" -> "vitor.demario@mendelics.com.br"

	Googler field changes:
	Aaron Kemp: Googler=false -> true
	Jason Hall: Googler=false -> true
	Jean de Klerk: Googler=false -> true

	(It also caused many emails to be added, but I'm not considering
	those changes since they're not relevant to golang/go#27517 and
	aren't causing harm.)

All of the Github and Googler field changes are good,
but not all of the Gerrit field changes are good.
I've manually checked them against the
go-review.googlesource.com Gerrit server,
and classified them as follows:

	bad  Andrew Bonventre: Gerrit="andybons@golang.org" -> "365204+andybons@users.noreply.github.com"
	bad  Carl Mastrangelo: Gerrit="notcarl@google.com" -> "carl.mastrangelo@gmail.com"
	ok   Chris McGee: Gerrit="sirnewton_01@yahoo.ca" -> "newton688@gmail.com"
	bad  Eric Lagergren: Gerrit="ericscottlagergren@gmail.com" -> "eric@ericlagergren.com"
	bad  Filippo Valsorda: Gerrit="filippo@golang.org" -> "6195@62eb7196-b449-3ce5-99f1-c037f21e1705"
	bad  Guillaume J. Charmes: Gerrit="guillaume@charmes.net" -> "gcharmes@magicleap.com"
	bad  Harshavardhana: Gerrit="hrshvardhana@gmail.com" -> "harsha@minio.io"
	ok   Jean de Klerk: Gerrit="jadekler@gmail.com" -> "deklerk@google.com"
	bad  Joe Tsai: Gerrit="joetsai@google.com" -> "joetsai@digital-static.net"
	bad  Martin Möhrmann: Gerrit="moehrmann@google.com" -> "martisch@uos.de"
	bad  Matthew Dempsky: Gerrit="mdempsky@google.com" -> "matthew@dempsky.org"
	ok   Olivier Poitrey: Gerrit="rs@dailymotion.com" -> "rs@netflix.com"
	bad  Paul Jolly: Gerrit="paul@myitcv.org.uk" -> "paul@myitcv.io"
	bad  Ralph Corderoy: Gerrit="ralph@inputplus.co.uk" -> "ralph.corderoy@gmail.com"
	bad  Raul Silvera: Gerrit="rsilvera@google.com" -> "rauls5382@gmail.com"
	ok   Richard Miller: Gerrit="miller.research@gmail.com" -> "millerresearch@gmail.com"
	bad  Sebastien Binet: Gerrit="seb.binet@gmail.com" -> "binet@cern.ch"
	bad  Tobias Klauser: Gerrit="tobias.klauser@gmail.com" -> "tklauser@distanz.ch"
	bad  Vitor De Mario: Gerrit="vitordemario@gmail.com" -> "vitor.demario@mendelics.com.br"

I considered any @google.com -> non-@google.com changes as bad.
For the rest, it was based on which email was recognized by the
Gerrit server and had more activity overall, as well as recently.

This CL undoes all the bad Gerrit field changes, reverting them
to their original pre-CL 132995 values. It also changes the Gerrit
email for Gopherbot, and cleans up my own entry. That leaves just:

	Gerrit field changes:
	Chris McGee: Gerrit="sirnewton_01@yahoo.ca" -> "newton688@gmail.com"
	Jean de Klerk: Gerrit="jadekler@gmail.com" -> "deklerk@google.com"
	Olivier Poitrey: Gerrit="rs@dailymotion.com" -> "rs@netflix.com"
	Richard Miller: Gerrit="miller.research@gmail.com" -> "millerresearch@gmail.com"

In future CLs, we'll need to be careful with the order in which
emails are added, until golang/go#27631 is resolved.

Updates golang/go#27631.
Fixes golang/go#27517.

Change-Id: I6bd289af6ea2c50c293c4576de3873658994b98a
Reviewed-on: https://go-review.googlesource.com/135456
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/195062 mentions this issue: internal/gophers: restore valid Gerrit emails (again)

@gopherbot
Copy link

Change https://golang.org/cl/195064 mentions this issue: internal/gophers: add missing documentation

gopherbot pushed a commit to golang/build that referenced this issue Sep 12, 2019
@dmitshur
internal/gophers: add missing documentation
09e36d8 
This is a step towards improving the usability of this package.

Use myself as an example of the types of queries that GetPerson
accepts.

Updates golang/go#27631

Change-Id: I3f4d497eb237958c652e51e03fd2459f0f9df8ef
Reviewed-on: https://go-review.googlesource.com/c/build/+/195064
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Sep 18, 2019
@dmitshur
internal/gophers: restore valid Gerrit emails (again)
e5813e3 
This is a cherry-pick of CL 135456 that restores Gerrit emails
for some people that were incorrectly changed in CL 165639, with
manual no-op addPerson line merges to address code review comments.

The cherry-pick applied very cleanly with just two minor merge
conflicts: one due to a Filippo's email already being fixed in
CL 176037, and another due to a close new entry.

Add tests to catch this from happening again, until the source
of the problem is resolved in issue golang/go#34259.

Updates golang/go#34259
Updates golang/go#28320
Updates golang/go#31919
Updates golang/go#27517
Updates golang/go#27631

Change-Id: Ia03a2b94403334d3f571ac5623e12d3bfd6f1e4f
Reviewed-on: https://go-review.googlesource.com/c/build/+/195062
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@dmitshur @codebien
internal/gophers: add missing documentation
7cba1b2 
This is a step towards improving the usability of this package.

Use myself as an example of the types of queries that GetPerson
accepts.

Updates golang/go#27631

Change-Id: I3f4d497eb237958c652e51e03fd2459f0f9df8ef
Reviewed-on: https://go-review.googlesource.com/c/build/+/195064
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@dmitshur @codebien
internal/gophers: restore valid Gerrit emails (again)
6910d1e 
This is a cherry-pick of CL 135456 that restores Gerrit emails
for some people that were incorrectly changed in CL 165639, with
manual no-op addPerson line merges to address code review comments.

The cherry-pick applied very cleanly with just two minor merge
conflicts: one due to a Filippo's email already being fixed in
CL 176037, and another due to a close new entry.

Add tests to catch this from happening again, until the source
of the problem is resolved in issue golang/go#34259.

Updates golang/go#34259
Updates golang/go#28320
Updates golang/go#31919
Updates golang/go#27517
Updates golang/go#27631

Change-Id: Ia03a2b94403334d3f571ac5623e12d3bfd6f1e4f
Reviewed-on: https://go-review.googlesource.com/c/build/+/195062
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@dmitshur dmitshur changed the title x/build/internal/gophers: improve documentation, internal package design x/build/internal/gophers: improve internal package design Dec 10, 2019
@gopherbot
Copy link

Change https://go.dev/cl/386794 mentions this issue: internal/gophers: remove unused Gerrit group 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmitshur dmitshur

Labels
Builders x/build issues (builders, bots, dashboards) Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

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