-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
--- | ||
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: @aanderse, @globin, @grahamc, @worldofpeace | ||
shepherd-leader: @globin | ||
related-issues: (will contain links to implementation PRs) | ||
--- | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
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. 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 | ||
|
||
Community members will be able to submit their changes using a bot after a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, you say community members instead of maintainers (this may be accurate, but should be reflected elsewhere). |
||
positive review of both the maintainers of the expressions they modify | ||
("relevant maintainers") as well that of a trusted reviewer. | ||
|
||
## Scope | ||
|
||
- 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
|
||
### 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewed may mean different things: are approved reviews maintained when a PR is updated? Currently, they are not because @grahamofborg requests new reviews from maintainers after each push, but for what I know, this could be a bug, or a design choice that is not set in stone. This RFC should be specific. It could be that it needs positive reviews from relevant maintainers on this version or a previous one, and a review from a trusted reviewer on the last version. |
||
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. | ||
|
||
### Flow chart | ||
|
||
TODO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. intend to add a flow chart and possibly a table |
||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
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. | ||
|
||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
1. Hand out write permissions more freely. More people will be able to make changes anymore. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
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. | ||
FRidh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
Should the possibility of merging PR's be part of @grahamcofborg or should it be a | ||
separate bot. For now it is assumed it would be part of @grahamcofborg. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure whether we decided to actually do this as part of ofborg or in a separate bot. |
||
|
||
# Future work | ||
[future]: #future-work | ||
|
||
1. Merge functionality would have to be implemented in @grahamcofborg or another bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is what is meant, then it's better to clarify it: