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: clarify assigning issues to the TSC #22759

Closed
wants to merge 1 commit into from
Closed

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Sep 7, 2018

Using the right hand side to assign @nodejs/tsc or to request reviews from @nodejs/tsc breaks all kinds of workflows and filter rules. Please use the appropriate tsc-review label or mention @nodejs/tsc in a comment instead.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 7, 2018
@fhinkel fhinkel requested a review from ofrobots September 7, 2018 23:50
@jasnell
Copy link
Member

jasnell commented Sep 7, 2018

Hmm... breaks workflows how? I'm not sure I agree with a restriction not to use the GitHub UI for requesting TSC review.

@fhinkel
Copy link
Member Author

fhinkel commented Sep 7, 2018

There's no way to filter requests to a specific person or to teams. Please don't use it.

@kadeatfox
Copy link

As a non-contributing observer, I agree with @fhinkel

@fhinkel
Copy link
Member Author

fhinkel commented Sep 7, 2018

Pull requests -> Reviews requests lists PRs where my reviews are requested. Right now this list is useless because of all the TSC items. Doesn't matter if I reviewed it or not. As long as TSC is assigned, it's rendering my to-do list useless.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2018

@fhinkel I use that feature to request reviews from the TSC for semver-major changes or in very rare cases, PRs that are controversial. Would it therefore actually not be good to see that PR in your list?

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2018

In my experience, the review label was completely ignored so far.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2018

I just put one PR back on the TSC agenda as well, since not enough members reacted to your request to provide feedback to two competing PRs.

@fhinkel
Copy link
Member Author

fhinkel commented Sep 8, 2018

Yes, it's very annoying to get those requests. Instead of making me anything review faster, I don't review anything at all because it's too much spam and end up missing requests targeted directly at me.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2018

But how is something spam that only the TSC can may do?


- is labeled `semver-major`, or
- has a significant impact on the codebase, or
- is inherently controversial, or
- has failed to reach consensus amongst the Collaborators who are
actively participating in the discussion.

Assign the `tsc-review` label or @-mentioning the
Copy link
Member

Choose a reason for hiding this comment

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

Nit: mentioning -> mention

@fhinkel
Copy link
Member Author

fhinkel commented Sep 8, 2018

It's spam because it goes to 19 people independent of whether they've already reviewed it. I have a day job. I want to look at TSC issues after hours and not have them in my list. Partially my fault for not using separate accounts. But when using the same account theres no way to separate them.

@fhinkel
Copy link
Member Author

fhinkel commented Sep 8, 2018

If you use the label or mention, I can grab for the label or search my inbox for TSC reviews.

@fhinkel
Copy link
Member Author

fhinkel commented Sep 8, 2018

And it's not like TSC members respond to the group review request, so you might as well not do it and save me a big headache.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. I still wonder how we could improve the TSC reviewing part in general.

@fhinkel
Copy link
Member Author

fhinkel commented Sep 8, 2018

Pinging the right individual people directly usually works better. Either via mention or Twitter/email. There's so much noise on GitHub, it's really hard to stay on top.

@gireeshpunathil
Copy link
Member

Unaware about the internal workflow complexity, but convincing to hear from the person who manages that. In addition, it brings the best use of the label - if an action with same purpose is carried out through multiple means, the rule gets vague. This brings clarity to those.

@targos
Copy link
Member

targos commented Sep 8, 2018

@fhinkel FWIW this is how I fixed this issue on my side: Pull Requests -> Review requests -> Add org:org_name to the query -> Bookmark URL

Edit: you can also keep everything but node with -org:nodejs


- is labeled `semver-major`, or
- has a significant impact on the codebase, or
- is inherently controversial, or
- has failed to reach consensus amongst the Collaborators who are
actively participating in the discussion.

Assign the `tsc-review` label or @-mention the
`@nodejs/tsc` GitHub team if you want to elevate an issue to the [TSC][].
Do not use the GitHub UI on the right hand side to assign to`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra backtick at the line end.

Copy link
Member

Choose a reason for hiding this comment

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

Rather than making this a hard rule without documenting the reasons for it, consider rewording as a suggestion with the reasons why one might want to avoid it. Also, consider, that the same likely applies to requesting review from any team, not just the @nodejs/tsc.

The following is too wordy given that it's 6:30am on a Saturday and I haven't had enough coffee but this should illustrate the point:

Use of the GitHub UI to assign an issue to, or request a review
from the `@nodejs/tsc` (or any team) is not recommended because,
at the current time, doing so causes unnecessary notifications to
be sent to team members who may already have provided feedback or
reviews for the issue or PR. It is better to selectively request
reviews on an individual basis.

More Friendly-Advice-To-Make-Co-Contributors-Lives-Easier, less Yet-Another-Rule-One-Has-To-Follow.

Assign the `tsc-review` label or @-mention the
`@nodejs/tsc` GitHub team if you want to elevate an issue to the [TSC][].
Do not use the GitHub UI on the right hand side to assign to
`@nodejs/tsc` or request a review from `@nodejs/tsc`.
Copy link
Member

Choose a reason for hiding this comment

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

Would it still be fine to use it once in the beginning? That way it would show up for the whole TSC. Just assigning it a second time would be discouraged after the first TSC member looked at it?

Copy link
Member

Choose a reason for hiding this comment

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

What's the hoped-for result that wouldn't be achieved with an @-mention?

I do think that getting TSC reviews is a bit of a problem right now. I think there are some culture/people changes that need to happen more than process changes to fix that, though.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Oct 3, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 3, 2018
PR-URL: nodejs#22759
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 3, 2018

Landed in af522c1

@Trott Trott closed this Oct 3, 2018
targos pushed a commit that referenced this pull request Oct 4, 2018
PR-URL: #22759
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #22759
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.