doc: specify fast-tracking by BridgeAR · Pull Request #22929 · nodejs/node

BridgeAR · 2018-09-18T11:31:49Z

Update: please check the commit message as description.

It is currently not clear if a PR approval also approves the
fast-tracking request. This simplifies things by explicitly asking
the collaborators to remove the label if they disagree with it and
that any PR approval is also a fast-tracking approval from as soon
as the label is attached to the PR.

@nodejs/collaborators PTAL (sorry for pinging everyone but since this
is important for each one of us, it's important that everyone is on the same
page).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-09-18T11:31:50Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/924/pipeline

tniessen · 2018-09-18T11:41:17Z

+1 for the the simplicity, but I'd prefer to have the possibility to approve a PR without approving or rejecting the fast-tracking request without having to explicitely state this. I liked the convention to add a comment along the lines of "Please add 👍 here to approve fast-tracking" (I think @vsemozhetbyt introduced that?).

targos · 2018-09-18T11:43:24Z

I like the use of a comment with upvotes. What's wrong with it?

BridgeAR · 2018-09-18T11:46:53Z

If we give a LG to a PR, should we not also agree on fast-tracking or not? If someone disagrees, removing the label right away is best :)

That way it's always clear what the intention is and it's more straight forward than having an extra comment that we have to upvote.

tniessen · 2018-09-18T12:07:25Z

If we give a LG to a PR, should we not also agree on fast-tracking or not?

@BridgeAR I don't think we should make that implication.

IMO approvals on GitHub are not flexible enough anyway, e.g. it would be great if one could only approve code or docs or fast-tracking etc. If anything, it should be possible to approve smaller parts, and this change contradicts that.
It is an additional burden while reviewing: What if I am not sure whether the change should be fast-tracked but I still want to approve it?
It is easy to miss a label: Oh wait, I approved fast-tracking? (Passive vs active approval)
Setting a label doesn't notify people who previously approved, a comment does.
If one has already approved the change and later notices that the fast-track label was set afterwards, one needs to approve the change again by going to "files → review → approve" instead of just clicking 👍 on a comment.

addaleax · 2018-09-18T12:13:38Z

No strong preferences here, but a couple of quick thoughts:

The 👍-a-comment solution has been effectively the standard way of doing this for a long time now, and has worked pretty well
As a rule of thumb, explicit > implicit, and implicitly reading an approval of one thing as an approval of something else is something we’d probably want to avoid
The solution suggested here (adding/removing labels as necessary) might play better with a commit queue

BridgeAR · 2018-09-18T12:22:41Z

I just reworded it to keep the comment type but to be specific about it.

vsemozhetbyt · 2018-09-18T12:31:52Z

COLLABORATOR_GUIDE.md

a approval -> an approval.

mcollina

LGTM

BridgeAR · 2018-09-18T14:00:46Z

Lite CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/928/

BridgeAR · 2018-09-18T14:03:45Z

Please +1 if you agree with fast tracking.

richardlau

Approving, but I don't think this PR should be fast tracked to allow other collaborators a chance to review (especially given that all collaborators were pinged).

mmarchini

LGTM, but I agree with @richardlau, this shouldn't be fast-tracked.

BridgeAR · 2018-09-18T14:19:28Z

Thanks :) I just added it because it already had a lot of LGs.

I am somewhat puzzled why the linter failed.

joyeecheung · 2018-09-18T14:46:36Z

It's possible that the master branch has been force-pushed since this PR branched. Can you do a rebase and try again? @BridgeAR

Currently the documentation is not specific how fast-tracking should be applied. This specifies exactly how things should be done to prevent confusion.

BridgeAR · 2018-09-18T14:58:15Z

New CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/930/

Trott · 2018-09-18T15:43:55Z

COLLABORATOR_GUIDE.md

-CI testing is done.
+`fast-track` and add a comment that collaborators may upvote. Please mention the
+collaborators that gave an approval before in that comment. If someone disagrees
+with the fast-tracking request, please go ahead and remove the label and leave a


Nit: remove go ahead and

Nit part two: probably remove please in this sentence too. Just this?:

If someone disagrees with the fast-tracking request, remove the label and leave a comment...

danbev · 2018-09-21T03:43:48Z

Landed in 54af973.

Currently the documentation is not specific how fast-tracking should be applied. This specifies exactly how things should be done to prevent confusion. PR-URL: #22929 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

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

lundibundi approved these changes Sep 18, 2018

View reviewed changes

BridgeAR force-pushed the specifiy-fast-tracking branch from 97535c9 to 3bb07f9 Compare September 18, 2018 12:21

addaleax approved these changes Sep 18, 2018

View reviewed changes

tniessen approved these changes Sep 18, 2018

View reviewed changes

vsemozhetbyt reviewed Sep 18, 2018

View reviewed changes

COLLABORATOR_GUIDE.md Outdated

Copy link

Contributor

vsemozhetbyt Sep 18, 2018

Choose a reason for hiding this comment

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

a approval -> an approval.

vsemozhetbyt approved these changes Sep 18, 2018

View reviewed changes

vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Sep 18, 2018

jdalton approved these changes Sep 18, 2018

View reviewed changes

mcollina approved these changes Sep 18, 2018

View reviewed changes

lpinca approved these changes Sep 18, 2018

View reviewed changes

targos approved these changes Sep 18, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 18, 2018

starkwang approved these changes Sep 18, 2018

View reviewed changes

BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 18, 2018

richardlau removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 18, 2018

richardlau approved these changes Sep 18, 2018

View reviewed changes

mmarchini approved these changes Sep 18, 2018

View reviewed changes

doc: specify fast-tracking

c0c5247

Currently the documentation is not specific how fast-tracking should be applied. This specifies exactly how things should be done to prevent confusion.

BridgeAR force-pushed the specifiy-fast-tracking branch from 85251b1 to c0c5247 Compare September 18, 2018 14:57

thefourtheye approved these changes Sep 18, 2018

View reviewed changes

hiroppy approved these changes Sep 18, 2018

View reviewed changes

Trott reviewed Sep 18, 2018

View reviewed changes

Trott approved these changes Sep 18, 2018

View reviewed changes

trivikr approved these changes Sep 19, 2018

View reviewed changes

fixup: address comment

d831712

gdams approved these changes Sep 19, 2018

View reviewed changes

danbev closed this Sep 21, 2018

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

BridgeAR deleted the specifiy-fast-tracking branch January 20, 2020 11:38

Uh oh!

Conversation

BridgeAR commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Sep 18, 2018

Uh oh!

tniessen commented Sep 18, 2018

Uh oh!

targos commented Sep 18, 2018

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

tniessen commented Sep 18, 2018

Uh oh!

addaleax commented Sep 18, 2018

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

vsemozhetbyt Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

joyeecheung commented Sep 18, 2018

Uh oh!

BridgeAR commented Sep 18, 2018

Uh oh!

Trott Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

danbev commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

BridgeAR commented Sep 18, 2018 •

edited

Loading