doc: clarify who may land on an LTS staging branch #24465

MylesBorins · 2018-11-18T17:29:06Z

Current language is a bit confusing

nodejs-github-bot · 2018-11-18T17:29:07Z

@MylesBorins sadly an error occured when I tried to trigger a build :(

targos · 2018-11-18T17:30:19Z

This paragraph is part of "How are LTS Branches Managed?" so it doesn't really make sense to have a rule about non-LTS branches in it IMHO.

MylesBorins · 2018-11-18T17:31:31Z

@targos I'm trying to figure out the intention of the original language as compared to what is documented in https://github.com/nodejs/release#lts-staging-branches

Perhaps it is better to just point at the Release repo as the source of truth?

refack · 2018-11-18T17:34:56Z

I read the same semantics in https://github.com/nodejs/release#lts-staging-branches

Every LTS major version has two branches in the GitHub repository: a release branch and a staging branch. The release branch is used to cut new releases. Only members of the release team should land commits into the release branch. The staging branch is used to land cherry-picked or backported commits from master that need to be included in a future release.

MylesBorins · 2018-11-18T17:39:01Z

@refack independent of the specific reading of the text this is how the LTS team has operated... otherwise there would be no need or an @nodejs/backporters team.

The only reason the branches are not protected are because they can't be force pushed

--> nodejs/Release#209

If we want to make the policy more lenient that is fine, but a different conversation. If the current way the policy is documented is confusing, we should fix that.

There have been more than one occasion where collaborators have accidentally backported stuff to the staging branch and it has been pointed out and not an issue. This is the first time it became clear that the documentation was inconsistent, or could be read as inconsistent, with how we actually do the work.

COLLABORATOR_GUIDE.md

Trott

LGTM in that this clearly documents our current practice and is more clear (I think) than the current text. Whether that practice could or should be changed, or whether the documentation could or should happen elsewhere rather than in this particular paragraph...
¯\(ツ)/¯

Co-Authored-By: MylesBorins <myles.borins@gmail.com>

targos · 2018-11-18T18:11:10Z

Then can this also mention the backporters team for LTS staging? I didn't know it's purpose until now

COLLABORATOR_GUIDE.md

MylesBorins · 2018-11-18T18:24:34Z

I've further clarified the language PTAL

richardlau · 2018-11-18T18:26:10Z

@targos I'm trying to figure out the intention of the original language as compared to what is documented in https://github.com/nodejs/release#lts-staging-branches

Perhaps it is better to just point at the Release repo as the source of truth?

IMO https://github.com/nodejs/release#lts-staging-branches has the same issue as the collaborator guide before the changes in this PR:

Every LTS major version has two branches in the GitHub repository: a release branch and a staging branch. The release branch is used to cut new releases. Only members of the release team should land commits into the release branch. The staging branch is used to land cherry-picked or backported commits from master that need to be included in a future release.

So having defined "release branch" and "staging branch" the only documented restriction is on the "release branch" -- If both branches are to be restricted then why specify "release branch" in the third sentence?

Refs: nodejs/node#24465

MylesBorins · 2018-11-18T18:46:25Z

can we fast track this?

@richardlau nodejs/Release#387 should fix the inconsistency in the Relase repo and I've opened nodejs/Release#388 to discuss changing the policy

jasnell · 2018-11-18T19:25:12Z

COLLABORATOR_GUIDE.md

-team should land commits into the LTS branch while preparing a new
-LTS release.
+Only the members of the @nodejs/backporters team should land commits into the
+LTS staging branch.


into LTS staging branches.

COLLABORATOR_GUIDE.md

mhdawson

LGTM as documenting current practice

mcollina

LGTM

MylesBorins · 2018-11-21T03:01:02Z

CI: https://ci.nodejs.org/job/node-test-commit/23474/

danbev · 2018-11-22T05:08:21Z

Landed in e9545e6.

Current language is a bit confusing PR-URL: #24465 Refs: #24344 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Current language is a bit confusing PR-URL: nodejs#24465 Refs: nodejs#24344 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Current language is a bit confusing PR-URL: #24465 Refs: #24344 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

doc: clarify who may land on an LTS staging branch

ca101da

Current language is a bit confusing

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 18, 2018

MylesBorins mentioned this pull request Nov 18, 2018

Revert "win,msi: install tools for native modules" #24344

Merged

3 tasks

Trott reviewed Nov 18, 2018

View reviewed changes

COLLABORATOR_GUIDE.md Outdated Show resolved Hide resolved

Trott approved these changes Nov 18, 2018

View reviewed changes

fixup: update text

ae50ca1

Co-Authored-By: MylesBorins <myles.borins@gmail.com>

richardlau reviewed Nov 18, 2018

View reviewed changes

COLLABORATOR_GUIDE.md Outdated Show resolved Hide resolved

MylesBorins added 2 commits November 18, 2018 13:23

fixup: clarify further

6328008

fixup: sigh

db735ad

addaleax approved these changes Nov 18, 2018

View reviewed changes

richardlau approved these changes Nov 18, 2018

View reviewed changes

targos approved these changes Nov 18, 2018

View reviewed changes

MylesBorins added a commit to MylesBorins/LTS that referenced this pull request Nov 18, 2018

doc: be explicit about who can land on staging

cd881e3

Refs: nodejs/node#24465

This was referenced Nov 18, 2018

doc: be explicit about who can land on staging nodejs/Release#387

Merged

Expanding access to landing on staging nodejs/Release#388

Closed

jasnell approved these changes Nov 18, 2018

View reviewed changes

devsnek approved these changes Nov 18, 2018

View reviewed changes

ofrobots approved these changes Nov 18, 2018

View reviewed changes

fixup: minor tweak from jasnell

1df57da

trivikr reviewed Nov 19, 2018

View reviewed changes

COLLABORATOR_GUIDE.md Show resolved Hide resolved

mhdawson approved these changes Nov 19, 2018

View reviewed changes

mcollina approved these changes Nov 19, 2018

View reviewed changes

danbev closed this Nov 22, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere mentioned this pull request Jan 15, 2019

v10.15.1 proposal #25346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: clarify who may land on an LTS staging branch #24465

doc: clarify who may land on an LTS staging branch #24465

MylesBorins commented Nov 18, 2018

nodejs-github-bot commented Nov 18, 2018

targos commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

refack commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

Trott left a comment

targos commented Nov 18, 2018 •

edited

Loading

MylesBorins commented Nov 18, 2018

richardlau commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

jasnell Nov 18, 2018

mhdawson left a comment

mcollina left a comment

MylesBorins commented Nov 21, 2018

danbev commented Nov 22, 2018

doc: clarify who may land on an LTS staging branch #24465

doc: clarify who may land on an LTS staging branch #24465

Conversation

MylesBorins commented Nov 18, 2018

nodejs-github-bot commented Nov 18, 2018

targos commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

refack commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

Trott left a comment

Choose a reason for hiding this comment

targos commented Nov 18, 2018 • edited Loading

MylesBorins commented Nov 18, 2018

richardlau commented Nov 18, 2018

MylesBorins commented Nov 18, 2018

jasnell Nov 18, 2018

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

MylesBorins commented Nov 21, 2018

danbev commented Nov 22, 2018

targos commented Nov 18, 2018 •

edited

Loading