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

meta: add initial CODEOWNERS file #20554

Closed
wants to merge 1 commit into from
Closed

meta: add initial CODEOWNERS file #20554

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 6, 2018

First pass at a CODEOWNERS file

This likely needs refinement.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label May 6, 2018
@devsnek
Copy link
Member

devsnek commented May 6, 2018 •
edited
Loading

could add /lib/internal/modules/esm/ @nodejs/modules

@trivikr
Copy link
Member

trivikr commented May 6, 2018

More suggestions:

  • /deps/http_parser/ @nodejs/http-parser
  • /deps/zlib/ @nodejs/zlib

@vsemozhetbyt
Copy link
Contributor

Does this file work with .txt extension?

vsemozhetbyt
vsemozhetbyt reviewed May 6, 2018
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We may also mutually merge this with data from COLLABORATOR_GUIDE.md#who-to-cc-in-the-issue-tracker

.github/CODEOWNERS.txt Outdated
/deps/npm/ @nodejs/npm
/deps/v8/ @nodejs/v8

/benchmark/ @nodejs/performance
Copy link
Contributor

Choose a reason for hiding this comment

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

+ @nodejs/benchmarking?

.github/CODEOWNERS.txt Outdated
*vm* @nodejs/vm
*zlib* @nodejs/zlib

/src/node.cc @nodejs/process
Copy link
Contributor

Choose a reason for hiding this comment

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

+ bootstrap_node.js?

targos
targos reviewed May 6, 2018
View reviewed changes
.github/CODEOWNERS.txt Outdated

/deps/libuv/ @nodejs/libuv
/deps/npm/ @nodejs/npm
/deps/v8/ @nodejs/v8
Copy link
Member

Choose a reason for hiding this comment

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

@nodejs/v8-update

@jasnell
Copy link
Member Author

jasnell commented May 6, 2018

Updated... fwiw, this is going to require ongoing refinement. It's going to take several tries to get this exactly right. I'd like to get this landed and iterate on it.

@targos
Copy link
Member

targos commented May 6, 2018

The documentation says "The people you choose as code owners must have write permissions for the repository." so I'm not sure it will work as intended.

@jasnell
Copy link
Member Author

jasnell commented May 6, 2018

Yep. For most of the groups that holds true. I don't know about groups that may contain a mix. This is a trial run :)

vsemozhetbyt
vsemozhetbyt reviewed May 6, 2018
View reviewed changes
.github/CODEOWNERS Outdated
*tls* @nodejs/tls
*dgram* @nodejs/dgram
*domain* @nodejs/domains
*fs* @nodejs/fs
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

.github/CODEOWNERS Outdated
*vm* @nodejs/vm
*zlib* @nodejs/zlib

/lib/internal/modules/esm/ @nodejs/modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it covered by *module* @nodejs/modules?

Copy link
Member

Choose a reason for hiding this comment

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

no, the modules team should not be pinged for cjs stuff (in /lib/internal/modules/cjs/)

Copy link
Contributor

Choose a reason for hiding this comment

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

But will not it be pinged as *module* glob above matches/lib/internal/modules/cjs/?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that will need to be refined. We don't have a team for the cjs module system. That one might need to be nodejs/tsc

Copy link
Contributor

Choose a reason for hiding this comment

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

(Then COLLABORATOR_GUIDE.md#who-to-cc-in-the-issue-tracker also needs to be refined as for @nodejs/modules cc)

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 6, 2018 •
edited
Loading

Also to consider (partly from "Who to cc.."):

*addons* @nodejs/addon-api

*tls* @nodejs/crypto
*https* @nodejs/crypto

*doc* @nodejs/documentation
*.md @nodejs/documentation

src/node_file.* @nodejs/fs
src/node_stat_watcher.* @nodejs/fs

*esm* @nodejs/modules
lib/internal/bootstrap/loaders.js @nodejs/modules

*stream* @nodejs/streams
*net* @nodejs/streams

/test/ @nodejs/testing

/api_assets/ @nodejs/Website

Makefile @nodejs/build
vcbuild.bat @nodejs/build
BUILDING.md @nodejs/build

*aix* @nodejs/platform-aix
*arm* @nodejs/platform-arm
*freebsd* @nodejs/platform-freebsd
*macos* @nodejs/platform-macos
*ppc* @nodejs/platform-ppc
*smartos* @nodejs/platform-smartos
*s390* @nodejs/platform-s390
*windows* @nodejs/platform-windows

@vsemozhetbyt vsemozhetbyt mentioned this pull request May 6, 2018
Closed
3 tasks
mmarchini
mmarchini approved these changes May 7, 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.

+1 on landing this and iterating on it later.

A few suggestions if you want to update before landing:

/src/node_postmortem_metadata.cc @nodejs/diagnostics @nodejs/post-mortem
/src/*.d @nodejs/diagnostics @nodejs/platform-smartos @nodejs/platform-freebsd @nodejs/platform-macos @nodejs/dtrace-mdb
/src/node.stp @nodejs/diagnostics

@jasnell jasnell force-pushed the add-codeowners branch 2 times, most recently from 5c5b823 to 883afb6 Compare May 12, 2018 23:44
jasnell added a commit that referenced this pull request May 12, 2018
@jasnell
meta: add initial CODEOWNERS file
c0359f0 
PR-URL: #20554
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@jasnell
Copy link
Member Author

jasnell commented May 12, 2018

Landed in c0359f0

@jasnell jasnell closed this May 12, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
@jasnell @addaleax
meta: add initial CODEOWNERS file
b108235 
PR-URL: #20554
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
@richardlau richardlau mentioned this pull request Oct 22, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@targos targos targos approved these changes

@mmarchini mmarchini mmarchini approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
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.

None yet
7 participants
@jasnell @devsnek @trivikr @vsemozhetbyt @targos @mmarchini @nodejs-github-bot