Skip to content

meta: add initial CODEOWNERS file#20554

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:add-codeowners
Closed

meta: add initial CODEOWNERS file#20554
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:add-codeowners

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

vsemozhetbyt commented May 6, 2018

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

@vsemozhetbyt vsemozhetbyt May 6, 2018

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

@vsemozhetbyt vsemozhetbyt May 6, 2018

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

@targos targos May 6, 2018

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

@vsemozhetbyt vsemozhetbyt May 6, 2018

Choose a reason for hiding this comment

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

Duplicate?

.github/CODEOWNERS Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 6, 2018

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

@devsnek devsnek May 6, 2018

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

@vsemozhetbyt vsemozhetbyt May 6, 2018

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

@jasnell jasnell May 6, 2018

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

@vsemozhetbyt vsemozhetbyt May 7, 2018

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

@targos targos targos approved these changes

@trivikr trivikr trivikr approved these changes

+3 more reviewers

@devsnek devsnek devsnek approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@mmarchini mmarchini mmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

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.

7 participants

@jasnell @devsnek @trivikr @vsemozhetbyt @targos @mmarchini @nodejs-github-bot