Do we want to remove people automatically or by request?

I think automatically. If they are removed from the maintainer list, they either are being forced out, or have requested removal.

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.

I don't think this is on topic for this RFC.

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.

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.

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.

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.

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.

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 :)

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.

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.

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.

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.

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.

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

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)

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.

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?

Right.

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?

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

Should we perhaps use user IDs instead?

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

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…

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

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

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?

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)

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

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?

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.

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

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

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

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

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.

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.

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

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.

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.

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

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.

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

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

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.

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…

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

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

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.

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

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

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

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

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.

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

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.

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.

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.

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.

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

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.

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.

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)