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

@nodejs team permissions #895

Closed
avivkeller opened this issue Jul 17, 2024 · 13 comments
Closed

@nodejs team permissions #895

avivkeller opened this issue Jul 17, 2024 · 13 comments

Comments

@avivkeller
Copy link
Member

avivkeller commented Jul 17, 2024

Tip

Several nodejs teams are referenced in this issue, but they are not notified because the following snippet is used:

**@[]()nodejs/team**

This format does not ping the teams.


After @Trott highlighted some points in this comment, I reviewed the configuration of the @nodejs/... teams. Based on my findings, I believe two changes could significantly improve the situation:

  1. The @nodejs/all-members team does not actually include all members. Out of over 340 organization members, only 142 are part of this team.
  2. Currently, reviewers are pinged via comments instead of using GitHub's review feature because the all-members team lacks triage access to nodejs/node.

I'm not proposing a major overhaul of the Node.js team system, but I suggest the following:

  1. All teams should be placed under @nodejs/all-members.
  2. @nodejs/all-members should be granted Triage permissions for nodejs/node, similar to the nodejs/moderation repo. This would allow the CODEOWNERS file to automatically request reviews. Most (if not all) members already have triage or higher access to the repository, just not at the all-members level.
@avivkeller avivkeller changed the title Organization of the @nodejs teams @nodejs team permissions Jul 17, 2024
@benjamingr
Copy link
Member

Please be mindful of pinging anyone in the org for an administrative issue like this

@avivkeller
Copy link
Member Author

avivkeller commented Jul 17, 2024

No one was pinged (except for @Trott)

@marco-ippolito
Copy link
Member

No one was pinged (except for @Trott)

I think everyone is pinged when you mention @nodejs

@avivkeller
Copy link
Member Author

avivkeller commented Jul 17, 2024

I think everyone is pinged when you mention @nodejs

I've tested it, and that's not the case (phew!)

@Trott
Copy link
Member

Trott commented Jul 17, 2024

As demonstrated by the negative reaction to a single ping here that might not have even actually happened, it's going to be very much an uphill battle trying to get engagement and consensus on a significant overhaul of the org setup just so that people can get even more pings (or the same amount of pings but done differently). I think this is the wrong approach and that we should abandon CODEOWNERS, or at least greatly reduce the number of files and teams in the CODEOWNERS file.

I think the problem is three-fold:

  1. People think they want fewer pings but also to be pinged only on things that they care about.
  2. There is no reliable way to programatically determine what PRs people will want to be pinged about. They might think there is ("I want all the changes to these files") but that leads us to the last problem....
  3. People think they know what they want but are sometimes not good at knowing what they want.

I think the CODEOWNERS file was a well-intentioned experiment that good people put work into but, unfortunately, has not worked out. There's nothing wrong about that. Failed experiments are part of the process. If all experiments succeed, they're probably not really experiments.

I think people generally ignore the CODEOWNERS ping. I could be wrong, but I think it's mostly noise at this point and we should get rid of it or at least vastly reduce its scope. Most files do not need CODEOWNERS entries.

This is related to the fact that the typical team contains one or two active people and six or eight people that will never respond to a ping and aren't involved in the org anymore. The only real solution to that is for one or more TSC members to actively manage the teams, removing a few people (and maybe adding a few) every few days (because doing all the teams in one sitting is just not possible).

@aduh95
Copy link
Contributor

aduh95 commented Jul 17, 2024

No one was pinged (except for @Trott)

I think everyone is pinged when you mention @nodejs

I very much doubt it, GitHub docs says it's only when mentioning people or teams (https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#mentioning-people-and-teams), nothing about mentioning orgs. Also the notification I've received says subscribed, not mention nor team mention.

@avivkeller
Copy link
Member Author

avivkeller commented Jul 17, 2024

significant overhaul

I wouldn't call it significant, it's just moving teams under a single roof, and possibly changing the permissions of that house.


The benefit of a CODEOWNERS ping rather than the comment is that it doesn't subscribe them to the issue (IIRC). It lets them know they've been pinged, but doesn't subscribe them to future updates.

Whether or not CODEOWNERS is the best method to ping users, I think the comment is too much: it pings and subscribes users to the issue.

I think this is the wrong approach and that we should abandon CODEOWNERS, or at least greatly reduce the number of files and teams in the CODEOWNERS file.

If that's the case, the bot ping can be removed, and the CODEOWNERS file deleted.

In my opinion, the order of most pings to least pings is
3. Bot comment
2. CODEOWNERS review
\1. None

If the ping was to be kept, I think doing it using GitHub reviews would be better, but if the ping was to be removed, than that can all be ignored.

The benefit of removing the ping entirely is that organization members monitor the issues, so if someone needed to be pinged, it could be done manually by members.


However, I still think all teams should fall under the all-members team, for ease-of-management.

And, if the all-members team is given triage access (or explicit read might work), members can request reviews from teams when needed.

@aduh95
Copy link
Contributor

aduh95 commented Jul 17, 2024

Ideally getting notified when some files are modified would be a feature of GitHub, so folks do not need to ask for someone to add them to a team (and remove them when they no longer want the notifications), and we do not need to manage teams at all (except those who give write access to some repo). Having said that, I realize it's convenient to be able to ping teams in e.g. issues, and we might miss having teams in those cases.
Realistically, we have teams, and have to use them (and we the TSC should do a better job at managing them).

Regarding the proposal of giving triage permission to all-members: -1, that would force us to either gate-keep joining ping-only teams, and force us to "clean up" inactive members.

Regarding moving more teams to all-members: why? why do you care? why should we care?

Regarding replacing bot pings with bot requesting reviews: why not, that seems relatively easy to do, I've seen it done on other open-source repo, I'd say it's a good idea, let's go for it (but it's not an issue for admin, it only requires someone to open a PR).
Regarding giving the possibility to request review from teams instead of individuals: meh, not worth it IMO.

@avivkeller
Copy link
Member Author

avivkeller commented Jul 17, 2024

Regarding replacing bot pings with bot requesting reviews: why not, that seems relatively easy to do, I've seen it done on other open-source repo, I'd say it's a good idea, let's go for it (but it's not an issue for admin, it only requires someone to open a PR).
Regarding giving the possibility to request review from teams instead of individuals: meh, not worth it IMO.

The reason this is an issue for admin, is because all teams need to be under all-members (or each team needs to have added permissions) for this to work. Once all teams are under all-members, that team needs to be given triage permissions (which is the minimum needed for reviews on teams)

@aduh95
Copy link
Contributor

aduh95 commented Jul 17, 2024

No, it can work by requesting reviews from individual instead of teams. The bot can check the CODEOWNERS file, list the members of the listed teams, and request reviews from them instead of pinging the team.

@avivkeller
Copy link
Member Author

No, it can work by requesting reviews from individual instead of teams. The bot can check the CODEOWNERS file, list the members of the listed teams, and request reviews from them instead of pinging the team.

Got it. I can open a PR later today. Feel free to close this if you think it's resolved, but otherwise feel free to keep this open.

@avivkeller
Copy link
Member Author

avivkeller commented Jul 17, 2024

Sorry for the noise, I've moved this to nodejs/node#53897

@avivkeller
Copy link
Member Author

Closing in favor for discussion there.

@avivkeller avivkeller closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants