Uh oh!
There was an error while loading. Please reload this page.
meta: ping TSC for manual deps changes - #53492
Conversation
nodejs-github-bot
commented
Jun 17, 2024
Review requested:
|
avivkeller
commented
Jun 17, 2024
CC @nodejs/security-wg |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aduh95
left a comment
There was a problem hiding this comment.
I'm -1 on requiring TSC approval for non-semver-major changes, please update the commit message so it says something like meta: ping TSC on PR with dependency changes. I'm also -0.5 on pinging the TSC, the signal/noise ratio is already rather low, this is likely not going to be an improvement on that front. Is there any reason you want to propose this change?
| with: | ||
| labels: blocked |
There was a problem hiding this comment.
| with: | |
| labels: blocked |
There was a problem hiding this comment.
The reason I can think of is that manual dependency updates (not from bot) require a level of trust and accurate review in order to avoid malicious binaries or malicious minified code. Does pinging tsc fix this? I dont know 🤷 |
I'll update the PR title now, but I won't be able to rebase for a few days, so if someone with write access would like to do so, feel free.
EDIT: I see @marco-ippolito already commented, it didn't load earlier, sorry |
deps changesCo-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow
commented
Jun 17, 2024
does this solve a real problem? are such changes blocked/delayed because TSC members don't see them? As per my experience this ping is redundant, TSC members are mostly aware of new PRs anyway |
MoLow
commented
Jun 17, 2024
setting a rule/guideline should happen before automating it. I would start by adding this to the collaborator guide, https://github.com/nodejs/node/blob/main/doc/contributing/collaborator-guide.md#breaking-changes or probably opening an issue to discuss it before adding a rule |
The changes were discussed in nodejs/security-wg#1329, I figured implementing them would be helpful, giving the members of such issue one less task to complete. I in no way intended to cause a stir, I was attempted to complete a chore via this PR. |
mhdawson
commented
Jun 18, 2024
PR to update contributing guide with additional guidance - #53499 |
I guess instead of pinging tsc as @mhdawson mentined here nodejs/security-wg#1329 (comment) we can add a comment asking reviewers to follow the guidelines on reviewing deps update PR #53499 |
I'll fix the commit message when I get a chance, apologies for the delay |
avivkeller
commented
Aug 19, 2024
This hasn't reached consensus, and most times, dependency changes get TSC reviewed anyway. Closing. |
Split from #53149
Fixesnodejs/security-wg#1329