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

RFC-0039: unprivileged maintainer team #39

Merged
merged 4 commits into from
Mar 13, 2019
Merged

RFC-0039: unprivileged maintainer team #39

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ceb4b3
RFC-0039: unprivileged maintainer team
grahamc Jan 16, 2019
fd731f7
Addressing feedback
grahamc Jan 16, 2019
19f6d2f
Incorporate RFC Shepherd meeting feedback, and clarify that maintaine…
grahamc Feb 28, 2019
1a0f5b6
fixup: resolved question duplication
grahamc Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions rfcs/0039-unprivileged-maintainer-teams.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
---
feature: unprivileged-maintainer-teams
start-date: 2019-01-16
author: Graham Christensen <[email protected]>
co-authors: zimbatm <[email protected]>
related-issues: https://github.com/NixOS/ofborg/pull/303
---

# Summary
[summary]: #summary

Package maintainers who are not able to commit directly to Nixpkgs
don't have adequate tools to attentively maintain their package.
OfBorg requests reviews of maintainers it can identify. GitHub only
allows requesting a review of a Collaborator of the repository.

This RFC bridges that gap, and allows OfBorg to request reviews of
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear (I think the later context says so) you mean, on-github reviews of PRs, not some other review mechanism?

and so if I am a maintainer, I'll get a regular looking github review and I can hit approve/request changes just like on any other project i'm a full super-full-powers member of?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

maintainers.

# Motivation
[motivation]: #motivation

The goal of this RFC is to involve package maintainers in reviewing
pull requests against their packages. This RFC does not grant
maintainers the ability to merge pull requests against their own
package.

Maintainers take a responsibility for their package, and want to know
about updates to their package's expression. However, Nixpkgs receives
over 1,000 pull requests each month and subscribing to them all is not
a reasonable requirement to maintain a package.

The ideal outcome is package maintainership means a more active role
in reviewing and approving changes to Nixpkgs.

# Detailed design
[design]: #detailed-design

Package maintainers will be a member of a GitHub team, allowing OfBorg
to request a review.

## The Team

We will create a GitHub team under the NixOS GitHub organization
called "Nixpkgs Maintainers" which only grants "read" access to
Nixpkgs.

This team will not grant any privileges to the Nix ecosystem
repositories which non-members don't already have. They will not be able to
close other people's issues or PRs or push branches. Experimentation
and documentation shows this will only grant access to a team
discussion board on GitHub.

Being a member of this team will let the user mark themselves as a
public member of the organization. This will show the NixOS logo on
their GitHub profile, and people will see "Member" next to their
account name when browsing issues.

In order to be a member, each user will need to enable 2FA on their
GitHub account, since [the GitHub organization requires 2FA of all
members](https://github.com/NixOS/nixpkgs/issues/42761).

See
https://help.github.com/articles/permission-levels-for-an-organization/
for more information about what this will grant.

## Changes to `maintainers/maintainer-list.nix`

The existing Nixpkgs maintainer list already contains a structured
attribute set of per-maintainer details, including GitHub account
names. Automation will sync this list of GitHub handles with the
team's membership, automatically adding and removing people to/from
the team as the master branch's maintainer list changes.

GitHub handles can change from one user to another, and so we will
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is weird phrasing for what i think you mean is that a github handle doesn't, over time, uniquely identify a user and might be reused?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, if you decide to rename @benclifford to something different then anyone can take back that old name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we perhaps use user IDs instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is what this paragraph is saying we should do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for fun I tried to find out my own user-id on GitHub via UI and failed; I guess it is only available in some API query…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://api.github.com/users/7c6f434c you're #1891350 https://github.com/NixOS/rfcs/pull/39/files#diff-68b0938e7e9a24cb0cc24ac6a70950b9R78

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, indeed API-only, but at least easy to access.

change the maintainer list to include the GitHub user *ID* as well as
their handle. When syncing, the automation will validate the user ID
matches. GitHub User IDs are easily found at
`https://api.github.com/users/«username»`.

If a user ID's GitHub handle changes, the maintainer should remain
part of the team under their new handle. The user's entry in
`maintainer-list.nix` should be updated to reflect their new handle.

## Team Automation

The team must be automatically updated at least once a day to ensure
the maintainer list is fresh and up to date. The automation for this
will be written in Rust with the hubcaps library. It will run on the
NixOS infrastructure with limited credentials, with only sufficient
permission to manage the team.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does GitHub allow credentials to be limited enough to only add/remove members to a read-only team? I would have guessed it required Owner access to the organization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think «Team maintainer» for «Maintainer team» might be a bit lower; it still seems to be able to give the team write access, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think API tokens can be created specifically for modifying teams, but maybe not a specific team. I tried to make this vague enough to say "as limited as possible, but still able to do the job."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is: if the credentials given to the automated system have the ability to write to NixOS repos, then it becomes attack surface in our threat model that currently assumes the state of the GitHub NixOS repos is trusted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum authority we can grant to a token is org:admin, unfortunately. This means it can add anyone to any team.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grahamc In this case I think we should make the code/node that has access to the token separate from the rest of the code and manage its lifecycle independently, exposing just the team managemnt as a locked down interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this go into the drawbacks section? It's more or less present with “a mistake in the automation”, but the offensive security aspect (ie. people willingly trying to break it) are only suggested here.

I was personally ambivalent before this information (unsure of both the advantages and the drawbacks), so adding potential attack surface for it makes me half-heartedly opposed to the change.

Maybe an option would be to have a cron mailing nixos owners when an invitation/ejection is required, and this could work without granting automation too much rights? If there are not too frequent changes to the maintainers list, then I think this could reasonably work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is enough if in the 72h after adding the first maintainership it is rare that any packages maintained by the new maintainer receive a non-maintainer change…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be an option to run it manually, but I think it is less good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the frequency of maintainer list changes, just in case the tradeoff discussion turns to estimating the amount of effort: git log reports ~1–2 commit/day on normal months (of course, there are merge commits in the count, so I would estimate the number of additions as half that):

     50 Date:   2018-04
…
     32 Date:   2018-09
     41 Date:   2018-10
     27 Date:   2018-11
     14 Date:   2018-12


The automation will fetch a fresh version of Nixpkgs's master branch,
extract the maintainer information, and update the team. It will
support a dry-run option.

New members of the team will receive an invitation to join the GitHub
organization.

## Changes to Reviewer/Maintainer Behavior

Reviewers and maintainers should use GitHub's review tools (Approve,
Request Changes, etc.) to clearly communicate their feedback about the
pull request.

## OfBorg changes

OfBorg will identify PRs which are approved by their maintainers, and
add a special label `approved-by-maintainer`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Are there drawbacks to do it independently from the rest of the RFC?
  2. Should ofBorg also autolabel PRs authored by corresponding maintainers?
  3. Should there be special labelling for change requests from corresponding maintainers?


## Roll-Out Plan

1. Write an explanatory post on Discourse about the what-and-why of
this plan.
2. Select a small group of maintainers who are not committers to be
part of the first round, and manually run the tooling, and pause
half a week to see what changes.
3. Automate the tooling on the infrastructure.
4. Expand the group to one quarter of the maintainers, and pause a
half a week to gauge response.
5. Expand the group to one half of the maintainers and wait one week.
6. Expand the group to all of the maintainers.

If we receive no major feedback or problems during the rollout, we
will continue to 100%.

# Drawbacks
[drawbacks]: #drawbacks

- Putting each maintainer in a read only team will display
maintainers as "member", without specifying which team they are a
member of. This gives the impression of authority which maintainers
don't already receive. This is a pro and a con.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, in the Coq organization, we invite lots of external contributors (in practice to a team granting write access, not just read access, but our branches are protected, so this is just about giving them rights to triage issues), but we wish that only the core developers are shown as members of the organization. Therefore, as an administrator, I can change the visibility status of these members from Public to Private (and not the converse). This could be automated by the bot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in case of Nixpkgs it is also convenient for members to see if the person commenting is a committer or not (basically, should I say «nice, mention me when this is ready to merge») — but yes, as a partial solution it might be better than nothing.

Of course, it creates a question whether we want to enforce public visibility of membership for committers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually like if there was a way to highlight maintainers. Gives a sense of membership and responsibility and gives their reviews more weight.

However making it completely indistinguishable from committers might be a problem. That information already is hard to get for nixpkgs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, unless you're already a committer, you don't even see "Member" on people's comments unless they've manually set that to be Public, so if you want to know if the person reviewing your PR will actually be able to merge it you have to poke through git to see if they've made a merge commit before.

Copy link
Member

@Zimmi48 Zimmi48 Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there is another way to know if they have write-access: whether their approved request appears as a green tick or a gray one (you have lots of examples of gray ticks in this very PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, as an administrator, I can change the visibility status of these members from Public to Private (and not the converse). This could be automated by the bot.

This can be done, sure, however there is nothing preventing them from marking themselves publicly immediately after.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having it public would actually be a good thing. It gives people a stake in the community, and it probably puts even more impetus on us to remove bad actors from our community.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can enforce public vivisbility anyway.

(And if we could, in a tradeoff between community-building «stake and visibility» and technical «visibility who can merge» I would argue for the latter, but I do recognise that different people have different opinions here)


- A mistake in the automation, or in the admin panel of GitHub could
grant the team write access to Nix ecosystem repositories.

- Package maintainers who do not wish to have a GitHub account will
not benefit from this change.

- Package maintainers who do have a GitHub account, but do not wish
to use 2 factor authentication will not benefit from this change.

- Someone who is banned from the NixOS GitHub organization is not
allowed to be a package maintainer.

# Alternatives
[alternatives]: #alternatives

Mentioning people in GitHub comments is the main alternative. This has
the major down-side of not receiving the support of [GitHub's UI
for requested reviews](https://github.com/pulls/review-requested).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How important is the GitHub UI for requesting reviews? Addresses most of the drawbacks, and I'm not sure the UI is that valuable in comparison.

Copy link
Member

@vcunat vcunat Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating the team seems fairly cheap to me. /cc comments will create unnecessary noise every time (e.g. e-mails), similarly to what borg did before being moved to the "checks" tab.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative, not mentioned here, is to just automatically send mail to maintainers. This would require a custom program to be written, but so would the proposal here. No GitHub features required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, it'd work for people who are not using GitHub. The only drawback (compared to this RFC) would be that reviews from maintainers wouldn't show up in color, but in gray -- but if ofborg adds approved-by-maintainer anyway that's likely not an issue ; esp. as otherwise reviews from maintainers of one package would appear in color for other packages too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it realistically bearable to be an active Nixpkgs package maintainer without Github account? I don't see many patches posted to mailing list, at least.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a page listing PRs where one's review is requested without answer yet is an argument, I don't think we should choose our workflow based on hypothetical improvements GitHub could do later on. First, make the tooling, then decide the workflow accordingly.

Now, whether a page listing PRs where the review is requested and/or third-party dashboards are an advantage enough to warrant this change is another question. I personally don't use them and would assume people who maintain just one or two packages would see even less use to these, but may be wrong here :)

Copy link
Member

@7c6f434c 7c6f434c Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given:
(a) various interesting corner cases in Github behaviour we hit from time to time,
(b) our story of migrating basically everything exchanging one featureset to another one
we shouldn't really look too much at what Github intends things to mean and we can ask which set of the side-effects we like better.

(I do probably like side-effects of review requests better than of a comment, unless there is a unified comment with everything ofBorg wants to say about the PR)

Separately, I think that making maintainership mean much more (even if desirable) has time horizon larger than replacing at least one part of the tool collection; so discussing how multiple tools (Github, ofborg, this team maintenance bot) interact can assume nothing big changes in the process for low-impact packages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we shouldn't bank on GitHub making big changes. I do think the existing tooling is very good already. I think changing behavior is much faster than you say -- small nudges have far reduced direct pushes within a short period of time of having ofborg do reasonable checks and validation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that works for tooling adoption — and high standards you uphold for ofborg help — better than for shifting general notions (especially in the direction that creates more single-person-of-delay situations).

Anyway, I agree that in the current situation review requests make the most sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having thought about this more, this isn’t actually mutually exclusive with an option that did not depend on GitHub, especially since implementing an email-based system would require no priveleges whatsoever, and could be done as an opt-in system by any community member. While I’m among the (probable) minority of community members who would like Nixpkgs development to move away from GitHub, this doesn’t actually increase our lock-in at all, and would be strictly better than what we have now.



# Resolved questions
[resolved]: #resolved-questions

- Is it possible for the automation to spam a user who doesn't want
to be part of the team with invitations?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would work better as an opt-in process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's reasonable to expect people who sign up to maintain packages to receive notifications about proposed changes to those packages. That's really all the maintainer role is for, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unresolved question is really will github let us spam a user with invitations, or will it protect the user from an automatic process trying to be a nuisance. Also, yes, I think it is reasonable to say people who are maintainers should get pings for their packages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reasonable amount of state should prevent this spamming from happenning on our side, so maybe this is not too important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyssais the maintainers may think bothering with 2fa isn't worth the benefits. In that case they may still do their maintainer duties but shouldn't be spammed by invitations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any spamming of invitations is a bug, probably on both GitHub's side, and definitely on our side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that GitHub allows spamming with review requests for same changes, I would say we cannot expect any extra line of defense…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One potential side effect of "spamming" could be that people remove their maintainership to avoid the notifications. I'd consider this probably a good thing, as it promotes "freshness" of maintainership info and packages actually being maintained.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spamming in my comment meant asking me to review the exact same commit I had previously approved (I evaluated it faster than ofborg, I guess). Spamming in this thread is about resending team invitation before the person in question reacts.

One mention per actual change is not spamming and makes sense etc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A certain amount of races will be inevitable. We should, of course, try to minimize time windows where eager human action is not detected.

No.

# Unresolved questions
[unresolved]: #unresolved-questions

This comment was marked as resolved.

Sign in to view


- Do maintainers want to be part of this team?
- Will the requirement of 2FA cause a significant number of people to
not want to participate?
- How will we handle people who have been invited, but have not
accepted the invitation?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, there are committers, maintainers in the team, maintainers not on GitHub, and maintainers who have received but not accepted the invitation. If such lists are kept and re-synchronised, for the people with invitation sent the bot should simply wait for acceptance? If someone rejects and then changes the mind, we can probably handle it manually.


# Future work
[future]: #future-work

- Writing the automation program.
- Adding UIDs to every maintainer.
- Creating the GitHub team
- Updating the NixOS Org Configurations repository to run the
automation with credentials on an automated basis.

# Future Potential RFCs
The following topics are explictly _not_ part of this RFC.

- Allowing maintainers to merge pull requests against their packages
without having commit access.
- Requiring all maintainers to have a GitHub account with 2FA.