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
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions rfcs/0050-merge-bot-for-maintainers.md
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
Copy link
Member

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:

Suggested change
Allow maintainers of packages in Nixpkgs to submit their own changes
Allow maintainers of packages in Nixpkgs to merge their own PRs

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
Copy link
Member

@Zimmi48 Zimmi48 Oct 15, 2019

Choose a reason for hiding this comment

The 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.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think stop is too generic (stop build?). Maybe dontmerge?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe abort-merge or cancel-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.
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The 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.
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?

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.
Copy link
Member Author

Choose a reason for hiding this comment

The 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.