Skip to content

meta: add gyp as owner of gyp files and tools/gyp - #34847

Merged
gengjiawen merged 3 commits into
nodejs:masterfrom
mmarchini:codeowners-gyp
Aug 20, 2021
Merged

meta: add gyp as owner of gyp files and tools/gyp#34847
gengjiawen merged 3 commits into
nodejs:masterfrom
mmarchini:codeowners-gyp

Conversation

@mmarchini

@mmarchinimmarchini commented Aug 19, 2020

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Aug 19, 2020•
edited by mmarchini
Loading

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-botnodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 19, 2020
@mmarchini

mmarchini commented Aug 19, 2020

Copy link
Copy Markdown
ContributorAuthor

cc @nodejs/gyp is it ok to add you as owner of gyp files?

@jasnell

jasnell commented Aug 19, 2020

Copy link
Copy Markdown
Member

Likely should also include @nodejs/build

@mmarchini

mmarchini commented Aug 19, 2020

Copy link
Copy Markdown
ContributorAuthor

Not sure, maybe @nodejs/build-files (which I can do on a follow up PR since there are other files like ./configure and Makefile which should be added)? @nodejs/build is so noisy it might not be worth to have as codeowner.

@mmarchini

mmarchini commented Aug 19, 2020

Copy link
Copy Markdown
ContributorAuthor

lol linter is failing (and I used a linter locally to check those exact lines 😅). Will figure out before landing, but the idea is to have @nodejs/gyp as owner for all gyp file changes in this repo.

@richardlau

richardlau commented Aug 19, 2020

Copy link
Copy Markdown
Member

Is

node/.github/CODEOWNERS

Lines 1 to 6 in 6f0b87d

# Node.js Project Codeowners
# 1. Codeowners must always be teams, never individuals
# 2. Each codeowner team should contain at least one TSC member
# 3. PRs touching any code with a codeowner must be signed off by at least one
# person on the code owner team.
still accurate? If so there are no TSC members in the gyp team.

@mmarchini

mmarchini commented Aug 19, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Nice catch, I didn't realize that. Is that something we still want to enforce? It's probably a leftover from the first codeowners attempt a while back.

@gengjiawen

gengjiawen commented Aug 20, 2020

Copy link
Copy Markdown
Member

cc @nodejs/gyp is it ok to add you as owner of gyp files?

cc @targos looks like you are not in this group.

ryzokuken
ryzokuken approved these changes Aug 20, 2020
View reviewed changes

@ryzokukenryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @nodejs/gyp is it ok to add you as owner of gyp files?

Yes! Thank you.

mcollina
mcollina approved these changes Aug 20, 2020
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@ryzokuken

ryzokuken commented Aug 20, 2020

Copy link
Copy Markdown
Contributor

P.S. I added @mmarchini to @nodejs/gyp so now we do have a TSC member on the team, it would be nice to have @targos on there too, if they are willing.

@targos

targos commented Aug 20, 2020

Copy link
Copy Markdown
Member

Thanks, I added myself to the team :)

@gengjiawen

This comment has been minimized.

Sign in to view
@mmarchini

mmarchini commented Aug 21, 2020

Copy link
Copy Markdown
ContributorAuthor

#34847 (comment)

gengjiawen
gengjiawen reviewed Aug 21, 2020
View reviewed changes
Comment thread.github/CODEOWNERS Outdated
@TrottTrott closed this Aug 22, 2020
@richardlaurichardlau reopened this Aug 22, 2020
@Trott

Trott commented Aug 22, 2020

Copy link
Copy Markdown
Member

(Sorry about the accidental close. Wrong window!)

@aduh95

aduh95 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

@mmarchini This needs a rebase.

@gengjiawen
gengjiawen merged commit 279162c into nodejs:masterAug 20, 2021
targos pushed a commit that referenced this pull request Aug 22, 2021
@mmarchini@targos
meta: add gyp as owner of gyp files and tools/gyp
158d446
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
PR-URL: #34847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targostargos mentioned this pull request Aug 25, 2021
Merged
targos pushed a commit that referenced this pull request Sep 4, 2021
@mmarchini@targos
meta: add gyp as owner of gyp files and tools/gyp
8f8f528
Co-authored-by: Jiawen Geng <technicalcute@gmail.com>
PR-URL: #34847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targostargos mentioned this pull request Sep 4, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
+1 more reviewer
@ryzokukenryzokukenryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

11 participants

@mmarchini@nodejs-github-bot@jasnell@richardlau@gengjiawen@ryzokuken@targos@Trott@aduh95@mcollina@lpinca