-
Notifications
You must be signed in to change notification settings - Fork 139
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
Comments
Please be mindful of pinging anyone in the org for an administrative issue like this |
No one was pinged (except for @Trott) |
I think everyone is pinged when you mention |
I've tested it, and that's not the case (phew!) |
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:
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). |
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 |
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.
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 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. |
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. Regarding the proposal of giving triage permission to Regarding moving more teams to 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 |
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) |
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. |
Sorry for the noise, I've moved this to nodejs/node#53897 |
Closing in favor for discussion there. |
Tip
Several nodejs teams are referenced in this issue, but they are not notified because the following snippet is used:
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:
nodejs/node
.I'm not proposing a major overhaul of the Node.js team system, but I suggest the following:
nodejs/node
, similar to thenodejs/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.The text was updated successfully, but these errors were encountered: