-
-
Notifications
You must be signed in to change notification settings - Fork 162
RFC-0053: defining pull-request workflow #53
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
Changes from 1 commit
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,93 @@ | ||
| --- | ||
| 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. | ||
|
|
||
| # 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. | ||
| This RFC is based on my experiences and talks I had | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
| with other contributors and maintainers. | ||
|
|
||
| # Detailed design | ||
| [design]: #detailed-design | ||
|
|
||
| Define all steps of a Pull-Request. | ||
| Use Roles to define responsibilities in every step. | ||
|
|
||
| ## Roles | ||
| [roles]: #roles | ||
|
|
||
| Everybody involved in the process of contributing has one or multiple | ||
| of the following roles | ||
|
|
||
| * Contributor | ||
| * Bot | ||
| * Reviewer | ||
| * Maintainer (has merge privileges) | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
|
|
||
| The responsibilities of theses roles are defined in the rest of this RFC. | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## States of a Pull-Request | ||
| [state]:#states | ||
|
|
||
| This diagram defines all states of a Pull-Request, | ||
| and their transitions to other states. | ||
|
|
||
|  | ||
|
|
||
| ## Responsibilities and Actions | ||
| [responsibilities]:#responsibilities | ||
|
|
||
| The responsibilities of every role is defined by the following diagram: | ||
|
|
||
|  | ||
|
Member
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. It is a common practice that the merger, will backport changes without having a backport pull request.
Author
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. The pull-request is created in by the contributor in the 5th line, right below the bar. |
||
|
|
||
| ## About Pull-Request | ||
|
|
||
| ### Packages | ||
|
|
||
| * contributor must decide if a Backport is necessary | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
| * after the Pull-Request to master is merged the Backport Pull-Request is created | ||
|
Member
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. master or staging or staging-next
Member
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. Linking #26 would be also good. |
||
| * Backport Pull-Requests must be linked to the original Pull-Requests in master | ||
|
Member
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. may include in parentheses (using
Member
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. Is there any change to tell GitHub to add the Also, what about "manual" backports (commiters cherry-picking patches directly to
Author
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've seen this tag, but I did not know who is setting that in which situation. This is also one reason why I created this RFC. What are the rules behind this tag? There is also a I would love to put the meaning and the ruling behind theses tags in this RFC.
Author
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 guess by "manual" you mean without a pull-request. This is something I don't know of at all. The only thing I heard from people is that they don't like it to be done.
Author
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 will add this, but I had more the pull-request-id in mind. |
||
| * reviewer and maintainer can deny the Backport | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### Modules | ||
|
|
||
| * modules should have tests | ||
| * new modules must have tests | ||
|
Member
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. Tbh I'm not sure if that rule is too strict: I remember several cases where I actually wanted to write a test, but failed to find a suitable approach. One example that comes to my mind right now would be the autorandr module where you'd have to simulate several monitors in the test VM. I'd rather go with "reviewers should encourage contributors to write tests for new modules" to make it clear that we want to have tests but if there's no way around this, it's still fine to skip that task.
Author
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. True, it is not possible to create module tests for everything.
Implies it is the responsibility of the reviewer to make sure tests are written (if possible). I can live with that.
Member
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 guess another approach is to say that «new modules should have tests or an explicit description of problems with testing (e.g. hardware interaction)» |
||
| * modules should not be Backported | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
| * Backports of modules must be pretty good argued. | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
|
|
||
| Modules are written by people who use NixOS every day. | ||
| So writing a test should be part of their skill-set. | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
|
|
||
| # 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. | ||
|
Member
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. Random idea: how about something like
Author
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. The problem is the bot would create the pull-request and the contributor must be able to edit it.
Member
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. And the main catch is what happens if person A creates a PR and person B requests a backport… |
||
| * Backports without changes in master are not discussed. | ||
|
Member
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 the PR should include this. In case the update cannot go through
Author
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. So this would follow the rules of a normal pull-request, just to a different branch?
Member
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. And with an extra sentence about the reasons it's irrelevant for
Author
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. @FRidh I think I should also put some more information about the branch structure we have. Bad thing is that I don't know the way we use branches. I know the These branches look like I should write about them:
Did I miss one?
Author
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. Ah I found #26 I will read it and come back to this comment. |
||
| 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?" | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
| * Add or link a description on how to write module tests in [the wiki](https://nixos.wiki/wiki/NixOS_Modules) | ||
|
mrVanDalo marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| .history |
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| with import <nixpkgs> {}; | ||
|
|
||
| stdenv.mkDerivation rec { | ||
|
|
||
| name = "nixos-diagrams"; | ||
|
|
||
| env = buildEnv { | ||
| name = name; | ||
| paths = buildInputs; | ||
| }; | ||
|
|
||
| buildInputs = [ | ||
| feh | ||
| plantuml | ||
| yed | ||
| ]; | ||
|
|
||
| shellHook = '' | ||
| HISTFILE=${toString ./.}/.history | ||
| ''; | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.