Except for Darwin tests IMHO, but that’s a detail.
Not sure why should have such a low limit, how do we decide arbitrarily that number? I feel like anything that is staging zone should just be blocked until human intervention.
This is a meaningless limitation IMHO, you can always create another GitHub account to do this type of things.
What would be the purpose or the aim?
That seems to encourage to game that metric and people who have high numbers are not necessarily always the best person to be that invoker IME (no offense).
Sounds reasonable except when such maintainer does not exist.
I don’t understand why, that seems limiting the bot to staging workflow.
I would make the delay even longer for certain set of packages. Like a systemd merge maybe should go on for 1 week or more.
Unclear what purpose does it serve, I would not introduce it without a clear idea of what does it bring to the table.
That’s kinda OfBorg responsibility, it seems excessive.
Here’s another thing I would like the mergebot to do:
choose reviewers based on the files touched, the code owners, etc.
assign 3/4 reviewers among this list, wait for their interaction with a timeout of N weeks
choose another set of reviewers and escalate it until you find like a group of people who interacts
I don’t think we need to discuss these exact requirements here. The main thing I want to see is confirmation that this bot isn’t being deployed without first going through either the RFC process or something resembling it closely.
This is rather a reduction in scope of the merge-bot. PRs that affect more should be subjected to proper review by a committer IMO.
Any automated merge bot would be susceptible to multiple accounts / a circle of friends cooperating, which should be a part of the discussion of attack vectors.
Two reasons: (1) to avoid abuse (someone could write a bot to post invocations on all PRs), and (2) to encourage reviewers to not get burned out. The incentives given to reviewers should be to produce a steady flow of reviews. A different system achieving the same effect is much appreaciated.
True, let’s not axe the committers quite yet.
Maybe my view of staging is lacking. I want to give committers a chance to revert bot-merged PRs before they hit master and get tagged in a release. A separate branch could do the trick.
Committers can still shortcut this wait time. I also central packages like systemd result in more than 10 rebuilds, making the bot stop at my second proposed check with a max rebuild limit. For simple version bump Prs and the like i think such a check is quite reasonable.
This is a crude way to incentivize invokers to be careful, and to disincentivize drive-by invokation. A better system is much appreciated
It is excessive, i believe the benefit is worth it. It could be conditioned on the ofborg build being older than x days
Great critique. These discussions really highlight the need for a RFC, and is better served on a site which supports tree-based discussion and voting
I’d first work on raising this 1% higher. No merge-bot or policy is needed for that (or down-sides like security questions).
I’ll much easier merge a PR if it’s approved by a .meta.maintainer. And anyone with merge privileges can use the above link and merge, based on their (human) judgement.
Some people keep thinking that their feedback is near-useless if they don’t have merge privileges, but I strongly oppose that.
Security principles: IMO the issue is that we’ve been handing out maintainership for many years assuming that it gives no permissions whatsoever. Now it would suddenly allow self-merging without anyone getting notified?
BTW, note that the label gets applied even in PRs where the person makes themselves a maintainer. A simple illustration how easily things can go wrong.
A merge-bot does have impacts. Let’s consider them by category:
short-term: day-to-day Nixpkgs operations. The people most likely who would be either harmed or benefited should comment about this. So if this starts to go haywire, who is likely to be on hand to fix it? Who are the people whose lives are made easier and stand to gain provide their thoughts.
long-term: the security model of Nixpkgs is impacted. But it is not quite clear if it is positive or negative.
active-contributors and committers: specific kinds of normal awareness is hidden by automation. The proposal distributes this and makes it possible to have fewer committers, but more people with a limited form of commit access.
general users: Will end-users benefit? What harm can come to them? Can this harm be mitigated in other ways?
I can’t honestly determine if everything ends up with a net benefit or not, but if we can proceed with a kind of risk that can be mitigate or dealt with, (and the people who will have to deal with it are okay with it) it might end up being quite productive.
There are a few proposals for mitigating the risk, let’s try to clarify them and see if they make the risks acceptable enough to proceed with an experiment.
We only use staging to batch mass-rebuilds together. Anything that doesn’t trigger a mass-rebuild goes to master. (i.e. most leaf-packages). Once something lands on master it takes a while for it to trickle down into a release due to hydra having to build the commit. So a commit can be reverted before it hits a release.
we moved the discussion to matrix https://matrix.to/#/#nixpkgs-merge-bot:lassul.us
just come by if you want to contribute, we can add some more checks before merging and make it more strict down in the beginning until most people are happy (like checking for eval, ofborg, etc.).
You can’t rely on that, unless the package is in the channel-blocking set. Because even your fix will wait, just as the breakage, and channel update might just as well happen in between.
Uhm, package additions would still benefit from having a lot of people with different interest areas who can merge them.
Does anyone know a cheap way to see the percentage across the previous 5000 PRs that were merged? Because if maintainer-approved PRs get merged faster, we would have a low percentage for what you measure here.
(These searches are only bounded below in time, so in the future all numbers will grow; you may have to take my word for it that they are accurate as of this writing.)
the bot can also do merges for new packages. we just have to code a set of rules which would apply to do that (like maybe a subset of people, or a thumbs up/down voting?, additionally to some CI/quality/style checks). The idea is to extend the bot further and further until nearly all actions can be done through the bot and we just have to fix the code instead of poking humans to do stuff differently.
Yeah the amount of PRs the bot can merge with the current set of restrictions would make it not very helpful. But change has to come in iterative steps or it will not come at all. All commiters I’ve talked to would gladly give up their commit bit if we would have something like a merger bot.
I’m starting to get kind of frustrated at how this topic is getting handled, fwiw:
Well, yeah. The bot could also just merge everything with no review from anyone. What the bot can do is irrelevant. What has been agreed that the bot should do is relevant, and we’ve definitely not have any kind of consensus on merges for new packages or anything remotely close to this. In fact, you don’t even have anything close to consensus for the original proposal in this thread, even though it was announced as a fait accompli.
That “just” is very load bearing. The automation for many of the checks that committers do before merging does not exist. In fact, we don’t even have a proper list of such checks. The “auto merge once checks are green” is trivial, the figuring out and coding the set of rules is the hard part, if it’s even possible (IMO it’s equivalent to the “automating a programmer’s job” problem, which I’d say we’re still very far from).
Going from “r-ryantm PRs” to basically anything else is a huge transformative step, not an iteration. There are hundreds of things that apply to PRs authored by humans, which don’t apply to r-ryantm PRs because they’re machine generated and extremely limited in scope and form. To the extent of my knowledge, r-ryantm PRs are the only significant source of machine generated PRs on nixpkgs, so I don’t see a trivial iterative step to “more than just r-ryantm”.
That’s absolutely wrong. Several committers have directly told you that they think this is a bad idea in its current form. There was hours and hours of discussion on Matrix yesterday. I don’t see any of this feedback integrated in what you’re saying today, and frankly I don’t really see the point of continuing to give feedback at this point when you’ll just ignore it and claim obviously verifiably false things like this last statement.
Even if that’s as far as the bot ever gets, in the last 10 days, r-ryantm PRs were 32% of all created PRs. Empowering the maintainer community to merge almost a third of Nixpkgs’ PRs without requiring committer involvement at all is surely a win.
Sure, but we also have literally 0 r-ryantm open PRs with maintainer approval right now. The committer workload to merge those PRs is very low, and as shown by the number of open PRs the benefits in terms of merge speed/latency would likely be very low as well. We already have several committers “patrolling” those PRs and making sure they get merged quickly, and it takes an insignificant amount of time compared to most other PR reviews. I think ryantm himself mentioned he does this sometimes?
To be clear: I’m not opposed to having a bot that auto-merges r-ryantm PRs that have maintainer approval. But I also don’t think it helps us much with the actual problems of nixpkgs PR reviews. It’s taking the part that’s already solved and micro-optimizing a detail of it.
I think it’s better to look at the history rather than what’s currently opened and approved by maintainers (maybe it’s going way faster if approved?): 1156 merged PR for r-ryantm were approved by maintainers, for a total of 44836 merged PRs, that’s just 2.6% of PRs.
This is not even counting when maintainers are people with commit access…
The incentives given to reviewers should be to produce a steady flow of reviews. A different system achieving the same effect is much appreaciated.