meta: add N-API to codeowners coverage #34039

mhdawson · 2020-06-24T14:22:30Z

We have this guidance for contributing to N-API:
https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md

It makes sense to have one of the N-API team sign off on commits
that changes N-API

Signed-off-by: Michael Dawson michael_dawson@ca.ibm.com

Checklist

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

We have this guidance for contributing to N-API: https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md It makes sense to have one of the N-API team sign off on commits that changes N-API Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

richardlau · 2020-06-24T14:31:26Z

It makes sense to have one of the N-API team sign off on commits
that changes N-API

The team may have to be made a sub team of collaborators for this to work, see discussion in #33895.

richardlau · 2020-06-24T14:32:34Z

.github/CODEOWNERS

+# N-API
+
+# ./src/node_api* @nodejs/n-api
+# ./src/js_native_api* @nodejs/n-api


Should the relevant doc pages also require sign off?

@richardlau makes sense added.

devsnek · 2020-06-24T17:26:37Z

.github/CODEOWNERS

+
+# N-API
+
+# ./src/node_api* @nodejs/n-api


are these supposed to be commented out

@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.

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.

mmarchini · 2020-06-28T08:35:15Z

@nodejs/tsc (since according to CODEOWNERS changes to CODEOWNERS should add tsc for review, but apparently it didn't work)

mcollina

lgtm

Trott · 2020-07-02T03:59:21Z

It makes sense to have one of the N-API team sign off on commits
that changes N-API

The team may have to be made a sub team of collaborators for this to work, see discussion in #33895.

So someone can get write permissions to the repo by accident. We don't have any real auditing or oversight of these teams. It also means that right now, we'd have to boot a bunch of people off the team.

mhdawson · 2020-07-02T14:53:23Z

@Trott sounds like the same issue applies to the other commented out teams as well.

At this point I mostly want to make sure we don't forget applying to the N-API team once we figure how to make it work in general (and since its commented out like the others there would be no harm).

mmarchini · 2020-07-02T16:35:51Z

I'm working on an alternative to code owners that should work for us. PRs opened here and on github-bot

.github/CODEOWNERS

Co-authored-by: Richard Lau <riclau@uk.ibm.com>

Trott · 2020-07-02T23:40:46Z

@Trott sounds like the same issue applies to the other commented out teams as well.

At this point I mostly want to make sure we don't forget applying to the N-API team once we figure how to make it work in general (and since its commented out like the others there would be no harm).

Oh, uh yeah, commented out is no problem. Sorry I missed that!

.github/CODEOWNERS

jasnell

Marking this 'Request changes' so it doesn't land without the suggested fixes

mhdawson · 2020-07-03T20:46:32Z

@james I think the suggestion was to leave commented out for now. I'll remove the dot but leave comment out until we figure out the issues that apply to all of the sections.

Co-authored-by: James M Snell <jasnell@gmail.com>

jasnell · 2020-07-03T22:03:46Z

Ok... in general, now that the paths are correct, the CODEOWNERS file is working perfectly for the QUIC files. Zero issues thus far.

mhdawson · 2020-07-03T22:06:15Z

Equivalent to a doc change and linters are ok so landing.

Resolved

mhdawson · 2020-07-03T22:07:36Z

@james just noticed you are still "changes requests" are you ok with it landing?

jasnell · 2020-07-03T22:08:01Z

Heh... our timing is perfect today... just resolved that :-)

We have this guidance for contributing to N-API: https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md It makes sense to have one of the N-API team sign off on commits that changes N-API Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #34039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

mhdawson · 2020-07-03T22:16:20Z

Landed as bff7de3

We have this guidance for contributing to N-API: https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md It makes sense to have one of the N-API team sign off on commits that changes N-API Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #34039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

mmarchini · 2020-07-28T01:09:58Z

Just FYI, the team is not being added as reviewer because it is not a subteam of collaborators (as an example: #34344)

We have this guidance for contributing to N-API: https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md It makes sense to have one of the N-API team sign off on commits that changes N-API Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #34039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jun 24, 2020

richardlau reviewed Jun 24, 2020

View reviewed changes

squash: address comments

30897f3

devsnek reviewed Jun 24, 2020

View reviewed changes

mcollina approved these changes Jun 28, 2020

View reviewed changes

mmarchini mentioned this pull request Jul 1, 2020

meta: introduce custom OWNERS file #34150

Closed

4 tasks

richardlau reviewed Jul 2, 2020

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

Update .github/CODEOWNERS

0ab5c01

Co-authored-by: Richard Lau <riclau@uk.ibm.com>

Trott approved these changes Jul 2, 2020

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 2, 2020

jasnell reviewed Jul 2, 2020

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

jasnell previously requested changes Jul 3, 2020

View reviewed changes

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 3, 2020

accept comments

c6e2806

Co-authored-by: James M Snell <jasnell@gmail.com>

Trott closed this Jul 4, 2020

MylesBorins mentioned this pull request Jul 14, 2020

v14.6.0 proposal #34371

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

meta: add N-API to codeowners coverage #34039

meta: add N-API to codeowners coverage #34039

mhdawson commented Jun 24, 2020

richardlau commented Jun 24, 2020

richardlau Jun 24, 2020

mhdawson Jun 24, 2020

devsnek Jun 24, 2020

mhdawson Jun 24, 2020

mmarchini Jun 28, 2020

mmarchini commented Jun 28, 2020

mcollina left a comment

Trott commented Jul 2, 2020

mhdawson commented Jul 2, 2020

mmarchini commented Jul 2, 2020

Trott commented Jul 2, 2020

jasnell left a comment

mhdawson commented Jul 3, 2020

jasnell commented Jul 3, 2020

mhdawson commented Jul 3, 2020

mhdawson commented Jul 3, 2020

jasnell commented Jul 3, 2020

mhdawson commented Jul 3, 2020

mmarchini commented Jul 28, 2020

meta: add N-API to codeowners coverage #34039

meta: add N-API to codeowners coverage #34039

Conversation

mhdawson commented Jun 24, 2020

Checklist

richardlau commented Jun 24, 2020

richardlau Jun 24, 2020

Choose a reason for hiding this comment

mhdawson Jun 24, 2020

Choose a reason for hiding this comment

devsnek Jun 24, 2020

Choose a reason for hiding this comment

mhdawson Jun 24, 2020

Choose a reason for hiding this comment

mmarchini Jun 28, 2020

Choose a reason for hiding this comment

mmarchini commented Jun 28, 2020

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Jul 2, 2020

mhdawson commented Jul 2, 2020

mmarchini commented Jul 2, 2020

Trott commented Jul 2, 2020

jasnell left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 3, 2020

jasnell commented Jul 3, 2020

mhdawson commented Jul 3, 2020

mhdawson commented Jul 3, 2020

jasnell commented Jul 3, 2020

mhdawson commented Jul 3, 2020

mmarchini commented Jul 28, 2020