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 triaging for lts releases guide #20165

Closed
wants to merge 2 commits into from

Conversation

BethGriggs
Copy link
Member

@BethGriggs BethGriggs commented Apr 20, 2018

Adds info for backporting commits and PRs to LTS branches.

If we can get a guideline process agreed and written down, it should make it easier for more people to help out with the backporting.

//cc @gibfahn, @MylesBorins

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 20, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 20, 2018

How does this guide relate to the https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md ?

@vsemozhetbyt vsemozhetbyt added meta Issues and PRs related to the general management of the project. lts Issues and PRs related to Long Term Support releases. labels Apr 20, 2018
@richardlau
Copy link
Member

Are the mechanics of backporting to LTS different from backporting to current?

@MylesBorins MylesBorins self-assigned this Apr 20, 2018
@MylesBorins
Copy link
Contributor

@vsemozhetbyt I'll review both documents and see if there is value in merging them or having different docs... might be good to have a separate one specifically about triaging.

@richardlau backporting to LTS is the same for external collaborators. Triaging and landing commits (backport and from release lines) is different.

@BethGriggs
Copy link
Member Author

backporting-to-release-lines currently instructs contributors on how to open a backport PR, whereas this guide would document the process of landing the backport PRs/triaging commits to land. Initially I separated it out as I thought it may be confusing to show how to open a backport commit (applicable to any contributor) in the same place as how to land backport PRs/commits on staging branches (collaborator specific).

Perhaps any additional content should just be merged and expand the COLLABORATOR_GUIDE.md#technical-howto?

The main aim is to make it easier/clearer for any collaborator to traige and pull commits into the staging branches.

@BridgeAR
Copy link
Member

@nodejs/documentation PTAL

@BridgeAR
Copy link
Member

BridgeAR commented Apr 25, 2018

@BethGriggs is this still WIP? (Please use the labels for that by the way)

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Apr 25, 2018
@@ -0,0 +1,124 @@
# How to backport to LTS branches
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called "Creating a release" instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to preparing an LTS release or Triaging for LTS releases


# How to backport to the current release

- Run branch-diff to generate a list of shas:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: shas -> SHAs or even SHA checksums or whatever the best word (instead of checksums) might be there (commit hashes?).

@BridgeAR
Copy link
Member

Closing due to no response. @BethGriggs please feel free to reopen / leave a comment in case you would like to work on this again or open a new PR.

@BridgeAR BridgeAR closed this May 29, 2018
@gibfahn
Copy link
Member

gibfahn commented May 29, 2018

We really need this.

@gibfahn gibfahn reopened this May 29, 2018
@jasnell
Copy link
Member

jasnell commented May 29, 2018

+1 to reopening... closing this was premature.

@BethGriggs
Copy link
Member Author

I have been on vacation - planning to sync up with @MylesBorins this week to progress this guide.

@BethGriggs
Copy link
Member Author

@MylesBorins, what do you think should happen to this guide based on discussions at the Berlin summit about a process change (e.g. about utilizing minor and patch branches from @BridgeAR)? Have we written that proposal down anywhere?

@gibfahn
Copy link
Member

gibfahn commented Jun 5, 2018

@MylesBorins, what do you think should happen to this guide based on discussions at the Berlin summit about a process change (e.g. about utilizing minor and patch branches from @BridgeAR)?

Not Myles Borins 😁, but my preference would be to document the process as is, and then we can use the PR to update it with the new proposal to discuss how exactly that should work.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

So I've added some comments in line.

I'm curious, this guide mostly breaks down to a handful of git-fu. Perhaps it makes sense to make a separate git guide for core that includes aliases and techniques and focus on how to decide if a commit should land and branch-diff techniques for this guide?

Also open to landing all the things in here, but there is the risk of repeated information

@@ -0,0 +1,124 @@
# How to backport to LTS branches
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to preparing an LTS release or Triaging for LTS releases


- Clone node checkout staging branch
```bash
git clone git@github.com/nodejs/node --origin DANGER
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain --origin DANGER rather than upstream which is usually the convention I've seen used

Copy link
Member Author

Choose a reason for hiding this comment

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

That was a suggestion from @gibfahn for my setup - i'll swap it back to upstream so that it is consistent with other docs.


- Make sure up to date with staging
```bash
git fetch --all
Copy link
Contributor

Choose a reason for hiding this comment

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

i usually do git remote update -p

- Check approvals (you can approve yourself)
- Check SemVer
- Check the PR metadata wasn't changed from `master` commit.
- If there are merge conflicts, comment in the issue asking the PR author to rebase.
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively you can fix this manually while landing

- Land if okay
- Add git alias if you haven't already:
```bash
git config --global alias.pa '!curl -L $1.patch | git am -3 -S --whitespace=fix #'
Copy link
Contributor

Choose a reason for hiding this comment

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

I've ended up removing --whitespace=fix as it can cause some serious issues with certain backports.

# Add this line to each commit under the PR-URL:
# Backport-PR-URL: https://github.com/nodejs/node/pull/12345
```
- Test the commit and push it
Copy link
Contributor

Choose a reason for hiding this comment

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

sometimes I'll just run the tests for the PR in question when backporting rather than running all the tests

see: https://github.com/MylesBorins/dot-files/blob/master/.bash_profile#L64


- Run branch diff /w parameters
```bash
branch-diff ???
Copy link
Contributor

Choose a reason for hiding this comment

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

branch-diff vX.x-staging upstream/vY.x -exclude-label semver-major,semver-minor,dont-land-on-vX.x,backport-requested-vX.x,backported-to-vX.x,baking-for-lts --filter-release

where X = LTS branch and Y = Current branch


- Run branch-diff to generate a list of shas:
```bash
branch-diff --reverse --sha
Copy link
Contributor

Choose a reason for hiding this comment

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

$ branch-diff vX.x-staging upstream/master --exclude-label semver-major,dont-land-on-vX.x,backport-requested-vX.x,backported-to-vX.x --filter-release --format=sha --reverse > commits
$ cat commits | xargs git cherry-pick

- For each commit try to cherry-pick it, if it fails:
- Open PR in browser.
- Either fix conflicts and backport, or don't fix and bail.
- If PR had multiple commits, `git cherry-pick --quit` and `git reset --hard` to
Copy link
Contributor

Choose a reason for hiding this comment

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

this will only work when doing the above step

@BethGriggs BethGriggs changed the title [WIP] doc: add backporting to LTS branches guide [WIP] doc: add triaging for lts releases guide Jun 8, 2018
```
- Close the backport PR with `Landed in $(git rev-parse --short HEAD)`, update the label on the original PR from `backport-requested-vX.x` to `backported-to-vX.x`.

### Triage commits
Copy link
Member

Choose a reason for hiding this comment

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

I'd say this should come at the top before the Triage PRs section potentially?

```
- Patch Pull Request onto staging branch:
```bash
git pa https://github.com/nodejs/node/pull/12345
Copy link
Member

Choose a reason for hiding this comment

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

For ease of understanding it might be helpful to add that this patch flow also applies to commits, and add a section to commit triaging that includes an example similar to:
$ git pa https://github.com/nodejs/node/commit/1a2b3c4d

- Land if okay
- Add git alias if you haven't already:
```bash
git config --global alias.pa '!curl -L $1.patch | git am -3 -S --whitespace=fix #'
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove --whitespace=fix #

@BethGriggs BethGriggs force-pushed the backporting-guide branch 3 times, most recently from c1f78e3 to 3087c7d Compare October 9, 2018 10:36
doc/releases.md Outdated
appropriate commits into it. To determine the relevant commits, use
appropriate PRs and commits into it.

Go through PRs with the label `vN.x`. e.g. [PRs with the v8.x label](https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Av8.x).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Go through PRs with the label `vN.x`. e.g. [PRs with the v8.x label](https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Av8.x).
Go through PRs with the label `vN.x`. e.g. [PRs with the `v8.x` label](https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Av8.x).

@Trott
Copy link
Member

Trott commented Dec 4, 2018

BethGriggs and others added 2 commits December 4, 2018 18:27
@Trott
Copy link
Member

Trott commented Dec 5, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 5, 2018
@Trott
Copy link
Member

Trott commented Dec 5, 2018

Landed in a845d7a

@Trott Trott closed this Dec 5, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 5, 2018
Add a section on triaging commits and PRs to land in releases.

PR-URL: nodejs#20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Add a section on triaging commits and PRs to land in releases.

PR-URL: nodejs#20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BethGriggs added a commit that referenced this pull request Feb 12, 2019
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
BethGriggs added a commit that referenced this pull request Feb 20, 2019
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Add a section on triaging commits and PRs to land in releases.

PR-URL: #20165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs BethGriggs deleted the backporting-guide branch June 11, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. lts Issues and PRs related to Long Term Support releases. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.