I am convinced that the ability to give individuals more granular access control restricted to individual packages or folders, and with that moving to a workflow where maintainers can and are expected to merge pull requests for the packages they maintain, will improve the situation.
The way I imagine it is to have a merge bot which can be pinged to merge pull requests if the criteria match. The necessary requirements could for example look like this:
The person requesting the merge is registered maintainer of the package which the pull request modifies
The pull request has at least one review approval from a person which is not the author of the pull request
This means that maintainers can review and merge pull requests they receive on their packages. Maybe it would be preferable to always require a review from an uninvolved person instead, not sure.
Sometimes I witness some “random” GitHub accounts simply “accepting” a pull request, without review. Maybe it would be a good idea to only count reviews from other contributors/maintainers.
The pull request does not fail CI
This does have a couple of implications for our development workflow, as it places more emphasis on the individual package maintainer. This also means we’ll eventually have to have a discussion about what being a maintainer means, and what requirements there are.
Maybe the bot could later be extended to grant some people access to sub-directories in a code-owner fashion. While I like the idea, I fear that it might clash a bit with the follow-up work that will hopefully happen after RFC 140.
In the long term, I could even envision a future where all pull requests are merged by the bot, and even global commit access is handled by it. Rust for example merges all pull requests through bors, and while the motivation is a different one I still think that it would benefit us too. For example we could implement “merge once CI passes” with it, or forbid self-merges without independent reviews.
I would really appreciate if somebody built a small minimal prototype (maybe just a name → allowed directories mapping, because checking who is maintainer requires Nix evaluation etc.) for this, so that we may try it out and see if this does help or not.
I think we’d need fairly extensive sub-directory code owner support to deal with updates to packages that lack a maintainer, so making that a first-class part of the approach seems important.
I’d argue packages without a maintainer don’t need more tooling support, but a maintainer. But yes, we are going to have a discussion about unmaintained packages too (both effectively unmaintained ones, and ones without registered maintainer).
One idea I’d like to pursue eventually is to use the problems infrastructure from RFC 127 to allow people to see which “unmaintained” packages they are currently using. (We actually already do have code for this in there, but it is not really discoverable because it was built for CI.)
Strong +1. But that’s also not something which needs to be addressed as a prerequisite to your proposal, since at worst it decreases the effectiveness of it, but I don’t think it makes anything actively worse.
We should however make sure that maintainers understand their “new” responsibilities, and probably provide some guidelines as to what level of depth we expect reviews to have. Right now this is informally enforced by committers. And while I don’t think we enforce anything near to uniform standards before merging, I do think extending that to a significantly larger group might make that situation worse.
Indeed we don’t need a bot that covers everything, first version can be very picky and still useful.
I don’t know, just to stress that this is a step and not the final state: a pre-existing package maintainer of a package can request a merge of any PR to that package approved by a committer. I have seen PRs approved by multiple committers, opposed by noone and not merged just because everyone said «but I will wait in case someone has a workflow to test» then noone remembered to revisit. This won’t be useful often, but this allows to gain confidence in a bot gradually, starting with a low-volume testing. It increases value of «LGTM-but-defer-to-maintainer» committer approvals a bit, so maybe it will naturally become slightly more useful over time, but even if not, we’d have a base to put fancier policies on top.
Currently the maintainer of the package needs to waiting for a committer for merging. Now the maintainer needs to find a random user approving their PR. This doesn’t make sense to me.
If the maintainer want a really useful review they need to find someone who use the package. It’s difficult.
If they just want to get the package merged, they can find whoever want to just approve. This doesn’t help improve the quality of the PR.
If they just want to slip in something bad they can even create another github account. It’s easy.
Imo one of the best ways to give partial access to maintainers is:
Have an official bot regularly run update scripts for packages and create pull requests from that. In practice this could mean making @ryantm’s bot official.
Merge those updates automatically if they’re approved by the majority of maintainers (and nobody requested changes for some time). This could be done with a GitHub Action workflow.
Yes this is more limited, maintainers only get the permission to merge changes that were done by the update script, and changes to the update script will have to be reviewed manually, but this would already be a big improvement without compromising security.
My proposal is assuming good faith in all contributors, and that people are at worst lazy. As already covered in my original post: We could only count reviews from contributors or other maintainers, to shield against random approvals or even puppet account (which I generally don’t want to include in my threat model, because good faith).
My general motivation is that every PR has been looked at by at least four eyes. I assume that most reviewers don’t just blindly click approve, and instead at least scroll through the code (“LGTM”). This is even encouraged by the GitHub UI, where you can only approve from the code view.
Maybe we could lift this restriction a bit though for changes that only bump version and hash. Another possible way of lifting the restriction is if the pull request has been open for a certain amount of time.
Another thing which wasn’t mentioned here but is tangentially relevant: I’m actively working on making sure the “approved: by-maintainer” label is applied consistently and quickly on nixpkgs PRs. When I have some time to spend on nixpkgs reviews that’s the first place I go look, since usually I can trust someone has done the bulk of the work (I of course don’t blind merge, but I don’t treat it as something I need to review “from scratch” either).
So there is already value today in maintainers using GitHub’s approval function if they don’t already!
(It’s currently applied by a bot which crawls the last ~1K PRs roughly every hour, and I’m in the process of migrating that to a GH action.)
I think we also have some automation regarding the first-time contributors label, could the code bases maybe merge into something more general? (CC @Janik)
This (currently semi-official) policy is a big part of nixpkgs’ current stagnation problems. It very strongly biases contributions towards superficial PRs, drive-by commits, and deletionists.
There are ~200 committers. Basically all of them are able to understand version-bump commits. Likewise for anything that deletes functionality not used by the “two big platforms”. So these kinds of PRs get instamerged.
Large-scale refactorings involving intimate understanding of the underlying software – which are essential to the ongoing health of nixpkgs – have a comparatively tiny base of qualified reviewers. Likewise for plumbing in the “trunk packages” like stdenv, cc-wrapper, and build-support. So this policy implicitly imposes a ~200x heavier burden on this kind of PR. In the long run it leads to people simply not wanting to write these sorts of PR. This is very bad news.
Before we can automate this kind of policy, we should put serious thought into whether or not it is a sensible policy in the first place. IMHO two preconditions are:
“Teams” should have territories clearly delimited in the codebase, and a duty to review any PR that touches code in their territory. Territorries can overlap. Rust does this using a bot. If a team can’t keep up with this duty, they must admit new members until they can. The reviews which count for merging are the reviews by the people who’ve accepted this responsibility. This links authority to responsibility, a principle that I am generally very much in favor of. I don’t think it’s such a great idea to have teams with (implied) authority but no clearly defined reviewing obligations. I’m mildly suspicious of any team that isn’t mapped into the CODEOWNERS file.
Monorepo development assumes an unlimited CI budget. This is a mild exaggeration, but it’s certainly true at Google. In a monorepo, it’s okay for other random people to edit your code because you’re expected to write tests (lots of tests) that will trigger an alarm if those random people break your code. Right now we can’t do that in nixpkgs because our CI system is so awful – largely due to github constraints, but also due to opaqueness around ofborg’s resource allocation and the fact that it lives outside the monorepo. Right now people aren’t allowed to easily add new CI tests for anything they care about not breaking – being able to do that is the only way monorepo development makes sense. So instead we have this system where humans are expected to manually watch for PRs that break stuff they care about… this is crazy; this is a recipe for conflict and burnout.
If we want nixpkgs to continue to be a monorepo, this needs to be addressed in a serious discussion that isn’t limited to what is possible in github. If that doesn’t happen then it is inevitable that nixpkgs will gradually cease to be a monorepo – either by pieces flaking off or by other mechanisms.
Review by a second human is not enforceable, and increases the acute throughput problem we have. Yes, ideally we need to define a subset of easy-and-low-impact changes that are neither created nor reviewed by humans (although with a grace period for humans to find them if there are news about the packages in question).
This feels very similar to something I proposed What's the point of becoming a package maintainer? - #24 by UefiPls . A merge bot that checks if the code owners or maintainers of a package have reviewed the code (maybe with other sanity checks) would make the state of a permissionless maintainer more useful.
IMO it would be better than the existing system, where a “maintainer” can’t actually maintain their own packages. They have to helplessly signal to somebody with committer rights when anything has to be merged. A maintainer’s feedback only has subjective value as a committer can simply choose to ignore it.
I started working on GitHub - LoveIsGrief/check-permissions: A generic action to check permissions (which has no documentation whatsoever atm as I had no time to work on it).
The idea was that it would act as a step to check approval count, approval percentage of code owners, and a total number of approvals from code owners. If the step passes, another step could in a trial period at first ping committers and in “production” automatically merge PRs.
What it got held up by was Github’s removal of lastPushedAt information on PRs. There doesn’t seem to be explanation and I can’t think of another way to get reviews that are up still valid, as they might be invalid after a new push to the branch.
Edit: have to check if PullRequestReviewState.DISMISSED can happen automatically after a push to a branch.
Would this be something along the lines of what you’re suggesting @piegames ?
Thank you for starting this. I was hoping someone would pick up the idea, as I currently do not have the capacity for working on it myself.
As a first step, my suggestion would be to write a minimal bot but which doesn’t actually merge PRs. Instead, it notifies some volunteer committers (through ping or adding a label or posting on Discourse or Matrix), which then do a last sanity check before merging. This way, we can explore the idea and experiment with the rules in a safe manner.
One thing that is very important to me though: this should not be a passive and automatic “check” done on PRs, but this should be an action triggered by people. It should feel more like “please merge this now” than “this has N approvals” (which we more or less already have currently).
Autonomously merging code must never be done, even in production. There must always be at least one human in the loop triggering the merge.
I’ve been toying with the idea of supercharging nixpkgs-review to additionally walk the reviewer through a testing process, which results in a easily skimmable report. If the quality of drive-by reviews were raised then comitters would be much more comfortable merging approved prs. Right now I’m writing code to generate reports about systematic errors in nixpkgs, and the code could be reused in such a review oriented tool
