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

RFC-0039: unprivileged maintainer team #39

Merged
merged 4 commits into from
Mar 13, 2019

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Jan 16, 2019

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.


Todo:

  • add GitHub IDs to the maintainer list (maintainers: add github user IDs in service to NixOS/rfcs#39 nixpkgs#66361)
  • Create the GitHub team: https://github.com/orgs/NixOS/teams/nixpkgs-maintainers
  • Create a tool to update a team's members based on the maintainer list
  • Write an explanatory post on Discourse about the what-and-why of this plan.
  • 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.
  • Automate the tooling on the infrastructure.
  • Expand the group to one quarter of the maintainers, and pause a half a week to gauge response.
  • Expand the group to one half of the maintainers and wait one week.
  • Expand the group to all of the maintainers.

Rendered: https://github.com/NixOS/rfcs/pull/39/files?short_path=68b0938#diff-68b0938e7e9a24cb0cc24ac6a70950b9

@grahamc grahamc force-pushed the unprivileged-maintainer-team branch from 38a356f to 1ceb4b3 Compare January 16, 2019 17:25
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 as the
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove people automatically or by request?

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 automatically. If they are removed from the maintainer list, they either are being forced out, or have requested removal.

Copy link
Member

Choose a reason for hiding this comment

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

I realize there are infrastructure difficulties preventing this, but ignoring those:

ofBorg could aggregate all its information into one comment. So after its done with eval and builds, it posts a comment like this:

I have successfully build on linux and darwin. Aarch64 was not attempted because it is marked as unsupported.
@MaintainerXYZ, you are listed as the maintainer for this package. Please review this change.

That way the "spam" is contained. In fact I would prefer it to no message at all. It also scales well for future ofBorg improvements. It could also add messages like

!Warning!: I have determined that this change will result in more than 500 rebuilds on linux but is targeted to the master branch.

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 don't think this is on topic for this RFC.


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

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

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.

- 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

It could also be relevant in other places on GitHub, since one could display the NixOS badge under their name

Copy link
Member

@7c6f434c 7c6f434c Jan 16, 2019

Choose a reason for hiding this comment

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

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)

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'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.

rfcs/0039-unprivileged-maintainer-teams.md Outdated Show resolved Hide resolved
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

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.

rfcs/0039-unprivileged-maintainer-teams.md Outdated Show resolved Hide resolved
team's membership, automatically adding and removing people as the
list changes.

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

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.

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.

rfcs/0039-unprivileged-maintainer-teams.md Outdated Show resolved Hide resolved
- If a GitHub user is a real jerk on the internet, it can potentially
reflect negatively on the NixOS ecosystem. Someone who is banned
from the NixOS GitHub organization is not allowed to be a package
maintainer.

Choose a reason for hiding this comment

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

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

[unresolved]: #unresolved-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.

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.

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

- 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.


The automation will fetch a fresh version of Nixpkgs, extract the
maintainer information, and update the team. It will support a dry-run
option.
Copy link
Member

Choose a reason for hiding this comment

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

Should membership be based on just master or also other branches like the active release?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe ping based on the branch that the PR is against?

Copy link
Member Author

Choose a reason for hiding this comment

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

OfBorg already does that, I think the question is what branches does the team's member list comprise of?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a good argument to query multiple branches, just asking in case somebody does.

@grahamc
Copy link
Member Author

grahamc commented Jan 16, 2019

I have addressed several questions in the latest push.

@shlevy
Copy link
Member

shlevy commented Jan 16, 2019

Seems good to me.

- 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

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)

@nixos-discourse

This comment has been minimized.

@7c6f434c
Copy link
Member

I would like to ask @ckauhaus for permission to nominate.

If that permission is granted, I want to nominate @ckauhaus as a person with an ongoing experience of both running an important part of semi-automated Nix*-related tooling and being a non-committer maintainer.

@grahamc
Copy link
Member Author

grahamc commented Jan 19, 2019

This link is now visible: https://discourse.nixos.org/t/new-rfc-39-open-for-shepherd-nominations-unprivileged-maintainer-team/1933/1

I'd be interested in nominating @alyssais, given her being a fairly new member.

@grahamc
Copy link
Member Author

grahamc commented Jan 19, 2019

Remember self-nominations are fine :)

@ckauhaus
Copy link

To me, implementing the changes suggested in this RFC would be definitely an improvement.

Possible add-on: would it be feasible to let non-committers have ofBorg set ticket labels? This is a capability I'm missing regularly.

@timokau
Copy link
Member

timokau commented Jan 20, 2019

@ckauhaus see NixOS/ofborg#216

@7c6f434c
Copy link
Member

@ckauhaus I also hope that your being on the record as Caring About Security and Caring About Tooling will be useful whe co-moderating the security-vs-usability tradeoff discussions.

@lheckemann
Copy link
Member

I'd like to self-nominate for shepherding this RFC.

@globin
Copy link
Member

globin commented Jan 24, 2019

Per meeting of the @NixOS/rfc-steering-committee: Shepherding Team is @lheckemann as leader, @ckauhaus and @alyssais.

EDIT: Also shepherds, please be reminded that it could make sense to have a chat on IRC or by videoconferece, to discuss your opinions and with the author to move this forward in an orderly and constructive way!

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/shepherds-chosen-for-rfc-13-17-and-39/1964/1

@globin
Copy link
Member

globin commented Jan 29, 2019

I think this RFC is a nice example of bringing our tooling forward to improve the use of features github can provide, without impeding current workflow and also enabling a wider audience to help review pull requests, which as I and other release managers have pointed out previously, is rather necessary!

I very much welcome this and would like to encourage anyone opposing this or having doubts about this to step up and voice their opinion, as otherwise if I haven't missed anything all issues brought forward earlier have been addressed.

@grahamc
Copy link
Member Author

grahamc commented Aug 18, 2019

I'm delighted to say that the permissions grantable for the new team management APIs are much finer grained than before, requiring a GitHub Application instead. The credentials used for this implementation are now restricted to a much finer subset than we could have done when this RFC was authored. The scope it has can be read about here: https://developer.github.com/v3/apps/permissions/#permission-on-members