-
-
Notifications
You must be signed in to change notification settings - Fork 161
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-0053: defining pull-request workflow #53
Changes from 17 commits
426e923
8bc00aa
5dbdf06
14f7a12
d5f78fa
07bfbec
d44a6cc
a731644
3682c00
1087e41
7d9c905
df31178
11bbd1c
2d2a28a
4a730fc
268a343
a4a63ee
9d9d570
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,127 @@ | ||
--- | ||
feature: pull-request workflow | ||
start-date: 2019-09-28 | ||
author: Ingolf Wagner (@mrVanDalo) | ||
co-authors: lassulus (@lassulus) | ||
shepherd-team: (names, to be nominated and accepted by RFC steering committee) | ||
shepherd-leader: (name to be appointed by RFC steering committee) | ||
related-issues: | ||
--- | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Pull request on GitHub are the main way we improve our code base. | ||
This document should settle everything that needs to be done | ||
to bring code upstream in the | ||
[Nixpkgs Repository](https://github.com/nixos/nixpkgs/). | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Eliminate questions from contributors and maintainers | ||
about what should be done next and who should do it. | ||
This is not a new approach, it is more a settlement | ||
on how we do it now. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
Define all steps of a pull request. | ||
Use Roles to define responsibilities in every step. | ||
|
||
Terms like **SHOULD** and **MUST** are defined in | ||
[IETF RFC 2119](https://tools.ietf.org/html/rfc2119). | ||
|
||
## Roles | ||
[roles]: #roles | ||
|
||
Everybody involved in the process of contributing has one or multiple | ||
of the following roles | ||
|
||
* *Contributor* is the person proposing the pull request | ||
* *Bot* is a bot that provides automated feedback | ||
* *Reviewer* is any person that reviews the pull request | ||
(for example a member of [NixOS/nixpkgs-maintainers](https://github.com/orgs/NixOS/teams/nixpkgs-maintainers)) | ||
* *Merger* is any person with merge privileges | ||
|
||
The responsibilities of these roles are defined in the rest of this RFC. | ||
|
||
## States of a pull request | ||
[state]:#states | ||
|
||
This diagram defines all states of a pull request, | ||
and their transitions to other states. | ||
|
||
![pull-request state](0053-pull-request-workflow/pull-request-states.svg) | ||
|
||
## Responsibilities and Actions | ||
[responsibilities]:#responsibilities | ||
|
||
The responsibilities of every role is defined by the following diagram: | ||
|
||
![pull-request activity](0053-pull-request-workflow/pull-request-roles.svg) | ||
|
||
## About pull requests | ||
|
||
### Packages | ||
|
||
* Contributors **SHOULD** evaluate and signal that a backport is necessary. | ||
* The [NixOS/backports team](https://github.com/orgs/NixOS/teams/backports) | ||
**SHOULD** be pinged in situations that are unclear. | ||
* Backport pull requests **MUST** be linked to the original pull requests (using `git cherry-pick -x`). | ||
* [NixOS/nixpkgs-maintainers](https://github.com/orgs/NixOS/teams/nixpkgs-maintainers) | ||
and | ||
[NixOS/backports](https://github.com/orgs/NixOS/teams/backports) | ||
can deny the backport. | ||
* After the pull request to `master`, `staging` or `staging-next` is merged, | ||
the backport pull request is created | ||
|
||
### Modules | ||
|
||
* Modules **SHOULD** have tests | ||
* Reviewers **SHOULD** encourage contributors to write tests for new modules | ||
* Module changes **SHOULD NOT** be backported. | ||
* For example a module change that's needed due to a package backport is a valid exception | ||
|
||
## What to backport | ||
|
||
* Security patches which aren't major updates | ||
* If a security patch is a major upgrade, try and find patches to our | ||
current version which accomplish the same goal. Apply the major | ||
update to master, and the patches to stable. | ||
* Bug fixes to applications which, again, aren't major updates. | ||
Generally be cautious about these. | ||
* Any updates when the current stable version is utterly broken. A | ||
key example of this is Spotify, who regularly breaks their old | ||
versions. | ||
* Extremely security-sensitive software, in particular Chrome, | ||
Chromium, Firefox, Thunderbird, and of course the kernel. | ||
|
||
### Don't backport if ... | ||
|
||
* the patch is just for Darwin, they use nixpkgs-unstable not a | ||
stable branch. | ||
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. Who is "they" in this context? There are hydra jobsets that test and release the 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 got inlined this from https://gist.github.com/grahamc/c60578c6e6928043d29a427361634df6#what-to-backport . I guess it is meant that darwin nix users use the unstable nixpkgs branch. 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. Seems like outdated information? The stable darwin channels were added after it was written. 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. Good to know, I will take this into account in my pull-request for the nixos/nixpkgs. |
||
|
||
## Links | ||
|
||
* [How to write module tests](https://nixos.org/nixos/manual/index.html#sec-nixos-tests) | ||
* [Contribution guidelines](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md) | ||
* [What to backport](https://gist.github.com/grahamc/c60578c6e6928043d29a427361634df6#what-to-backport) | ||
* [RFC26 : staging workflow](./0026-staging-workflow.md) | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
* The pull request of a backport should be created by the bot. | ||
But if that is the case, the original contributor might not be able | ||
to make changes on the branch behind the pull request. | ||
* Backports without changes in `master` are not discussed. | ||
for example security patches that only affect older versions in stable. | ||
|
||
# Future work | ||
[future]: #future-work | ||
|
||
* The pull request template needs an option "backport 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. I am a bit concerned that our template becomes too complex. We already have 10 checkboxes in our current template. 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. Where else would you put it? 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 quite sure either. Maybe we can replace a different checkbox that is not too important. i.e. the closure size checkbox, which is checked very rare. |
||
* Add a link to this document in the | ||
[Contribution Guidelines](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
.history |
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.
It is a common practice that the merger, will backport changes without having a backport pull request.
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.
The pull-request is created in by the contributor in the 5th line, right below the bar.