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

Jump to bottom

doc: add .github to CODEOWNERS #20733

Closed
wants to merge 1 commit into from
Closed

doc: add .github to CODEOWNERS #20733

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 15, 2018 •
edited
Loading

Add .github directory to CODEOWNERS file.

Checklist

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label May 15, 2018
@Trott
Copy link
Member Author

Trott commented May 15, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/709/

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt vsemozhetbyt added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 15, 2018
@apapirovski
Copy link
Member

@Trott sorry, this needs a rebase due to the other CODEOWNERS changes landing.

@jasnell
Copy link
Member

jasnell commented May 16, 2018

btw.. it's not clear if the CODEOWNERS file is actually working at this point... need to do more verification...

@Trott
Copy link
Member Author

Trott commented May 16, 2018 •
edited
Loading

btw.. it's not clear if the CODEOWNERS file is actually working at this point... need to do more verification...

@jasnell Seems to be working as expected. What's makes it not clear that it's working in your view?

@Trott
doc: add .github to CODEOWNERS
eb115d8 
Add .github directory to CODEOWNERS file.
@Trott
Copy link
Member Author

Trott commented May 16, 2018

@apapirovski Rebased.

@jasnell
Copy link
Member

jasnell commented May 16, 2018

What makes it not clear ...

For example, the @nodejs/fs team did not appear to be requested for review automatically in the various fs related PRs I just opened.

@Trott
Copy link
Member Author

Trott commented May 16, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/717/

@Trott
Copy link
Member Author

Trott commented May 16, 2018

For example, the @nodejs/fs team did not appear to be requested for review automatically in the various fs related PRs I just opened.

@jasnell Per https://help.github.com/articles/about-codeowners/: "The people you choose as code owners must have write permissions for the repository." I very much doubt that GitHub expands the team and checks the permissions of every person on the team. So the nodejs/fs team will never be requested for review automatically, if I'm right about that. Again, if I'm right, choices are:

  • List people as individuals in CODEOWNERS (seems undesirable to me)
  • Add the team as having write privileges to the repo (also undesirable IMO as there's no reason to think that group should be limited to current Collaborators)
  • Use CODEOWNERS as documentation for groups like that rather than automation (seems pointless to me)
  • Shrug and remove entries for teams other than TSC and call it a mostly-failed experiment. (No shame in failed experiments. If all your experiments succeed, you're not experimenting enough.)

@Trott
Copy link
Member Author

Trott commented May 16, 2018

@jasnell My speculation is confirmed at https://git.luolix.topmunity/t5/How-to-use-Git-and-GitHub/CODEOWNERS-works-with-users-but-not-teams/m-p/4991#M1612:

It's not enough for all the members to have write access, you need to grant write access to the actual team.

As I say above, I don't think we actually want to do that (and certainly not without the TSC discussing it). So I think we're stuck with a CODEOWNERS file that works for TSC assignments and not much else.

Paring down the CODEOWNERS file to just TSC might actually be a good start.

We can then add individuals for certain files or areas. I'm not sure that will actually work for the same reason teams don't work. (Does an individual need to be listed in the Settings as having write permission or is it enough that they're a member of a team with Write permission? We'll find out, I guess.)

I know I said previously that I thought we probably don't want to add individuals, but thinking about it more, it's probably OK. If an individual is no longer a Collaborator, their entry in CODEOWNERS will be ignored (because they no longer have write access to the repo).

@richardlau
Copy link
Member

I very much doubt that GitHub expands the team and checks the permissions of every person on the team.

Confirmed: https://git.luolix.topmunity/t5/How-to-use-Git-and-GitHub/CODEOWNERS-works-with-users-but-not-teams/m-p/4991/highlight/true#M1612

@jasnell
Copy link
Member

jasnell commented May 16, 2018

Boo... that's unfortunate. I'd say let's leave the CODEOWNERS file as it is and have a discussion with github about the limitations here. It may be worthwhile feedback for them.

Trott added a commit to Trott/io.js that referenced this pull request May 16, 2018
@Trott
doc: add .github to CODEOWNERS
2183b25 
Add .github directory to CODEOWNERS file.

PR-URL: nodejs#20733
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott
Copy link
Member Author

Trott commented May 16, 2018

Landed in 2183b25

@Trott Trott closed this May 16, 2018
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@Trott @MylesBorins
doc: add .github to CODEOWNERS
8f489a2 
Add .github directory to CODEOWNERS file.

PR-URL: #20733
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
@Trott Trott deleted the add-github branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

@devsnek devsnek devsnek approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. meta Issues 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.