-
Notifications
You must be signed in to change notification settings - Fork 30k
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: instructions on how to make membership public #17688
Conversation
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.
One small comment, otherwise LGTM.
doc/onboarding.md
Outdated
@@ -38,6 +38,10 @@ onboarding session. | |||
* Branches in the nodejs/node repository are only for release lines | |||
* [See "Updating Node.js from Upstream"](./onboarding-extras.md#updating-nodejs-from-upstream) | |||
* Make a new branch for each PR you submit. | |||
* Membership: We ask that collaborators make their membership in the Node.js |
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.
Maybe specify GitHub membership
or move into a separate GitHub section? At first is a little bit unclear.
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.
Just to clarify, is this a request or a requirement for collaborators?
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.
Maybe move to near the requirement for enabling 2FA?
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 agree, the current wording makes this sound like a requirement, which is something I would be really careful about.
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.
Just to clarify, is this a request or a requirement for collaborators?
Having your membership public means GitHub can assign you the appropriate UI badges. As it is now...
...there's no indicator that you're part of the Node org. much less a member of the TSC or collaborator.
Your membership is already public in the sense that your name is listed here and here. This just lets GitHub do their UI thing so that users can more easily identify project maintainers.
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 know. Aren't the labels that GitHub shows next to your name in these threads, along with the list in the project's README enough?
I'm fine with a request/recommendation, but would be -1 on a hard requirement.
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'm saying as it is now your label is incorrect. I'm also labeled a "contributor" but I'm not part of the org (not a collaborator, TSC, or core). The "contributor" badge just means I've had at least 1 commit merged. If you were part of the org you would have the label "member" or "owner".
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.
Ah, I thought you were a contributor :-)
Even still, I'm not sure that it would be a huge help. We have a members team with over 500 people that includes more people than the core collaborator list.
I'm mostly being argumentative on principle. It feels like Node would be overstepping its boundaries by requiring that collaborators make membership public. I'm a member of several other orgs, and none of them have such a requirement.
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.
Ah, I thought you were a contributor :-)
Exactly. You proved my point 😋
The closeness of "collaborator" and "contributor" may also be part of the confusion.
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.
@cjihrig the "Owner" next to your name is only shown if the current logged-in user is in the @ nodejs org. If I wasn't logged in, I would see "Contributor" instead.
I updated the text a bit (capitalized |
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.
The current wording sounds fine to me :)
Landing... |
Landed in 7a0f774 |
PR-URL: nodejs#17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc