-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
How does this guide relate to the https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md ? |
Are the mechanics of backporting to LTS different from backporting to current? |
@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. |
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. |
@nodejs/documentation PTAL |
@BethGriggs is this still WIP? (Please use the labels for that by the way) |
@@ -0,0 +1,124 @@ | |||
# How to backport to LTS branches |
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.
Should this be called "Creating a release" instead.
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.
+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: |
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.
Nit: shas
-> SHAs
or even SHA checksums
or whatever the best word (instead of checksums
) might be there (commit hashes
?).
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. |
We really need this. |
+1 to reopening... closing this was premature. |
I have been on vacation - planning to sync up with @MylesBorins this week to progress this guide. |
@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? |
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. |
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.
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 |
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.
+1 to preparing an LTS release or Triaging for LTS releases
|
||
- Clone node checkout staging branch | ||
```bash | ||
git clone [email protected]/nodejs/node --origin DANGER |
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.
can you explain --origin DANGER
rather than upstream
which is usually the convention I've seen 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.
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 |
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.
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. |
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.
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 #' |
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.
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 |
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.
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 ??? |
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.
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 |
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.
$ 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 |
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.
this will only work when doing the above step
01e9436
to
7a82142
Compare
``` | ||
- 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 |
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.
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 |
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.
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 #' |
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.
nit: remove --whitespace=fix #
c1f78e3
to
3087c7d
Compare
c81ec9e
to
4847890
Compare
4847890
to
f53ed14
Compare
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). |
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.
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). |
f53ed14
to
cc7f4c7
Compare
Add a section on triaging commits and PRs to land in releases.
cc7f4c7
to
de7a185
Compare
Landed in a845d7a |
Add a section on triaging commits and PRs to land in releases. PR-URL: nodejs#20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: nodejs#20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Add a section on triaging commits and PRs to land in releases. PR-URL: #20165 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
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