The Wayback Machine - https://web.archive.org/web/20210909180209/https://github.com/nodejs/node/pull/34847
Skip to content

/ node Public

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

Jump to bottom

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

Merged
merged 3 commits into from Aug 20, 2021
Merged

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

merged 3 commits into from Aug 20, 2021

Conversation

@mmarchini
Copy link
Member

@mmarchini mmarchini commented Aug 19, 2020

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
@mmarchini
meta: add gyp as owner of gyp files and tools/gyp
8021388
@nodejs-github-bot
Copy link
Contributor

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

Review requested:
@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Aug 19, 2020

cc @nodejs/gyp is it ok to add you as owner of gyp files?
@jasnell
jasnell approved these changes Aug 19, 2020
View changes
@jasnell
Copy link
Member

@jasnell jasnell commented Aug 19, 2020

Likely should also include @nodejs/build
@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Aug 19, 2020

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
Member Author

@mmarchini mmarchini commented Aug 19, 2020

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

@richardlau richardlau commented Aug 19, 2020

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

@mmarchini mmarchini commented Aug 19, 2020 •
edited

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 approved these changes Aug 20, 2020
View changes
@gengjiawen
Copy link
Member

@gengjiawen gengjiawen commented Aug 20, 2020

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

@ryzokuken ryzokuken left a comment

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

@mcollina mcollina left a comment

lgtm
@ryzokuken
Copy link
Member

@ryzokuken ryzokuken commented Aug 20, 2020

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 targos commented Aug 20, 2020

Thanks, I added myself to the team :)
@lpinca
lpinca approved these changes Aug 20, 2020
View changes
@gengjiawen

This comment has been hidden.

Sign in to view
@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Aug 21, 2020

#34847 (comment)
@gengjiawen
gengjiawen reviewed Aug 21, 2020
View changes
.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 Trott commented Aug 22, 2020

(Sorry about the accidental close. Wrong window!)
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 8, 2020

@mmarchini This needs a rebase.
@guybedford guybedford force-pushed the nodejs:master branch from dc5a5da to 8e46568 Mar 29, 2021
gengjiawen added 2 commits Aug 20, 2021
@gengjiawen
Merge branch 'master' into codeowners-gyp
8a2422f
@gengjiawen
fix
47592a3
@gengjiawen gengjiawen merged commit 279162c into nodejs:master Aug 20, 2021
17 of 18 checks passed
17 of 18 checks passed
@github-actions
lint-commit-message lint-commit-message
Details
@github-actions
build-tarball
Details
@github-actions
build-windows
Details
@github-actions
coverage-linux
Details
@github-actions
coverage-windows
Details
@github-actions
lint-addon-docs
Details
@github-actions
build-docs
Details
@github-actions
test-asan
Details
@github-actions
test-linux
Details
@github-actions
test-macOS
Details
@github-actions
test-tarball-linux
Details
@github-actions
lint-cpp
Details
@github-actions
lint-md
Details
@github-actions
lint-js
Details
@github-actions
lint-py
Details
@github-actions
lint-sh
Details
@github-actions
lint-codeowners
Details
@github-actions
lint-pr-url
Details
targos added 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>
@targos targos mentioned this pull request Aug 25, 2021
Merged
targos added 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>
@targos targos mentioned this pull request Sep 4, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta