nodejs
meta: request reviews from github actions
Part of #51236
This PR moves the process of commenting on a PR to the main repo's Github Actions, instead of it being handled by the github bot.
This is open to discussion, and I'm not a member of the @nodejs/github-bot team, so I'd love them to take a look and see if this looks good :-)
If landed, the new 'initial comment' that will be posted on PRs is:
Hi š! Thank you for submitting this pull-request!
According to the CODEOWNERS file, the following people are responsible for reviewing changes to the files you've modified:
- @nodejs/team
- @nodejs/other-team
They, along with other project maintainers, will be notified of your pull request. Please be patient and wait for them to review your changes.
If you have any questions, please don't hesitate to ask!
@RafaelGSS This PR isn't solely for the TSC to be pinged when a dependency is changed. This moves the process of requesting reviews into the core repo, as mentioned in the past. When the security group mentioned also reviewing from the TSC, I figured I'd add it here as well.
This moves the process of requesting reviews into the core repo, as mentioned in the past.
But, what's the motivation behind it? AFAIK This works pretty well at the moment. Don't you think it would cause just more maintenance burden?
But, what's the motivation behind it? AFAIK This works pretty well at the moment. Don't you think it would cause just more maintenance burden?
I believe it's less cumbersome for collaborators if the code is stored in the main repository and uses a supported Node.js version. As @targos pointed out:
Maintenance of the GitHub bot has become a problem. The machine it's running on is on a quite old version of Debian that doesn't support recent versions of Node.js and we have very few people who know and work on the bot's code (nodejs/github-bot).
Requesting review from @targos, as it was his initial issue that brought the idea of migrating to GH actions.
I still believe this is unnecessary and an overkill in terms of https://github.com/nodejs/security-wg/issues/1329
I still believe this is unnecessary and an overkill in terms of nodejs/security-wg#1329
I opened this before that discussion, and I figured that while I added this, I could also incorporate what was discussion
@RafaelGSS @Uzlopak
This change is not a response to the security discussion. This is migration of requesting reviews away from the GitHub bot and into GitHub actions.
When I saw the security discussion, I figured I could incorporate it here, but you were correct that it needs its own workflow.
+1 on the sentiment of @RafaelGSS - with regards to processes and automation of the project - let's not fix things that are not broken?
let's not fix things that are not broken?
IMO the current setup is (in a way) broken. It's using outdated software and it's hard to maintain, this change would fix part of that, as mentioned in the linked issue.
Maintenance of the GitHub bot has become a problem. The machine it's running on is on a quite old version of Debian that doesn't support recent versions of Node.js and we have very few people who know and work on the bot's code (https://github.com/nodejs/github-bot).
The bot is unmaintained and cannot be easily upgraded. IMO this qualifies as broken
IMO this qualifies as broken
with that in mind, how do we feel about this PR as a solution for part of the GitHub bot's brokenness?
@RafaelGSS are you still blocking?
Is there a specific reason why your -1?
Pretty much https://github.com/nodejs/node/pull/53149#issuecomment-2174243693. I see your point of view, but I believe all of this can be easily managed through a single github action instead of js file. Also, I'm sharing my point of view related to the security-wg request (request TSC review when deps is changed).
fwiw I've removed the security part of this, and it's back to what it was originally: a migration to GH actions, which isn't something that IMO can be achieved via a workflow without JS.
The bot is unmaintained and cannot be easily upgraded. IMO this qualifies as broken
I don't think the bot is unmaintained, exactly, but the server it has been running on has been out of date for a long, long, long time...
...until today! We'll let it bake for a few days, or maybe even weeks, but @richardlau updated it and it now runs on Node.js 20.x and I think we're in a better place.
...until today! We'll let it bake for a few days, or maybe even weeks, but @richardlau updated it and it now runs on Node.js 20.x and I think we're in a better place.
(Side note: Yay!)
Even so, in my opinion, using Github Actions to achieve the same result is a more efficient method for several reasons:
- Increased Security: GitHub Actions operates on ephemeral runners with dynamic tokens, ensuring continued functionality even if the @nodejs-github-bot is compromised.
- Greater Robustness: GitHub Actions eliminates the reliance on webhook calls, streamlining the bot's operation by removing an extra step.
- Reduced Maintenance: GitHub Actions can be automatically updated with @dependabot, eliminating the need for manual updates.
- Reduced Clutter: It may be helpful to store all the files in one place, such as this repo.
WDYT?
@Trott you mentioned that the bot has been / will be updated, so Iād love your feedback on how you feel about this change. Thank you!
@Trott you mentioned that the bot has been / will be updated, so Iād love your feedback on how you feel about this change. Thank you!
I don't have an opinion on whether this should be a bot thing or a GitHub Actions thing. I have lots of opinions on the process itself, but they might be unpopular ones that other maintainers don't share. I'd be interested in their reactions.
-
I dislike the longer initial comment that will be used if this PR lands ("Hi š! Thank you for submitting this pull-request!" and all that). I know there's a general tendency to believe that it's better to provide lots of information and welcoming text, but honestly, this comment is for maintainers and should be optimized for them. Having a comment like this on every pull request is going to cause people to ignore it. Nobody needs most of that information. Just ping the teams and we're good. Less is more here.
-
Building on the previous item: If it were up to me, I would remove the comment entirely and instead add the team to the Request Review list.
-
Going even further: This whole process was added because people on certain teams complained that code affecting their subsystem was landing without their review. The only reason we have this process in place is to ping them so things won't slip by them and to also let reviewers know that maybe they should make sure someone from the relevant team reviews it. If it were up to me, I'd remove most of the teams from CODEOWNERS and leave just the ones that want to be pinged, which isn't very many of them. Probably streams, probably TSC on a few files, probably crypto, and maybe one or two others.
-
Going all the way to the logical conclusion: The big problem for Node.js maintainers is notification overload. This process was supposed to help solve it by enabling contributors to turn off notifications for the repo but still get pinged for specific things they care about. In practice, I don't think very many (any?) people have done that and this process has backfired and created more notifications for people. I don't even think it's working for the folks for whom it was designed (streams, I think?). Maybe it should just be removed. (However, if I'm wrong, and it in fact works super well for even just one or two maintainers, it's probably worth keeping, although perhaps narrowing the scope as in the previous item.)
- ...
Got it! I've adjusted the comment
- ...
While I like the idea of this, every team on the CODEOWNERS file would need to be given triage access or higher to this repository. (I'm aware the collaborator team gives all members that access, but it would need to be done on the team level)
- ...
- ...
Maybe the TSC could meet to decide these kind of larger decisions?
All in all, I think that the future of review requests should be discussed, and if it's to be continued via a comment, handling that via GitHub actions is a good idea.
I've opened an issue in https://github.com/nodejs/admin/issues/895 which will address your comments. The only reason reviews can't be used is because the all-members team doesn't have triage access to this repo (at that level). By changing that, the review request feature can be used.
If it aint broke don't fix it. While I believe moving the infra away from the github bot is the way to go, it doesn't seem like this is going to land anytime soon.