RFC-0053: defining pull-request workflow #53
Conversation
rfcs/0053-pull-request-workflow.md
Outdated
| ### Packages | ||
|
|
||
| * contributor must decide if a Backport is necessary | ||
| * after the Pull-Request to master is merged the Backport Pull-Request is created |
There was a problem hiding this comment.
master or staging or staging-next
There was a problem hiding this comment.
Linking #26 would be also good.
rfcs/0053-pull-request-workflow.md
Outdated
| * 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. |
There was a problem hiding this comment.
I think the PR should include this. In case the update cannot go through master/staging/staging-next it should be made directly against release-YY.MM or staging-YY.MM.
There was a problem hiding this comment.
So this would follow the rules of a normal pull-request, just to a different branch?
There was a problem hiding this comment.
And with an extra sentence about the reasons it's irrelevant for master
There was a problem hiding this comment.
@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 master and release branches. But I don't know what the other branches in https://github.com/nixos/nixpkgs/ are for and how they interact with each other.
These branches look like I should write about them:
- security-updates : I have no clue how this branch work
- staging-next : I guess this is the branch that will be the next release. But what is
stagingthan for? I guess your pull-request goes tomaster, and than you backport tostaging-nextandstaging. - staging : I guess this is the branch that will be the next release. But what is
staging-nextthan for? I guess your pull-request goes tomaster, and than you backport tostaging-nextandstaging. - master : I put my pull-request always against master.
staging-yy.mm: Hmm we havestagingandstaging-nextwhy do we need astaging-19.09?release-yy.mm: I guess this is the branch which tracks the so called stable channels (nixpkgs-19.03, nixpkgs-19.09, ... ).
Did I miss one?
There was a problem hiding this comment.
Ah I found #26 I will read it and come back to this comment.
|
Would explicitly mentioning the option of pure-Tests PR serve as a kind of encouragement? |
rfcs/0053-pull-request-workflow.md
Outdated
| ### Modules | ||
|
|
||
| * modules should have tests | ||
| * new modules must have tests |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
True, it is not possible to create module tests for everything.
reviewers should encourage contributors to write tests for new modules
Implies it is the responsibility of the reviewer to make sure tests are written (if possible). I can live with that.
There was a problem hiding this comment.
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)»
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * contributor must decide if a Backport is necessary | ||
| * after the Pull-Request to master is merged the Backport Pull-Request is created | ||
| * Backport Pull-Requests must be linked to the original Pull-Requests in master |
There was a problem hiding this comment.
Is there any change to tell GitHub to add the 9.needs: port to stable in case of a backport by an unprivileged contributor?
Also, what about "manual" backports (commiters cherry-picking patches directly to release-yy.mm)?
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * 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. |
There was a problem hiding this comment.
Random idea: how about something like @ofborg backport 19.09?
There was a problem hiding this comment.
The problem is the bot would create the pull-request and the contributor must be able to edit it.
There are two backport possibilities
- cherry pick
- new pull-request, because the software differs in the stable branches.
There was a problem hiding this comment.
And the main catch is what happens if person A creates a PR and person B requests a backport…
|
Thanks @Ma27 and @FRidh for the quick review. |
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * contributor must decide if a Backport is necessary | ||
| * after the Pull-Request to master is merged the Backport Pull-Request is created | ||
| * Backport Pull-Requests must be linked to the original Pull-Requests in master |
There was a problem hiding this comment.
Is there any change to tell GitHub to add the
9.needs: port to stablein case of a backport by an unprivileged contributor?
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 8: has port to stable.
I would love to put the meaning and the ruling behind theses tags in this RFC.
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * contributor must decide if a Backport is necessary | ||
| * after the Pull-Request to master is merged the Backport Pull-Request is created | ||
| * Backport Pull-Requests must be linked to the original Pull-Requests in master |
There was a problem hiding this comment.
Also, what about "manual" backports (commiters cherry-picking patches directly to
release-yy.mm)?
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.
rfcs/0053-pull-request-workflow.md
Outdated
| ### Modules | ||
|
|
||
| * modules should have tests | ||
| * new modules must have tests |
There was a problem hiding this comment.
True, it is not possible to create module tests for everything.
reviewers should encourage contributors to write tests for new modules
Implies it is the responsibility of the reviewer to make sure tests are written (if possible). I can live with that.
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * 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. |
There was a problem hiding this comment.
The problem is the bot would create the pull-request and the contributor must be able to edit it.
There are two backport possibilities
- cherry pick
- new pull-request, because the software differs in the stable branches.
rfcs/0053-pull-request-workflow.md
Outdated
| * 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. |
There was a problem hiding this comment.
So this would follow the rules of a normal pull-request, just to a different branch?
rfcs/0053-pull-request-workflow.md
Outdated
|
|
||
| * contributor must decide if a Backport is necessary | ||
| * after the Pull-Request to master is merged the Backport Pull-Request is created | ||
| * Backport Pull-Requests must be linked to the original Pull-Requests in master |
There was a problem hiding this comment.
may include in parentheses (using
git cherry-pick -x)
I will add this, but I had more the pull-request-id in mind.
|
@7c6f434c what do you mean by that?
You mean I should create a special rule-set for pull-requests that are only adding tests? |
|
I guess… The main point I would put there is «don't forget to mention the maintainers of key packages and modules», as the bot doesn't handle this. (Maybe the same is actually true for adding modules…) |
|
Nice writeup. One question about modules. Would it be wise to specify when they should be backported? Right now it says “SHOULD NOT”, but off the top of my head the only situation I can imagine where I would want a module backported is if it’s security-related. |
|
@carlosdagos In #53 (comment) , I try to come up with examples when it makes sense to backport modules. If you can remember a Pull-Request please let me know.
@Ma27 pointed out
I can't think of any other reason so far. But SHOULD NOT does not mean MUST NOT. |
|
Hi. Could there be something like “bots SHOULD NOT open pull requests”? Pull requests are calling humans for reviews. Unless there is a fully automated reviewing and merging process that could be triggered by bots, opening a pull request is the privilege of reasonable beings. |
|
@vbgl a lot of Pull-Requests already created by bots. For example : https://github.com/r-ryantm |
|
@vbgl I disagree. We currently have automated processes creating pull requests upgrading packages for e.g. security updates to good effect, saving the time and attention of humans. However, these updates in my opinion should still be checked manually by a human in most cases. |
|
I'm definitely in favour of continuing to allow bots to open PRs, like @r-ryantm does. Saves lots of effort. In an ideal world, bots would do all of the work of finding, building, testing and PRing new versions, and humans would just approve that the changelog looks good and that the upgrade is desired. |
|
I too would like to throw in my support for bots, they should be allowed to open PRs. I also have some grammatical advice for this document.
|
mrVanDalo
force-pushed
the
rfcs-pull-request-workflow
branch
from
a3be8dc to
1087e41
Compare
There was a problem hiding this comment.
This RFC seems to me much more about the backport workflow than the PR workflow. But I disagree that deciding whether a PR should be backported is the role of the contributor alone. The contributor may have no idea. The reviewer may also have an opinion.
FTR, if there is a need for a backport bot, then people should be aware that there are already a number of them available, so it might not be needed to reimplement one.
rfcs/0053-pull-request-workflow.md
Outdated
| * `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 |
There was a problem hiding this comment.
In my experience "merger" usually means the resulting of a merge and rarely designate a person. The standard terms that could be used instead are "integrator" and "committer" (the second being less clearly related to PRs but clearer about the required privileges).
Co-Authored-By: Théo Zimmermann <[email protected]>
@zimbatm I'm ok with that. I can put it in the documentation once all the discussions are settled. I guess looking at the diagrams is a bit harder in the docbook manual. |
@Zimmi48 What are you missing in the pull request workflow?
@Zimmi48 Of course the reviewer can also arrange a backport, but this would increase the workload of reviewers. I'm not against it, just mention it. |
I don't know exactly, but as a maintainer without much experience, I don't see anything really informative in this workflow. It's probably too much of a bird view to my taste. |
…s into rfcs-pull-request-workflow
|
I added some changes from the discussions. Thanks to everybody for your help. |
|
Just a small nitpick, the Also, should there be some kind of policy about force pushing vs creating new commits during code review, or should we just use whatever works? |
|
(looking at the SVG) I think the main part of the PR workflow that needs clarifications is depth of testing, and sometimes handling tradeoffs in the known breakages. Saying «functionality of all affected packages» doesn't help much in any interesting case (like Also, has I missed something or is there nothing at all about upstream Changelog in the RFC? I guess this text could be a fine first introduction to the process if put in the documentation, but putting this as an established policy is risky |
Very much true. This slips through some times, sorry. I'm trying.
You force pushes to you own branch. Or more what belongs to a commit and what should be split in multiple commits? |
|
You force pushes to you own branch. Or more what belongs to a commit and what should be split in multiple commits?
No, this was a question about iterating the fixes in additional commits before squashing and force-pushing
|
| ### 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.
Who is "they" in this context? There are hydra jobsets that test and release the nixpkgs-xx.xx-darwin channels for the stable branch on darwin, are these not intended for use?
There was a problem hiding this comment.
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.
Seems like outdated information? The stable darwin channels were added after it was written.
There was a problem hiding this comment.
Good to know, I will take this into account in my pull-request for the nixos/nixpkgs.
I typically ask contributors to please use additional commits for the points I review. |
For PR flow I've recently settled on pushing a bunch of commits called "fixup xxx Description," then rebasing, then pushing again, which leads to both a clean history and a nice review flow. |
|
@sauyon Yep that's exactly what I suggest. Git also has the fixup commit feature which is very handy for interactive rebases. |
We might also consider adding https://reviewable.io integration to our PRs. I found it an excellent review tool for my open-source projects, because it allows to:
Comments made there reflect back into Github, and the other way around, so any reviewer may choose to use it or not. Github review emails work as usual. Like Github, it's free for open-source projects. For an example, here's a screenshot of me and @qrilka reviewing a PR iterated with force-pushes on my project nh2/hatrace#49:
Here you can see how it tracks how far the reviewers have read across the 7 different force-pushed versions of the PR. As a reviewer, you can display either only the changes since you last looked, or between any force-pushed revisions you manually choose. I found that this approach saves time because it saves a review roundtrip: People can directly force-push their respective best versions, instead of first pushing fixup commits and then post-review squashing them. |
|
I've used Reviewable in the past and it's an excellent tool, so I second that recommendation. |
|
I think that this RFC, as it stands, is far too intimidating to be useful for new contributors — an extremely long list of rules and complicated diagrams and SHOULD/MUST-style language is going to put people off contributing at all, and the ease of contributing to Nixpkgs compared to other package repositories is one of its core strengths. I think a description of how things usually work for new contributors in the manual would be good. This feels like more like imposing how things should work, and I don’t think we need rules like that. The RFC also claims to be describing how things currently work rather than proposing something new, but the flow it describes doesn’t match with my experience as a committer. For one thing, it talks at length about backport PRs, but in my experience at least backport PRs are much rarer than the committee who merged just cherry-picking directly to the appropriate branches. I think this works well for most cases. Backport are usually uncontroversial, but they can already be annoying to do, so making it more effort by making them go through PRs would, I think, result in fewer backports. I think you should take some time to figure out what it is you want to do here — if it’s just to describe things as they are, make a pull request to the manual and we’ll help make sure it’s accurate. If you actually want to change how things work, then it should be done as an RFC that doesn’t suggest that it’s entirely descriptive. |
What does such an integration consist of? From what you’ve described, I don’t see anything that would even be visible to anybody but the person using it for review? |
|
@alyssais Agree with you completely here, was waiting for you to state it 😄 . I also don't see what is a Though I don't disagree with the inspiration, which I'm seeing is that documentation on how to submit a pull request to nixpkgs and who's going to review it and such would all be helpful (github teams, ofborg). All things I've gone out of my way to explain to people individually. In it's current form it's not designed for that kind of consumer of information though. But all in all, I'm glad these discussions happened because an RFC was opened. |
@alyssais See what I mentioned above:
Concrete example: nh2/hatrace#41 (review) When you post review comments in Reviewable, they appear on the PR as comments, and you can click to jump directly into reviewable. That means they will be visible to anybody subscribed to the PR or reading through its updates, as usual. |
|
Ok, I would close this pull request than, as everybody agrees (including me) this pull request should never be merged. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/88 |
Define our current Pull-Request workflow.
It is meant to be as a guideline for contributors and maintainers which can be referenced in a pull-request to clear discussions about the process.
I find it necessary to define theses things because the NixOS community is growing a lot in the last years. Which can be seen by the amount of people who contribute and the number of pull-requests. Having well defined workflows reduces errors, simplifies on-boarding and provides a proper basis for improvement discussions.
Because github is not properly showing the resulting file, and the SVGs in them, please have a look at them here https://github.com/mrVanDalo/rfcs/blob/rfcs-pull-request-workflow/rfcs/0053-pull-request-workflow.md