-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: add guide for backporting PRs #11099
Changes from 9 commits
f153855
837bcb9
aa40433
444359c
066e617
b06d19f
4db2488
2782890
89e7bfb
fc434b2
a374208
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,122 @@ | ||
# How to Backport a Pull Request to a Release Line | ||
|
||
## Staging branches | ||
|
||
Each release line has a staging branch that the releaser will use as a scratch | ||
pad while preparing a release. The branch name is formatted as follows: | ||
`vN.x-staging` where `N` is the major release number. | ||
|
||
### Active staging branches | ||
|
||
| Release Line | Staging Branch | | ||
| ------------ | -------------- | | ||
| `v7.x` | `v7.x-staging` | | ||
| `v6.x` | `v6.x-staging` | | ||
| `v4.x` | `v4.x-staging` | | ||
|
||
## What needs to be backported? | ||
|
||
If a cherry-pick from master does not land cleanly on a staging branch, the | ||
releaser will mark the pull request with a particular label for that release | ||
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. should we be more specific regarding the label here? 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. Yes please, specificity is important 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. @MylesBorins I think if we see this (at least initially) as a basic guide for people raising backport PRs, then we don't need to specify this yet (all they care about is the comment). 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. @gibfahn should we specify the label? 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. @MylesBorins I think that should probably be done in #12431 |
||
line, specifying to our tooling that this pull request should not be included. | ||
The releaser will then add a comment that a backport is needed. | ||
|
||
## What can be backported? | ||
|
||
The "Current" release line is much more lenient than the LTS release lines in | ||
what can be landed. Our LTS release lines (v4.x and v6.x as of March 2017) | ||
require that commits mature in a Current release for at least 2 weeks before | ||
they can be landed on staging. If the commit can not be cherry-picked from | ||
master a manual backport will need to be submitted. Please see the [LTS Plan][] | ||
for more information. After that time, if the commit can be cherry-picked | ||
cleanly from master, then nothing needs to be done. If not, a backport pull | ||
request will need to be submitted. | ||
|
||
## How to submit a backport pull request | ||
|
||
For these steps, let's assume that a backport is needed for the v7.x release | ||
line. All commands will use the v7.x-staging branch as the target branch. | ||
In order to submit a backport pull request to another branch, simply replace | ||
that with the staging branch for the targeted release line. | ||
|
||
* Checkout the staging branch for the targeted release line | ||
* Make sure that the local staging branch is up to date with the remote | ||
* Create a new branch off of the staging branch | ||
|
||
```shell | ||
# Assuming your fork of Node.js is checked out in $NODE_DIR, | ||
# the origin remote points to your fork, and the upstream remote points | ||
# to git://github.com/nodejs/node | ||
cd $NODE_DIR | ||
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. Might be easier to just say something like: git clone -o upstream https://github.com/nodejs/node.git && cd node
git remote add fork git@github.com:$USER/node.git
git fetch --all I'd find it easier to understand, and it doubles as a guide for someone not sure about setting up multiple remotes. 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. sure, although I would think that 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 like to be specific in instructions, origin can end up being 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 I'm a lot more used to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-1-fork 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. Okay fair enough, makes sense to keep it consistent then. |
||
# Fails if you already have a v7.x-staging | ||
git branch v7.x-staging upstream/v7.x-staging | ||
git checkout v7.x-staging | ||
git reset --hard upstream/v7.x-staging | ||
# We want to backport pr #10157 | ||
git checkout -b backport-10157-to-v7.x | ||
``` | ||
|
||
* After creating the branch, apply the changes to the branch. The cherry-pick | ||
will likely fail due to conflicts. In that case, you will see something this: | ||
|
||
```shell | ||
# Say the $SHA is 773cdc31ef | ||
$ git cherry-pick $SHA # Use your commit hash | ||
error: could not apply 773cdc3... <commit title> | ||
hint: after resolving the conflicts, mark the corrected paths | ||
hint: with 'git add <paths>' or 'git rm <paths>' | ||
hint: and commit the result with 'git commit' | ||
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. isn't the correct thing here to do |
||
``` | ||
|
||
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. At this point do we need to say what you should expect to see and how to fix it up? I assume that following this guide is only needed if the cherry-pick is going to fail (I'm also assuming that $SHA is for the original commit that went into master) 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'll clarify. Yes, this guide is for when the releaser cannot cleanly cherry-pick the commit and requests a backport. |
||
* Make the required changes to remove the conflicts, add the files to the index | ||
using `git add`, and then commit the changes. That can be done with | ||
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 delete |
||
`git cherry-pick --continue`. | ||
* The commit message should be as close as possible to the commit message on 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. AFAIK I have never changed the original commit message. You can add content to the end of it... but the original message should not chage 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 are definitely cases where the commit message should change. Especially when the original commit relies on something that is breaking. 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. To be more specific, the original message should be preserved, that does not preclude adding additional information 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. To be more specific, the original message should be preserved, that does not preclude adding additional information |
||
master branch, unless the commit has to be different due to dependencies that | ||
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. If I understand correctly, this means the metadata(PR urls, reviewers, etc) should remain the same as the original commit? 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. In the case of a backport, I don't think it should. I'll clarify that. |
||
are not present in the targeted release line. The only exception is that the | ||
metadata from the original commit should be removed. If a backport is | ||
required, it should go through the same review steps as a commit landing | ||
in the master branch. | ||
* Make sure `make -j4 test` passes | ||
* Push the changes to your fork and open a pull request. | ||
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. There should probably be a:
above this line, see #11797 (comment) |
||
* Be sure to target the `v7.x-staging` branch in the pull request. | ||
|
||
Below are examples of an original commit message and a backport commit message. | ||
|
||
In this example, https://github.com/nodejs/node/pull/1234 is the original pull | ||
request and https://github.com/nodejs/node/pull/5678 is the backport. | ||
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. These URL dummies become real URLs and get to be confusing as absolutely different PRs. 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. @gibfahn my other comment was because of this. If the PR-URL is correct, then this sentence is a bit confusing because PR 5678 doesn't appear in the example |
||
|
||
Original: | ||
|
||
``` | ||
lib: make something faster | ||
|
||
Switch to using String#repeat to improve performance. | ||
|
||
PR-URL: https://github.com/nodejs/node/pull/1234 | ||
``` | ||
|
||
Backport: | ||
|
||
``` | ||
lib: make something faster | ||
|
||
Switch to using String#repeat to improve performance. | ||
|
||
PR-URL: https://github.com/nodejs/node/pull/5678 | ||
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. This is not accurate. we maintain the original PR-URL |
||
``` | ||
|
||
### Helpful Hints | ||
|
||
* Please include the backport target in the pull request title in the following | ||
format: `(v7.x backport) <commit title>` | ||
* Ex. `(v4.x backport) process: improve performance of nextTick` | ||
* Please check the checkbox labelled "Allow edits from maintainers". | ||
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. a++ |
||
This is the easiest way to to avoid constant rebases. | ||
|
||
In the event the backport pull request is different than the original, | ||
the backport pull request should be reviewed the same way a new pull request | ||
is reviewed. When each commit is landed, the new reviewers and the new PR-URL | ||
should be used. | ||
|
||
[LTS Plan]: https://github.com/nodejs/LTS#lts-plan |
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.
Might be best to avoid embedding "current" versions in here as they won't stay current long and this doc would need to get updated constantly.