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

maintainers: add github user IDs in service to NixOS/rfcs#39 #66361

Merged
merged 9 commits into from Aug 17, 2019

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Aug 9, 2019

Motivation for this change

Begin implementing NixOS/rfcs#39 : unprivileged maintainer team.

Step one: Track user's GitHub IDs alongside their github username (re: https://github.com/NixOS/rfcs/blob/master/rfcs/0039-unprivileged-maintainer-teams.md#changes-to-maintainersmaintainer-listnix)

This list is generated by taking the usernames in the list and looking up their ID, via https://github.com/grahamc/rfc39/blob/master/src/main.rs#L79

There is a question of have any of these usernames changed hands? Hard to verify. I can tell you these errors do occur:

Aug 09 00:05:51.420 INFO Getting ID for user, github_account: hamhut1066, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.633 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: hamhut1066, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.634 INFO Getting ID for user, github_account: siddharthist, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.693 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: siddharthist, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.694 INFO Getting ID for user, github_account: tobimpub, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.768 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: tobimpub, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.769 INFO Getting ID for user, github_account: f-breidenstein, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.840 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: f-breidenstein, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.841 INFO Getting ID for user, github_account: ma9e, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.909 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: ma9e, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.910 INFO Getting ID for user, github_account: cf6b88f, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:51.980 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: cf6b88f, exec-mode: BackfillIDs, module: rfc39:112
Aug 09 00:05:51.981 INFO Getting ID for user, github_account: nfjinjing, exec-mode: BackfillIDs, module: rfc39:100
Aug 09 00:05:52.052 WARN Error fetching ID for user, e: 404 Not Found: 'Not Found', github_account: nfjinjing, exec-mode: BackfillIDs, module: rfc39:112
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@infinisil
Copy link
Member

Aw man, really disappointed that github allows reuse of usernames. This can make user links in commit messages very misleading. This PR wouldn't be needed if GitHub didn't allow that.

@grahamc grahamc force-pushed the rfc39 branch 2 times, most recently from dbf060a to 2068c8f Compare August 9, 2019 11:54
@grahamc
Copy link
Member Author

grahamc commented Aug 9, 2019

I updated the recorded GitHub account for @langston-barrett, @moredhel, @fleaz, @furrycatherder, and @tobim by finding the commit which added them to the maintainer list, and using the new GitHub username associated with their account.

This leaves two maintainers I couldn't track down, cf6b88f and nfjinjing. I've deleted their github record since they no longer have accounts.

Edit: this eliminates all of the errors in the PR body. A new run is clean.

@FRidh
Copy link
Member

FRidh commented Aug 17, 2019

I think we should go ahead with this. There is indeed the risk that the username (and thereby the ID that @grahamc found) corresponds now to a different user, however the impact such users can have is limited to the GitHub "Read" permission level or possibly later "Triage" level. In my opinion such a risk is acceptable.

@furrycatherder
Copy link
Contributor

The change is correct in my case, at least. Thanks for taking care of this 🙂

@tobim
Copy link
Contributor

tobim commented Aug 17, 2019

It is correct in my case as well, sorry for not taking care of that.

@grahamc
Copy link
Member Author

grahamc commented Aug 17, 2019

Thanks @FRidh, I agree. I'm going to do a bit more looking in to the IDs I can't be 100% sure of, but let's plan on merging this (or a regeneration of this PR) today.

@grahamc
Copy link
Member Author

grahamc commented Aug 17, 2019

The original commit adding all the IDs was +1,118. I've deleted that commit, replacing it with one adding the github ID parts to the template only. I started to regenerate the list with only IDs I'm totally confident on, but hit the rate limit which I think means the universe is telling me to do at least 37 minutes of housework.

@FRidh
Copy link
Member

FRidh commented Aug 17, 2019

Use the graphql api, generally speaking you get more out of it :)

For each contributor's github handle, get that account name's GitHub ID.

Then, «git blame» the maintainer file and ensure GitHub agrees the commit which added the maintainer to the list was authored by the GitHub ID we've recorded.
@grahamc
Copy link
Member Author

grahamc commented Aug 17, 2019

The new commit adding GitHub IDs has 1,019 additions. I'm inclined to leave the remaining ~100 un-updated for now.

@grahamc
Copy link
Member Author

grahamc commented Aug 17, 2019

Note I've verified these names by:

  1. evaluating the maintainer.nix, and getting the position of each username in the list: NixOS/rfc39@758426b#diff-50f329b544176dd430de203797d46d2cR345
  2. running git-blame on the maintainer.nix NixOS/rfc39@758426b#diff-50f329b544176dd430de203797d46d2cR281
  3. finding the commit which modified the line the maintainer is on NixOS/rfc39@758426b#diff-50f329b544176dd430de203797d46d2cR137 (plus support for looking "behind" alphabetization and reformats)
  4. fetching the commit from github, where they'll give us the author's github ID (they track it) NixOS/rfc39@758426b#diff-50f329b544176dd430de203797d46d2cR205
  5. seeing if the author matches the github ID we have: NixOS/rfc39@758426b#diff-50f329b544176dd430de203797d46d2cR207-R221
  6. only if these match exactly, do we trust their ID is correct and have it added to the maintainer list NixOS/rfc39@758426b#diff-40582676543fe253534e493c89a2adf4R68

@grahamc grahamc merged commit cd1d034 into NixOS:master Aug 17, 2019
@grahamc grahamc deleted the rfc39 branch August 17, 2019 19:43
@infinisil infinisil mentioned this pull request Sep 9, 2019
10 tasks
@infinisil infinisil mentioned this pull request Mar 13, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants