Feature Request: Automatic merging

[matthewbauer]

It's a security concern to have so many contributors to Nixpkgs with write access. Eventually, I think we will want to start removing commit access for inactive contributors.

For now, though, there's not a very easy way to contribute to Nixpkgs without having write access. I'm thinking "Ofborg" might be able to help with that. A basic proposal (maybe needs to be an RFC):

When at least 1 "known user" approves a PR based on master and all tests have passed, a timer is set for 24 hours. Ofborg sends a comment explaining the 24 hour rule and the expected merge time. After 24 hours, Ofborg checks that: no changes have been requested, no commits have been changed, and that the PR is mergeable into master. If all checks pass, Ofborg automatically merges the PR into master.

The idea is not to replace regular merging but to provide an alternative route for "harmless PRs". Once feedback is given, then it's assumed that manual merging should take place (veto). "Harmless PRs" would include things like new packages and package updates. Users with commit access can still revert the merge later on if they object. The goal is to cut down on the number of open PRs and also make life easier for some of the "mergers".

See also NixOS/nixpkgs#20836

[Mic92]

The maintainers field could also play a role maybe?

[7c6f434c]

@matthewbauer I think this the wrong first step. I would want to first support explicit borg merge-if-build-succeeds package-name (next step: by platform, and allow tests as conditions). A benefit over your approach: well, a rollout doesn't suddenly change the landscape.

[matthewbauer]

I think this the wrong first step

Yes, this is more of a long term thinking than a first step. A conditional "merge" command makes a lot of sense as a first step.

[7c6f434c]

Re: security: as long as the backlog keeps growing it is a bad idea to reduce the number of people who are allowed to review. If I can merge any PR that passes tests, you may mitigate my negligence, but not my mailce. Maybe it is a good long-term idea to have merge-if-someone-else-approves support.

As for approvals: I generally use approvals when I would not want them to trigger a delayed merge… A «borg countdown 24h» command would be tempting; as would be «borg remind 24h». We shouldn't assume GitHub UI makes sense in the long term and formulate policy in terms of UI.

[Ericson2314]

Yeah something along these lines is essential.

[7c6f434c]

Also, a merged PR could be cherry-picked into another branch, too. Something like backport 18.03

[grahamc]

@Ericson2314 along what lines exactly?

[grahamc]

My imagination for a first round is something like:

@grahamcofborg build-then-merge foo bar baz

This would then build foo, bar, and baz on all the platforms the user is authorized to build on. If none fail (a package not being supported on a platform is not an issue) and the PR's latest commit is properly evaluated, it'll merge.

1. It is important that the commit the build was triggered on is the same as the current HEAD at merge time
2. what do we do about users without darwin access? do we merge anyway?
3. "extra-known-userss" will have to be separated out in to a third, lower level who cannot call the merge command.

[grahamc]

An additional problem (I should have waited a moment before commenting) is how to handle the merges w.r.t. attribution? Showing ofborg merged a bunch of PRs detracts from the community leaderboards, however doing more custom git merges where we impersonate users is much more difficult and can't be done simply via the API. Another option is being able to call the GitHub API on behalf of users, and using their token for their merge.

Opinions?

[matthewbauer]

how to handle the merges w.r.t. attribution?

I think having the bot be the merge author makes the most sense. AFAIK merge commits are not included in GitHub insights.

[7c6f434c]

Does anyone care about high-precision leaderboards? Do we want to encourage caring? If so, my vanity.sh can be polished a bit and actually used.

[grahamc]

cc #112

[davidak]

On 23.05.2017 the "social coding experiment" Chaos bot started.

It was a bot that merges PRs based on democratic voting by any github user. The interesting part is that it was applied on it's own repository, so a merged PR restarts the bot with the change. That lead to interesting events, like trolls who tries to take over the bot, what succeed once. Later a meritocracy was installed where at least one trusted user has to vote for it. It had an active community from all over the world and was pretty successful i would say.

https://github.com/Chaosthebot/Chaos/blob/master/README.md

Here you see an example for such PR with the last comment from the bot: Chaosthebot/Chaos#533

[Synthetica9]

"extra-known-userss" [sic] will have to be separated out in to a third, lower level who cannot call the merge command.

Ideally, you'd only allow the build-then-merge for users that would already have write access.

[7c6f434c]

Ideally, you'd only allow the `build-then-merge` for users that would already have write access.

Unclear: having an own authoritative ACL has its own benefits, and there are some voices for giving less direct write access and more some-ofborg-check-gated merge access.

[Synthetica9]

@7c6f434c Okay, I suppose I could also see that

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/bootstrap-files-updates-amplifiy-exploit-of-any-package-into-exploit-of-every-package/50534/6