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.

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 gyp as owner of gyp files and tools/gyp #34847

Merged
merged 3 commits into from
Aug 20, 2021

Conversation

mmarchini
Copy link
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
Copy link
Collaborator

nodejs-github-bot commented Aug 19, 2020

Review requested:

  • @nodejs/tsc

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

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

@jasnell
Copy link
Member

jasnell commented Aug 19, 2020

Likely should also include @nodejs/build

@mmarchini
Copy link
Contributor Author

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

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

Is

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

mmarchini commented Aug 19, 2020

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

Copy link
Contributor

@ryzokuken ryzokuken left a comment

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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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

targos commented Aug 20, 2020

Thanks, I added myself to the team :)

@gengjiawen

This comment has been minimized.

@mmarchini
Copy link
Contributor Author

#34847 (comment)

.github/CODEOWNERS Outdated Show resolved Hide resolved
@Trott Trott closed this Aug 22, 2020
@richardlau richardlau reopened this Aug 22, 2020
@Trott
Copy link
Member

Trott commented Aug 22, 2020

(Sorry about the accidental close. Wrong window!)

@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2020

@mmarchini This needs a rebase.

@gengjiawen gengjiawen merged commit 279162c into nodejs:master Aug 20, 2021
targos pushed a commit that referenced this pull request Aug 22, 2021
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>
targos pushed a commit that referenced this pull request Sep 4, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.