To celebrate nearly reaching 5000 concurrent open pull requests on nixpkgs, we’ve created a merge bot!
What should it do?
This bot would allow maintainers to merge PRs that touch their packages. It means that now maintainers can react to user feedback and self-service. That should scale much better since there are 3030 maintainers, and only 203 committers.
Maintainers will be able to leave a comment to get their PRs merged
@nixpkgs-merge-bot merge
The bot will check if the right conditions are met and either merge the PR, or leave a comment explaining the issue.
Limitations
In order to reduce the attack surface, the bot is currently limited to the following rules:
Only works on packages that are under pkgs/by-name/*
Only allows master, staging and staging-next target branches.
It doesn’t check CI results (since they are unreliable right now)
Then as we learn from it, keep iterating to increase the self-service surface.
While I personally support this idea, does this have a corresponding RFC? As it greatly changes the security model of nixpkgs and would worth some discussion.
Good questions; this is something we also asked ourselves as well.
The current design has a very small impact as there are only ~50 leaf packages in pkgs/by-name. This will provide us with much-needed feedback, that we won’t get if we go trough the RFC process. And by the time the RFC would be accepted, the package set will be bigger.
In general, I feel like we are leaning on the RFC process too much. RFCs are good for things that are hard to revert. Experimentation is also important and something we should nurture.
While the current impact is limited by the size of the by-name package set, by-name itself does not directly reflect the importance (whether leaf or not) of a package. Can we add additional checks in the meantime? Like limiting the number of rebuilds, or restricting the origin of the pr (those from r-ryantm is mostly safe to merge).
I agree that we’ve been relying on the RFC process a lot [edit: too much], and that this [edit: by which I mean a merge bot] is probably a good thing, but it seems a bit weird to fundamentally change the security model of Nixpkgs contributions without any sort of discussion beforehand.
Now that being a maintainer actually confers privileges, we have to be quite a bit more cautious with letting people add themselves as maintainers, and we have a big collection of maintainerships that haven’t had those standards applied, which is quite scary to me.
The bot is not yet live. I think the current impact would be just like 5 packages and most comiters I know would merge a PR which is approved by a maintainer without a second thought. so IMHO there is no big change in the security aspect happening.
Can you explain what you mean by this? Wouldn’t it affect many thousands of packages?
[edit: I see, because it only works with by-name, it would only affect 250 packages.]
I’d hope that they’d still give it a quick glance to make sure it isn’t doing something obviously weird, like changing where the sources came from or adding a big, unexplained patch. That sort of quick check is the sort of thing I’m worried about losing, if thousands of people can suddenly edit packages without any oversight at all that isn’t retrospective.
(I do see advantages here too, of course — one being that we probably don’t need to be giving full committer privileges away to so many people if they’re able to get permissions to work on just the packages they really care/know about, without the permissions to do changes across all of Nixpkgs.)
I agree with @qyliss, I think it’s reckless to introduce this without any prior discussion with the community.
I’ve been working on migrating essentially all packages to pkgs/by-name, but this seriously makes me reconsider that, because I’d be contributing to decreasing Nixpkgs security.
Though it creates the incentive to move your package to by-name to get faster merges to be honest.
Which creates an interesting situation which I think @Infinisil should have in mind w.r.t. to their goals with RFC140. Because as I understand it, we didn’t decide it to do the treewide move to by-name and there are probably good reasons to wait before doing it too fast.
As we learn from this, we’ll be able to make the bot more useful and then eventually remove commit access to the 200 people. It will make the process more democratic and self-service
I think the concerns that are brought up are reasonable, and they also apply against the existing committers. Part of the answer is to add more tooling and automate.
Another thing that I would love to see happen is the appearance of a “core” package set with different rules and guarantees.
FWIW: it’s not actually being introduced without prior discussion. That was my initial impression too, because the original post implies that the bot is already running, but since it turns out that it isn’t, this is the prior discussion.
So what we should be doing here is identifying what the concerns are, and then having a productive discussion how we address those concerns and get to the end point where maintainers do have more power over the packages they maintain. @NickCao has already made some good suggestions, as has @RaitoBezarius in #dev:nixos.org
I think the set of affected packages/maintainers should be chosen manually instead of using by-name. As written by others, using by-name creates unwanted incentives.
Moreover, if a few maintainers (instead of packages) are chosen, gathering feedback (edit: and establishing trust) will be easier.
While this is sorely wanted by many non-comitters, I’d say caution is essential.
I took some time to write a couple of additional checks, not all of which are in my eyes required, but are worth discussing:
For clarity, a glossary:
invoker: the one posting @nixpkgs-merge-bot merge
maintainer: anyone in maintainers.nix
CI must pass
max rebuilds: 10
higher limits could be earned on a per-invoker basis
no self merge
rate limiting: max x invocations per invoker per day
higher limits could be earned on a per-invoker basis
invoker must be a maintainer of at least X packages / nixos modules (query packages.json)
PR must be reviewed by at least one other maintainer than the one invoking the bot
PR must target staging or staging-next, not master
the merge bot waits 24 hours before merging, and can be canceled if anyone reacts with thumb down / posts some invocation like @nixpkgs-merge-bot object
Invocations of the bot when prior checks fail result in a timeout / penalty for the invoker
bot runs nixpkgs-review right before merging, which must succeed
Perhaps even a trial run where the comitters cobble together a list of say 20-500 maintainers they think should get try out the bot.
I’ve not really been involved in any RFCs, but even if there have been too many things that should not have been RFCs, that doesn’t mean that this is one of those things. If there’s anything that really deserves an RFC, policy and automation around security critical subjects seems like it.
I would suggest that experimentation be done with a dormant bot that cannot actually perform meaningful actions, but will demonstrate what it would do. An RFC can lay the groundwork for the core functionality, and that can evolve while we experiment with the dormant bot, before the RFC is finally accepted. Then, the bot can be awakened, and we can further experiment with further dormant features. Rinse and repeat as new ideas come to bear.
