RFC-0039: unprivileged maintainer team #39
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| --- | ||
| 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 | ||
| 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 list changes. | ||
|
|
||
| GitHub handles can change from one user to another, and so we will | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. yes, if you decide to rename @benclifford to something different then anyone can take back that old name There was a problem hiding this comment. Should we perhaps use user IDs instead? There was a problem hiding this comment. I think that is what this paragraph is saying we should do? There was a problem hiding this comment. 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… There was a problem hiding this comment. There was a problem hiding this comment. 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. | ||
|
There was a problem hiding this comment. Does GitHub allow credentials to be limited enough to only add/remove members to a read-only team? I would have guessed it required There was a problem hiding this comment. 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. There was a problem hiding this comment. 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." There was a problem hiding this comment. 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. There was a problem hiding this comment. The minimum authority we can grant to a token is org:admin, unfortunately. This means it can add anyone to any team. There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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… There was a problem hiding this comment. I think it would be an option to run it manually, but I think it is less good. There was a problem hiding this comment. 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): |
||
|
|
||
| The automation will fetch a fresh version of Nixpkgs, extract the | ||
| maintainer information, and update the team. It will support a dry-run | ||
| option. | ||
|
There was a problem hiding this comment. Should membership be based on just master or also other branches like the active release? There was a problem hiding this comment. a great question! It should probably be the sum of all the maintainers in the branches we release channel updates for: https://github.com/NixOS/nixos-org-configurations/blob/33967236a51f41fe4af1d1a321b505f61ed4f246/nixos-org/hydra-mirror.nix#L44-L57 any other release-branches should probably no longer accept PRs. However, the new maintainer format was introduced in 18.03, so I don't think we can include 17.09. Additionally, the old branches should receive a patch including the user IDs of each maintainer. This is also pretty complicated if a user changes their name, hmmm... There was a problem hiding this comment. Maybe ping based on the branch that the PR is against? There was a problem hiding this comment. OfBorg already does that, I think the question is what branches does the team's member list comprise of? There was a problem hiding this comment. Realistically I would say just master is fine. If someone removed themselves as a maintainers since the last release, they probably won't be willing to review anyways. There was a problem hiding this comment. I can't think of a good argument to query multiple branches, just asking in case somebody does. |
||
|
|
||
| New members of the team will receive an invitation to join the GitHub | ||
| organization. | ||
|
|
||
| ## OfBorg changes | ||
|
|
||
| OfBorg will identify PRs which are approved by their maintainers, and | ||
| add a special label `approved-by-maintainer`. | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| ## 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. | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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). There was a problem hiding this comment.
This can be done, sure, however there is nothing preventing them from marking themselves publicly immediately after. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. | ||
|
|
||
| - If a GitHub user is a real jerk on the internet, it can potentially | ||
|
There was a problem hiding this comment. Do we want to phrase it this way as opposed to «non-constructively aggressive in project discussions» or something? Both from the point of view of not using vague insults, and to put the focus on interactions where the maintainer status actually comes up. There was a problem hiding this comment. It could also be relevant in other places on GitHub, since one could display the NixOS badge under their name There was a problem hiding this comment. Not sure I have ever seen it done with NixOS badge, even where it would be constructive and relevant (as we are not writing the rules for banning for behaviour while pretending to represent NixOS organisation, I think focus should be on what happens usually) There was a problem hiding this comment. I'd like to keep this case vague, as it isn't intending to side-load a code of behavior. The first time I brought this up people were concerned with what if a real jerk is part a maintainer, and I think the general solution is we'll handle it however we handle it, and them being a member of this read-only team doesn't really change anything. |
||
| reflect negatively on the NixOS ecosystem. Someone who is banned | ||
| from the NixOS GitHub organization is not allowed to be a package | ||
| maintainer. | ||
|
There was a problem hiding this comment.
i don't understand quite what this means? If i'm a jerk, you will ban me from the github organisation and then I will have to maintain my packages in the same way as: "Package maintainers who do not wish to have a GitHub account will not benefit from this change." - i.e. jerks will not benefit from this but will not otherwise be harmed? There was a problem hiding this comment. Well, they will also not be able to participate in GitHub discussions, and submit PRs (which are the main way of non-committer maintainers updating their packages) |
||
|
|
||
| # 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). | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 :) There was a problem hiding this comment. I think given: (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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
|
|
||
|
|
||
| # Unresolved questions | ||
| [unresolved]: #unresolved-questions | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
|
||
| - Is it possible for the automation to spam a user who doesn't want | ||
| to be part of the team with invitations? | ||
|
There was a problem hiding this comment. I think this would work better as an opt-in process. There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. There was a problem hiding this comment. A reasonable amount of state should prevent this spamming from happenning on our side, so maybe this is not too important. There was a problem hiding this comment. @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. There was a problem hiding this comment. Any spamming of invitations is a bug, probably on both GitHub's side, and definitely on our side. There was a problem hiding this comment. Given that GitHub allows spamming with review requests for same changes, I would say we cannot expect any extra line of defense… There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. A certain amount of races will be inevitable. We should, of course, try to minimize time windows where eager human action is not detected. |
||
| - 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? | ||
|
There was a problem hiding this comment. 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. | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.