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: document the collaborator nomination process #18268

Closed

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 20, 2018

Also describes the privileges of the collaborators and improve the instructions of the onboarding PR.

This is a draft done after the discussion in #18090 and this week's TSC meeting, any feedback is welcome!

Refs: nodejs/TSC#472
Fixes: #18090

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jan 20, 2018
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

Very nice update -- LGTM

metadata.
* [`core-validate-commit`][] automates the validation of commit messages.
* [`node-core-utils`][] automates the generation of metadata and the landing
process. See the documentation of [git-node][].
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, broken link (no backticks). Will fix it later.

Copy link
Contributor

@Leko Leko left a comment

Choose a reason for hiding this comment

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

LGTM

@joyeecheung
Copy link
Member Author

Forgot to cc @nodejs/tsc @nodejs/collaborators

GOVERNANCE.md Outdated
* Commits in the [nodejs/node][] repository.
* Can be shown using the link
`https://github.com/nodejs/node/commits?author=${GITHUB_ID}`
(replace `${GITHUB_ID}` with their GitHub ID).
Copy link
Member

Choose a reason for hiding this comment

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

the collaborator's GitHub ID? Singular they should be used sparingly, IMO.

GOVERNANCE.md Outdated
Prior to the public nomination, the collaborator initiating it can seek
feedback from other collaborators in private using
[the GitHub discussion page][collaborators-discussions] of the
collaborators team, and work with the nominee to improve their contribution
Copy link
Member

Choose a reason for hiding this comment

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

Another instance where I'd be explicit and write the nominee's contribution profile. A rushed reader might think it's about the collaborators team.

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

GOVERNANCE.md Outdated
The Technical Steering Committee (TSC) has final authority over this project
including:
A subgroup of the Collaborators form the Technical Steering Committee (TSC),
who admits and oversees all top-level Projects in the Node.js Foundation,
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, maybe put a period instead of a comma after (TSC) on the line above this one? Then start this line with The TSC admits and oversees all...

There are currently no "top-level projects" other than core. (express and libuv never completed the process and therefore never graduated to top-level project status.) There is unlikely to ever be any other top-level projects. I think the terminology should be removed as it is confusing. Using it implies that that whole top-level project is a process that exists, a thing we do. I would just say:

The TSC has final authority over this project, including:

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott I copied the sentence from the TSC readme, we should also update that one.

Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung Sounds like a good idea to me. nodejs/TSC#477

GOVERNANCE.md Outdated
@@ -78,7 +84,8 @@ including:
* Conduct guidelines
* Maintaining the list of additional Collaborators

* [Current list of TSC members](./README.md#current-project-team-members)
The current list of TSC members can be found
[here](./README.md#current-project-team-members).
Copy link
Member

Choose a reason for hiding this comment

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

Generally, it's good to avoid "here" as link text. Maybe use "in the project README" or something like that instead?

GOVERNANCE.md Outdated

To nominate a new collaborator, open an issue in the [nodejs/node][]
repository, with a summary of the nominee's contributions, including but
not limited to:
Copy link
Member

Choose a reason for hiding this comment

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

The language "including but not limited to" suggests that all of the below bullet items should always be included. I'm OK with that, but just leaving this comment to make sure that is 100% the intention here.

Copy link
Member Author

@joyeecheung joyeecheung Jan 22, 2018

Choose a reason for hiding this comment

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

@Trott Yes it does look like we are requiring people to list all of the items, which is not really what I meant... I'll change it to , for example:

GOVERNANCE.md Outdated
* Other participation in the wider Node.js community

Mention @nodejs/tsc and @nodejs/collaborators in the issue to notify the TSC
and other collaborators about the nomination.
Copy link
Member

Choose a reason for hiding this comment

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

Since TSC is a subset of Collaborators, it's sufficient to mention nodejs/collaborators only.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott I am fine with that, just thinking people might be using different email filters for different mentions.


### Onboarding

When the nomination is accepted, the new Collaborator will be onboarded
Copy link
Member

Choose a reason for hiding this comment

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

"Collaborator" is capitalized here but not elsewhere in the new text (such as the previous paragraph). We seem to have standardized on capitalizing it, so I'd recommend doing that throughout the doc, or at least in the new text.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 23, 2018

@Trott Thanks for the review, updated. PTAL.
Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/116/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2018
@joyeecheung
Copy link
Member Author

I am going to land this later today if there are no objections.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 24, 2018

Landed in a083786...6288050, thanks!

joyeecheung added a commit that referenced this pull request Jan 24, 2018
PR-URL: #18268
Fixes: #18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
joyeecheung added a commit that referenced this pull request Jan 24, 2018
PR-URL: #18268
Fixes: #18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #18268
Fixes: #18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #18268
Fixes: #18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
@mcollina mcollina mentioned this pull request Mar 14, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18268
Fixes: nodejs#18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18268
Fixes: nodejs#18090
Refs: nodejs/TSC#472
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meta: about the collaborator nomination process