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.

Sign up for GitHub

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

Jump to bottom

doc: specify fast-tracking #22929

Closed
wants to merge 2 commits into from
Closed

doc: specify fast-tracking #22929

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 18, 2018 •
edited
Loading

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

@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

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 18, 2018
@tniessen
Copy link
Member

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

targos commented Sep 18, 2018

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

@BridgeAR
Copy link
Member Author

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

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

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

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

vsemozhetbyt
vsemozhetbyt reviewed Sep 18, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
approve both the pull request and the fast-tracking request, and the necessary
CI testing is done.
`fast-track` and add a comment that collaborators may upvote. Please mention the
collaborators that gave a approval before in that comment. If someone disagrees
Copy link
Contributor

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 vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Sep 18, 2018
mcollina
mcollina approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR 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
@BridgeAR
Copy link
Member Author

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

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

Please +1 if you agree with fast tracking.

@richardlau richardlau removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 18, 2018
richardlau
richardlau approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

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
mmarchini approved these changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

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

@BridgeAR
Copy link
Member Author

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

I am somewhat puzzled why the linter failed.

@joyeecheung
Copy link
Member

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

@BridgeAR
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
Copy link
Member Author

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

Trott
Trott reviewed Sep 18, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
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
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 go ahead and

Copy link
Member

Choose a reason for hiding this comment

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

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

danbev commented Sep 21, 2018

Landed in 54af973.

@danbev danbev closed this Sep 21, 2018
danbev pushed a commit that referenced this pull request Sep 21, 2018
@BridgeAR @danbev
doc: specify fast-tracking
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>
targos pushed a commit that referenced this pull request Sep 21, 2018
@BridgeAR @targos
doc: specify fast-tracking
00dd973 
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>
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@BridgeAR BridgeAR deleted the specifiy-fast-tracking branch January 20, 2020 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@richardlau richardlau richardlau approved these changes

@mmarchini mmarchini mmarchini approved these changes

@jdalton jdalton jdalton approved these changes

@Trott Trott Trott approved these changes

@starkwang starkwang starkwang approved these changes

@gdams gdams gdams approved these changes

@addaleax addaleax addaleax approved these changes

@tniessen tniessen tniessen approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@mcollina mcollina mcollina approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@trivikr trivikr trivikr approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
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. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.