From b1a70d661bfa9bc6978d52404eec13c5684cce4c Mon Sep 17 00:00:00 2001 From: Frederik Rietdijk Date: Sat, 17 Aug 2019 11:57:54 +0200 Subject: [PATCH 1/3] rfc 50: init --- rfcs/0050-merge-bot-for-maintainers.md | 83 ++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 rfcs/0050-merge-bot-for-maintainers.md diff --git a/rfcs/0050-merge-bot-for-maintainers.md b/rfcs/0050-merge-bot-for-maintainers.md new file mode 100644 index 000000000..0036bc7d5 --- /dev/null +++ b/rfcs/0050-merge-bot-for-maintainers.md @@ -0,0 +1,83 @@ +--- +feature: merge-bot-for-maintainers +start-date: 2019-08-17 +author: Frederik Rietdijk +co-authors: (find a buddy later to help our with the RFC) +shepherd-team: (names, to be nominated and accepted by RFC steering committee) +shepherd-leader: (name to be appointed by RFC steering committee) +related-issues: (will contain links to implementation PRs) +--- + +# Summary +[summary]: #summary + +Allow maintainers of packages and modules in Nixpkgs to submit their own changes +when certain criteria are fullfilled. + +# Motivation +[motivation]: #motivation + +Nixpkgs is growing. More people should be able to help out with merging changes. +Merging changes typically requires write permission, however, that gives the +possibility for Nixpkgs-wide changes and decreases the security. + +By allowing maintainers of packages and modules to submit their own changes the +load on members of the @nixpkgs-committers that have write permission group is +reduced. + +# Detailed design +[design]: #detailed-design + +Maintainers of packages and modules will be able to submit their own changes +when the following criteria are met: +1. The label `11.by: package-maintainer` is set by @grahamcofborg +2. The packages/modules are not new. +3. All packages/modules affected need to be maintained by the maintainer. + +When these conditions are met the maintainer can write + + @grahamcofborg merge + +Note that also another bot could be used than @grahamcofborg. + +Criteria 1) is to be met to ensure that it is indeed the maintainer that wants +to submit the changeset. It is important that the label is adjusted with the +requirement set in criteria 3), that is, that all affected derivations, so also +reverse dependencies, are to be maintained by the maintainer. This is to ensure +that maintainers cannot make changes elsewhere in the tree and be permitted to +submit them because they have also made a change in an expression they own. This +also means that this process nearly only applies to leaf packages. Criteria 2) +exists to ensure that at least the initial expression was reviewed by a member +of @nixpkgs-maintainers. + +# Drawbacks +[drawbacks]: #drawbacks + +There are major concers regarding security. First of all, if e.g. due to a bug +in @grahamcofborg it sets the required label by accident, then that could open +up Nixpkgs for other unauthorized changes. + +Second, after the initial expression was approved and submitted, the new +maintainer is free to make whatever changes in that attribute and submit them +without reviewing, making it trivial to include malicious content. +Members of @nixpkgs-committers can also do this, however, membership of that +group is not given directly to anybody. + +# Alternatives +[alternatives]: #alternatives + +1. Hand out write permissions more freely. More people will be able to make changes anymore. +2. Reduce the size and scope of Nixpkgs. Include only core packages and function in Nixpkgs or +a Nixpkgs-core and use e.g. Flakes for leaf packages. + +# Unresolved questions +[unresolved]: #unresolved-questions + +Should the possibility of merging PR's be part of @grahamcofborg or should it be a +separate bot. + +# Future work +[future]: #future-work + +1. The algorithm for deciding whether the label should be set would have to be changed as it is now stricter. +2. Merge functionality would have to be implemented in @grahamcofborg or another bot. \ No newline at end of file From 2f3efd455e6512266358bb013e496e3512cc6af2 Mon Sep 17 00:00:00 2001 From: Frederik Rietdijk Date: Mon, 14 Oct 2019 17:51:02 +0200 Subject: [PATCH 2/3] fix group name --- rfcs/0050-merge-bot-for-maintainers.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/0050-merge-bot-for-maintainers.md b/rfcs/0050-merge-bot-for-maintainers.md index 0036bc7d5..20f454b70 100644 --- a/rfcs/0050-merge-bot-for-maintainers.md +++ b/rfcs/0050-merge-bot-for-maintainers.md @@ -48,7 +48,7 @@ that maintainers cannot make changes elsewhere in the tree and be permitted to submit them because they have also made a change in an expression they own. This also means that this process nearly only applies to leaf packages. Criteria 2) exists to ensure that at least the initial expression was reviewed by a member -of @nixpkgs-maintainers. +of @nixpkgs-committers. # Drawbacks [drawbacks]: #drawbacks @@ -80,4 +80,4 @@ separate bot. [future]: #future-work 1. The algorithm for deciding whether the label should be set would have to be changed as it is now stricter. -2. Merge functionality would have to be implemented in @grahamcofborg or another bot. \ No newline at end of file +2. Merge functionality would have to be implemented in @grahamcofborg or another bot. From 295da37ed3fe4f9005bf73463b5b7cd4a99faaf9 Mon Sep 17 00:00:00 2001 From: Frederik Rietdijk Date: Tue, 15 Oct 2019 14:17:54 +0200 Subject: [PATCH 3/3] changes rfc meeting 2019-10-14 Flow chart needs to be added. --- rfcs/0050-merge-bot-for-maintainers.md | 103 ++++++++++++++++--------- 1 file changed, 68 insertions(+), 35 deletions(-) diff --git a/rfcs/0050-merge-bot-for-maintainers.md b/rfcs/0050-merge-bot-for-maintainers.md index 20f454b70..29c1d033e 100644 --- a/rfcs/0050-merge-bot-for-maintainers.md +++ b/rfcs/0050-merge-bot-for-maintainers.md @@ -3,65 +3,99 @@ feature: merge-bot-for-maintainers start-date: 2019-08-17 author: Frederik Rietdijk co-authors: (find a buddy later to help our with the RFC) -shepherd-team: (names, to be nominated and accepted by RFC steering committee) -shepherd-leader: (name to be appointed by RFC steering committee) +shepherd-team: @aanderse, @globin, @grahamc, @worldofpeace +shepherd-leader: @globin related-issues: (will contain links to implementation PRs) --- # Summary [summary]: #summary -Allow maintainers of packages and modules in Nixpkgs to submit their own changes -when certain criteria are fullfilled. +Allow maintainers of packages in Nixpkgs to submit their own changes +when a trusted reviewer reviewed their change and other criteria are fullfilled. +When the criteria are fullfilled a change can be merged using + + @grahamcofborg merge # Motivation [motivation]: #motivation -Nixpkgs is growing. More people should be able to help out with merging changes. -Merging changes typically requires write permission, however, that gives the -possibility for Nixpkgs-wide changes and decreases the security. +Nixpkgs is growing. Community members want to be able and should be able to help out +with merging changes. Merging changes typically requires write permission, however, +that gives the possibility for Nixpkgs-wide changes and decreases the security. By allowing maintainers of packages and modules to submit their own changes the load on members of the @nixpkgs-committers that have write permission group is reduced. +A new group, trusted reviewers, is introduced to help out with the reviewing +process to improve the reviewing culture by introducing peer-review and making +the whole process more inclusive. + # Detailed design [design]: #detailed-design -Maintainers of packages and modules will be able to submit their own changes -when the following criteria are met: -1. The label `11.by: package-maintainer` is set by @grahamcofborg -2. The packages/modules are not new. -3. All packages/modules affected need to be maintained by the maintainer. +Community members will be able to submit their changes using a bot after a +positive review of both the maintainers of the expressions they modify +("relevant maintainers") as well that of a trusted reviewer. -When these conditions are met the maintainer can write +## Scope - @grahamcofborg merge +- Only packages are considered. Modules are excluded because of how intertwined modules are. +- Only `master` and `staging` branches are considered. In the future release branches may be included as well. + +## Trusted reviewers + +A new trusted reviewers group is introduced, called @nixpkgs-reviewers. This +group is permission-wise positioned between @nixpkgs-maintainers and +@nixpkgs-committers and can be considered a stepping stone towards becoming a +member of @nixpkgs-committers and obtaining push permission. + +This new group is initially implemented with GitHub Teams. In the future this +may be done differently, e.g. when narrower control lists are pursued. + +## Merge bot behavior + +The bot @grahamcofborg will support two new commands, `@grahamcofborg merge` and +`@grahamcofborg stop` for respectively merging a PR and aborting a PR merge. + +### Merging a change + +The following requirements need to be fullfilled for the bot to be able to merge: +1. Target branch of PR is `master` or `staging`. +2. Review is green. A review is green when the relevant maintainers gave a positive review as well as a member of the trusted reviewers. Alternatively, a positive review of a committer is needed. +3. Build is green. This always applies, even when a committer gave a positive review. Furthermore, the builds of all supported platforms need to pass. +4. PR is at the same revision as when the `@ofborg merge` request was done. +5. Amount of rebuilds is smaller than 500 packages unless a committer gave a positive review. That way large rebuilds are supported. + +In the above "relevant maintainers" corresponds to one maintainer for each of +the modified expressions. + +When an expression is modified that has no maintainer, then a committer needs to +approve. This should hopefully also lead to community members taking up a maintainer +role. + +### Aborting a merge + +A merge can be aborted when an `@ofborg stop` is issued by any of: +1. PR author +2. Maintainer of any of the modified expressions +3. Trusted reviewer +4. Committer + +Note a committer can always force a merge by performing the merge without the bot. -Note that also another bot could be used than @grahamcofborg. +### Flow chart -Criteria 1) is to be met to ensure that it is indeed the maintainer that wants -to submit the changeset. It is important that the label is adjusted with the -requirement set in criteria 3), that is, that all affected derivations, so also -reverse dependencies, are to be maintained by the maintainer. This is to ensure -that maintainers cannot make changes elsewhere in the tree and be permitted to -submit them because they have also made a change in an expression they own. This -also means that this process nearly only applies to leaf packages. Criteria 2) -exists to ensure that at least the initial expression was reviewed by a member -of @nixpkgs-committers. +TODO # Drawbacks [drawbacks]: #drawbacks -There are major concers regarding security. First of all, if e.g. due to a bug -in @grahamcofborg it sets the required label by accident, then that could open -up Nixpkgs for other unauthorized changes. +There are major concers regarding security. First of all, @grahamcofborg will +have permission to push which can make it a more interesting target, resulting +in potentially unauthorized changes to Nixpkgs. -Second, after the initial expression was approved and submitted, the new -maintainer is free to make whatever changes in that attribute and submit them -without reviewing, making it trivial to include malicious content. -Members of @nixpkgs-committers can also do this, however, membership of that -group is not given directly to anybody. # Alternatives [alternatives]: #alternatives @@ -74,10 +108,9 @@ a Nixpkgs-core and use e.g. Flakes for leaf packages. [unresolved]: #unresolved-questions Should the possibility of merging PR's be part of @grahamcofborg or should it be a -separate bot. +separate bot. For now it is assumed it would be part of @grahamcofborg. # Future work [future]: #future-work -1. The algorithm for deciding whether the label should be set would have to be changed as it is now stricter. -2. Merge functionality would have to be implemented in @grahamcofborg or another bot. +1. Merge functionality would have to be implemented in @grahamcofborg or another bot.