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,124 @@ | ||
| # 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. this is a bit confusing in the way it is selectively using / switching backporting + cherry-picking. Perhaps
|
||
| 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. 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. isn't the correct thing here to do |
||
| ``` | ||
|
|
||
|
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. |
||
| * Make the required changes to remove the conflicts, and then commit the | ||
| changes. That can be done with `git commit`. | ||
|
There was a problem hiding this comment. don't forget |
||
| * 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. 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. | ||
| * 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. | ||
| * When landing a backport commit, please include the PR-URL from the original | ||
|
There was a problem hiding this comment. /cc @nodejs/lts is everyone on board with this meta data change. For the record we have not been doing this at all. I for one would like to see this come up at an LTS meeting before we commit to taking on this process. If not in a meeting I would at least like to head from @jasnell @rvagg @mhdawson and @Fishrock123 in this comment thread There was a problem hiding this comment. Including the link to the original PR is fine but if this a backport-PR then a different metadata label should be used to avoid accidentally confusing tooling. For instance: There was a problem hiding this comment. @jasnell After the previous discussion in this PR, the consensus for the name (so far) was See #11099 (comment) There was a problem hiding this comment. Thanks @gibfahn ... missed that earlier discussion There was a problem hiding this comment. @jasnell that being said, are you alright with that approach? There was a problem hiding this comment. I good with the approach I think including the link to the original PR in Backport-of: is the right thing to do. |
||
| pull request as `Backport-of: <origin PR-URL>`. The `Backport-of` line should | ||
| be directly after the `PR-URL` line in the metadata. | ||
|
|
||
| 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. These URL dummies become real URLs and get to be confusing as absolutely different PRs. There was a problem hiding this comment. @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. This is not accurate. we maintain the original PR-URL |
||
| Backport-of: https://github.com/nodejs/node/pull/1234 | ||
| ``` | ||
|
|
||
| ### 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.
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?