Skip to content
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

Closed
wants to merge 11 commits into from
5 changes: 4 additions & 1 deletion COLLABORATOR_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,8 @@ LTS release.

When you send your pull request, consider including information about
whether your change is breaking. If you think your patch can be backported,
please feel free to include that information in the PR thread.
please feel free to include that information in the PR thread. For more
information on backporting, please see the [backporting guide][].

Several LTS related issue and PR labels have been provided:

Expand All @@ -364,3 +365,5 @@ When the LTS working group determines that a new LTS release is required,
selected commits will be picked from the staging branch to be included in the
release. This process of making a release will be a collaboration between the
LTS working group and the Release team.

[backporting guide]: doc/guides/backporting-to-release-lines.md
124 changes: 124 additions & 0 deletions doc/guides/backporting-to-release-lines.md
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
Copy link
Member

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.


| 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be more specific regarding the label here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, specificity is important

Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn should we specify the label?

Copy link
Member

Choose a reason for hiding this comment

The 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 (currently v7.x) is much more lenient than the LTS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/(currently v7.x)/(v7.x as of Feb 2017)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop the v in the version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit confusing in the way it is selectively using / switching backporting + cherry-picking.

Perhaps

Our LTS release lines (v4.x and v6.x as of Feb 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.

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
Copy link
Member

Choose a reason for hiding this comment

The 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 [email protected]:$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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, although I would think that origin is more common than fork

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to be specific in instructions, origin can end up being upstream or fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the correct thing here to do git cherry-pick continue?

```

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe delete . That can be done, otherwise it seems like maybe git cherry-pick continue is just one way to commit the changes, and we don't want people defaulting to git commit.

`git commit`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be git cherry-pick --continue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, I've always just fixed it up and then committed manually

* The commit message should be as close as possible to the commit message on the
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
* Push the changes to your fork and open a pull request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be a:

  • Make sure make -j4 test still passes.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Original-PR-URL: https://... (original PR)
PR-URL: https://... (backport PR)

Copy link
Member

@gibfahn gibfahn Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell After the previous discussion in this PR, the consensus for the name (so far) was Backport-of: <origin PR-URL>, it's on the next line.

See #11099 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gibfahn ... missed that earlier discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell that being said, are you alright with that approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that definitely works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
* Please check the checkbox labelled "Allow edits from maintainers".
Copy link
Contributor

Choose a reason for hiding this comment

The 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