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

meta: add N-API to codeowners coverage #34039

Closed
wants to merge 4 commits into from
Closed

meta: add N-API to codeowners coverage #34039

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d66b000
meta: add N-API to codeowners coverage
mhdawson Jun 24, 2020
30897f3
squash: address comments
mhdawson Jun 24, 2020
0ab5c01
Update .github/CODEOWNERS
mhdawson Jul 2, 2020
c6e2806
accept comments
mhdawson Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,10 @@
# ./lib/internal/modules/* @nodejs/modules
# ./lib/internal/bootstrap/loaders.js @nodejs/modules
# ./src/module_wrap* @nodejs/modules @nodejs/vm

# N-API

# ./src/node_api* @nodejs/n-api
Copy link
Member

Choose a reason for hiding this comment

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

are these supposed to be commented out

Copy link
Member Author

Choose a reason for hiding this comment

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

@devsnek that is a good question. I saw most of them were commented out so followed the same pattern. @jasnell since you left many of the them commented out for now is it ok to turn it on for N-API now or do you think we should let the initial set bake for a while? Either way I'd like to add N-API now (either active or commented out) so that we don't forget.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ones commented out are for teams which does not have write permission on this repository (teams which are not subteams of collaborators), as per discussions in #33895.

I'm working on an alternative to codeowners using the github-bot to work around GitHub's constraint on only teams with write permissions being valid owners. IMO it's best to keep these commented out for now, then we can uncomment when we move to the github-bot-based owners. Note that this PR is essentially a no-op this way though.

# ./src/js_native_api* @nodejs/n-api
Copy link
Member

Choose a reason for hiding this comment

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

Should the relevant doc pages also require sign off?

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau makes sense added.

# ./doc/guides/adding-new-napi-api.md @nodejs/n-api
# ./doc/api/n-api.md @nodejs/n-api
mhdawson marked this conversation as resolved.
Show resolved Hide resolved