doc: add guide for backporting PRs #11099
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # 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 | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| | Release Line | Staging Branch | | ||
| | ------------ | -------------- | | ||
| | `v7.x` | `v7.x-staging` | | ||
| | `v6.x` | `v6.x-staging` | | ||
| | `v4.x` | `v4.x-staging` | | ||
|
|
||
| ## What needs to be backported? | ||
|
|
||
| When a release is being prepared, the releaser attempts to cherry-pick a | ||
| certain set of commits from the master branch to the release staging branch. | ||
|
There was a problem hiding this comment. "certain" is vague, but elaborated on below, maybe state that criteria for consideration depends on the the target version (current or LTS)? There was a problem hiding this comment. actually, not elaborated on below, I don't think. There was a problem hiding this comment. Yea, I hear ya. It is a bit difficult to find the middle ground between spilling my guts on cutting releases vs the actual intention of the guide. I'll improve this. There was a problem hiding this comment. I think ideally this would link to the part of releases.md which talks about the criteria, not sure if there is currently anything in there to link to though. There was a problem hiding this comment. There is not anything in the releases.md document regarding this. I'll work on documenting that more specifically as well. There was a problem hiding this comment. I don't think that level of information belongs in this doc, if we can actually figure out what "certain" means then it belongs elsewhere anyway. |
||
| The criteria for consideration depends on the target version (Current vs. LTS). | ||
| If a cherry-pick does not land cleanly on the staging branch, the releaser | ||
| will mark the pull request with a particular label for that release 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 (currently v7.x) is much more lenient than the LTS | ||
|
There was a problem hiding this comment. s/(currently v7.x)/(v7.x as of Feb 2017) There was a problem hiding this comment. Please drop the There was a problem hiding this comment. maybe just drop the (currently v7.x) as it is quickly going to be out of date. |
||
| release lines in what can be landed. Our LTS release lines | ||
| (currently v4.x and v6.x) require that commits live in a Current release for at | ||
| least 2 weeks before backporting. Please see the [`LTS Plan`][] for more | ||
|
There was a problem hiding this comment. As I understand it, the two week rule doesn't apply to doc fixes and infrastructure changes (things that don't change what we ship). This is probably a question for @nodejs/lts, but either way we should probably be explicit here. Refs: #11351 (comment) and nodejs/build#613 (comment) There was a problem hiding this comment. The length of time is not set in stone as is based on the potential impact. A doc change has no impact on runtime stability so those, as long as they are relevant to the version, can pretty much land any time. A code change, however, should go through at least one There was a problem hiding this comment. Based on what James said, I think we should remove this part. How does one know what sensitive parts of the codebase are when submitting a backport PR? It's hard to document something that isn't concrete imo. There was a problem hiding this comment. It's definitely a grey area, but people do talk about the two week rule, so I think we should try and document it somehow, even if we have to say that it's quite subjective. |
||
| 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. Might be easier to just say something like: git clone -o upstream https://github.com/nodejs/node.git && cd node
git remote add fork [email protected]:$USER/node.git
git fetch --allI'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. sure, although I would think that There was a problem hiding this comment. I like to be specific in instructions, origin can end up being There was a problem hiding this comment. 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. Okay fair enough, makes sense to keep it consistent then. |
||
| git checkout v7.x-staging | ||
| git pull -r 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. | ||
|
|
||
| ```shell | ||
| git cherry-pick $SHA # Use your commit hash | ||
| ``` | ||
|
|
||
|
There was a problem hiding this comment. 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. I'll clarify. Yes, this guide is for when the releaser cannot cleanly cherry-pick the commit and requests a backport. |
||
| * The commit message should be as close as possible to the commit message on the | ||
|
There was a problem hiding this comment. 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. 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. To be more specific, the original message should be preserved, that does not preclude adding additional information There was a problem hiding this comment. 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. 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. In the case of a backport, I don't think it should. I'll clarify that. |
||
| are not present in the targeted release line. | ||
| * Push the changes to your fork and open a pull request. | ||
|
There was a problem hiding this comment. There should probably be a:
above this line, see #11797 (comment) |
||
| * Be sure to target the `v7.x-staging` branch in the pull request. | ||
|
|
||
| ### 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 include the text `Backport of #<pull request number>` in the | ||
| pull request description. This will link to the original pull request. | ||
|
|
||
| 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. Is there a reason you used |
||
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.
'Backport' to make it consistently titlecase eh?