Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0050] Merge bot for maintainers #50

Closed
wants to merge 3 commits into from
Closed

[RFC 0050] Merge bot for maintainers #50

wants to merge 3 commits into from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Aug 17, 2019

An RFC for having a bot that allows maintainers to merge their changes to Nixpkgs. I think this can help a lot to reduce the load on nixpkgs-committers by letting maintainers of leaf packages merge their own changes, however, there is also a big security risk.

I think the latter outweighs the former. Even so, I hope this RFC can help us further in our quest on how to scale Nixpkgs either by directly offering a way to reduce load on the nixpkgs-committers, or by agreeing to not pursue a certain option.

@FRidh FRidh changed the title rfc 50: init [RFC 50] Merge bot for maintainers Aug 17, 2019
@FRidh FRidh changed the title [RFC 50] Merge bot for maintainers [RFC 0050] Merge bot for maintainers Aug 17, 2019
# Alternatives
[alternatives]: #alternatives

1. Hand out write permissions more freely. More people will be able to make changes anymore.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I much much much prefer this. Have we had any issues because someone was given the commit bit who shouldn't have been, ever? What exactly are we trying to prevent that community norms + git wouldn't give us just as easily?

@timokau
Copy link
Member

timokau commented Aug 17, 2019

If we're building new tooling, I would instead prefer to go further in the direction of separating review and merge. This could be extended with a bot that allows reviewers to add tags (and committers to filter for them).

Alternatively we could take up a sort of mentorship program: Someone that wants to help up teams up with someone with commit permissions. They review PRs and ping their "mentor" when they think its mergable. They take a last look, maybe suggest improvements (thereby teaching the reviewer "on the job").

That may lead to giving the mentee commit permission after a while. However that is also a problem of this approach, as the mentee may feel entitled for this and this may be gamed.

Giving any maintainer merge permissions for whatever they maintain without at least having a trusted committer have a last look over it sacrifices too much security in my opinion.

@7c6f434c
Copy link
Member

7c6f434c commented Aug 17, 2019 via email

@shlevy
Copy link
Member

shlevy commented Aug 17, 2019

I, personally, am trying to prevent people complaining too much about maybe giving commit access too easily, and people not asking for the full access because they think they are not ready for general commit access.

This is an RFC, if people have such complaints then they can raise them here and if people have such concerns about their readiness then we can address them here.

If we think liberal provision of the commit bit is wrong, fine let's make that case. But if we just think it's a challenging social transition, then let's please not try to address social problems with technical solutions.

@shlevy
Copy link
Member

shlevy commented Aug 17, 2019

To be clear, I'd honestly be happy to have some very general high level guidelines about what to do with commit access (e.g. don't merge to something you don't own without trying to work with the owner first, especially not over their explicit rejection, don't merge anything that breaks automation, operate in good faith, etc.) and then give people access after their first contribution. If people think that's too soon, what specifically is the concern, and has it ever actually materialized in nixpkgs's fairly long history?

@7c6f434c
Copy link
Member

7c6f434c commented Aug 17, 2019 via email

@shlevy
Copy link
Member

shlevy commented Aug 17, 2019

Whether they raise them here or not, someone will raise them elsewhere again, eventually.

And they will have an RFC process to let them do that.

Replacing a step change with a more gradual change by technical means is quite often a proper part of a (multi-faceted) solution to the social problem of arguing what is the correct threshold.

Sure. If there is disagreement that can't be bridged but there is a change all reasonable sides can agree to, that makes sense. But as far as I know we haven't actually raised this issue in RFC format, which is the right place to try. Do we know we can't actually reach a good outcome here directly?

@asymmetric
Copy link
Contributor

Is the current process for obtaining commit access detailed somewhere?

@7c6f434c
Copy link
Member

7c6f434c commented Aug 17, 2019

@shlevy have you read NixOS/nixpkgs#50105 for example?

Also, NixOS/nixpkgs#20836

@Ma27
Copy link
Member

Ma27 commented Aug 17, 2019

Is the current process for obtaining commit access detailed somewhere?

I've been added as committer after reaching the top ~80 contributors and I know from some other people that they were proposed by a committer. Not sure if there's an explicitly defined process though.

Regarding the RFC itself: will read through it during the weekend :)

@FRidh
Copy link
Member Author

FRidh commented Aug 17, 2019

This could be extended with a bot that allows reviewers to add tags (and committers to filter for them).

GitHub added a beta permission level called "Triage" which allows users to add tags. That may be even better than "Read" for @nixpkgs-maintainers. See #39 (comment)

@shlevy
Copy link
Member

shlevy commented Aug 17, 2019

@shlevy have you read NixOS/nixpkgs#50105 for example?

I hadn't. Why wasn't it an RFC? 😀

@shlevy
Copy link
Member

shlevy commented Aug 17, 2019

Alright I'll open up my own RFC I guess. 👎 on this one personally unless we really can't get past the current restrictive access otherwise.

@zimbatm
Copy link
Member

zimbatm commented Aug 17, 2019

The current version of the RFC focuses a bit too much on security. For me the more exiting aspect is that once we have a script to map diffs to maintainers, it will be easier to assign PRs to maintainers as well. And maybe issues even. That will allow to distribute the load quite nicely and reach the right people who have experience with their part of nixpkgs.

Security is definitely a concern I have had raised by a few customers. If anyone can merge to master the attack surface is pretty big. Maybe it hasn't happened today but it's naive to think it will never happen as NixOS becomes more popular.

@7c6f434c
Copy link
Member

@zimbatm ofBorg already recognises maintainers (except some rare corner cases that are against best practices for other reaons anyway) and requests review. There is a catch that GitHub doesn't allow requesting review from non-member maintainers, and #39 was about fixing that — and Graham has just found time to implement and start deploying the tooling for immediately giving maintainers membership (without immediate write access).

So this PR is about a natural next level of tooling: partial write access focused on the packages most likely to get attention-starved.

@jonringer
Copy link
Contributor

@FRidh it would be nice to be able to label the PRs I review with something that relates to it's "Ready to Merge". Would also give 2 person accountability to all the merges.

@globin
Copy link
Member

globin commented Aug 19, 2019

I'm not in favour of extending commit rights more liberally by itself, because I think this makes it even harder to provide adequate security. An attacker can currently simply push a little commit to a nixpkgs release branch without anyone noticing except if someone is actively checking all commits.

I'd prefer us to protect all release-* and master branches to only allow commits to them via pull requests or possibly only let a bot push to them after the basic checks (i.e. ofborg) have been run (similar to rust's @bors). Maybe also use the @NixOS/nixpkgs-maintainers group + 2 reviewers necessary github restriction.

@7c6f434c
Copy link
Member

7c6f434c commented Aug 19, 2019 via email


When these conditions are met the maintainer can write

@grahamcofborg merge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means that the source can be altered to anything else, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, within the expression any change could be made.

@symphorien
Copy link
Member

It seems that @GrahamcOfBorg adds the "by package maintainer" as soon as the author of the PR becomes maintainer once the PR merged. It is not required that the author be already a maintainer before. Example:
NixOS/nixpkgs#67115
Therefore if I understand the RFC correctly I can modify freely any package A by just making a PR which also makes me maintainer of A.

So it should probably be clarified that a maintainer can merge modifications to a package only if they were maintainer before the PR is merged. Or alternatively that a PR which modifies the list of maintainers cannot be auto-merged.

@symphorien
Copy link
Member

Therefore if I understand the RFC correctly I can modify freely any package A by just making a PR which also makes me maintainer of A.

Rereading the RFC, it seems the PR needs to add me as maintainer of all reverse dependencies as well.

@7c6f434c
Copy link
Member

And of course there is also the additional requirement that the «maintainer of all modified packages» check is not in a race condition with reevaluation of pushes, and also the check for modified packages should include some cross-builds (otherwise a change that always changes one package and a lot more for corss-builds will be handled weirdly)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nixos-package-information-platform/3787/1

@globin
Copy link
Member

globin commented Aug 22, 2019

This RFC is open for shepherd nominations, please bring forward nominations (might include yourself).

Quoting RFC #36 for the responsibilities of the shepherd team:

Shepherd Team

A team of 3-4 community members defined unanimously by the RFC Steering Committee, responsible for accepting or rejecting a specific RFC. This team is created per RFC from community members nominated in the discussion on that RFC.

This team should be people who are very familiar with the main components touched by the RFC. The author cannot be part of the Shepherd Team. In addition, at most half of the Shepherd Team can be part of the RFC Steering Committee.

The responsibility of the team is to guide the discussion as long as it is constructive, new points are brought up and the RFC is iterated on and from time to time summarise the current state of discussion. If this is the case no longer, then the Shepherd Team shall step in with a motion for FCP.

Shepherd Leader

The person in charge of the RFC process for a specific RFC, and responsible for ensuring the process is followed in a timely fashion. The Shepherd Leader has no special responsibility with regard to moving an undecided Shepherd Team to a certain decision.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/new-rfc-50-merge-bot-for-maintainers/3795/1

@jollheef
Copy link
Member

jollheef commented Jan 6, 2020

Unfortunately, GitHub not warning in any way when you answer with an email that a post that you answer for is already removed. So, it'll be strange for me to defend a deleted comment that was the result of my misread.

However, @jtojnar, you cannot trust previous version bumps anyway because not all of the nixpkgs-committers actually checking what they merge. Just like not all of the people who make a pull request and checkmark "I've tested what I did" actually testing it.

@timokau, there was no idea to merge it "checking that only version and sha256 are changed". It was a bit more complicated. Anyway, the RFC assumes that the package maintainer will be able to merge his packages, which means from a security model point of view there's no difference.

@worldofpeace
Copy link
Contributor

worldofpeace commented Jan 21, 2020

@aanderse Could you lead this one in @globin absence?

@aanderse
Copy link
Member

@worldofpeace certainly.

@FRidh @grahamc @worldofpeace I would like to schedule a quick call so we can regroup on this. Seeing as it is already Tuesday and this is an office hours week I don't expect everyone to be available this week but I really hope everyone can make 30 minutes available before the end of next week.

@FRidh I believe aside from you the rest of us are in EST (or somewhere close to). Can everyone post a few desired times or time ranges for the next 2 weeks please?

@aanderse
Copy link
Member

ping @FRidh @grahamc @worldofpeace on availability

@worldofpeace
Copy link
Contributor

Next week monday at 10am EST would be good for me (I also thing that would be the afternoon for @FRidh) How about we do a https://doodle.com/create/?

@fgaz
Copy link
Member

fgaz commented Jan 31, 2020

How about we do a https://doodle.com/create/?

Sorry for pinging you all, but may I suggest framadate (there are instances at https://framadate.org/ and https://poll.disroot.org/)? Unkike doodle it doesn't share your data with literally hundreds of third parties, it has no ads, and it's free software.

@aanderse
Copy link
Member

aanderse commented Feb 1, 2020

ping @FRidh @grahamc @worldofpeace on availability via link below.

https://poll.disroot.org/8lXUangUAt2jNR5q

Let me know if these don't work out and what date+times work better for you.

@worldofpeace
Copy link
Contributor

@aanderse Did it

@aanderse
Copy link
Member

aanderse commented Feb 1, 2020

None of the options worked for everyone so I have posted a new poll with new suggested times.

ping @FRidh @grahamc @worldofpeace with https://poll.disroot.org/8lXUangUAt2jNR5q

@worldofpeace
Copy link
Contributor

None of the options worked for everyone so I have posted a new poll with new suggested times.

ping @FRidh @grahamc @worldofpeace with https://poll.disroot.org/8lXUangUAt2jNR5q

Completed.

@FRidh
Copy link
Member Author

FRidh commented Feb 2, 2020

I am reducing the amount of time I spend on NixOS/Nixpkgs and this RFC is not very high on my priority list so I intend to close this unless someone is willing to take it over. The main thing that still had to be done was to create a flow chart.

@worldofpeace
Copy link
Contributor

@FRidh Closed.
Thanks for making it though, I hope somebody can pick this up eventually.

@jonringer
Copy link
Contributor

@worldofpeace agreed, if only I had infinite time I would pick this up :(

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/steps-towards-even-more-pr-automation/5634/13

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/stats-trends-on-github-issues-prs/7936/36

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/24

@danieldk danieldk mentioned this pull request Oct 2, 2020
10 tasks
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/misuse-of-commit-permissions/10071/38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.