meta: ping TSC for manual deps changes#53492
Conversation
|
Review requested:
|
|
CC @nodejs/security-wg |
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?
.github/workflows/tsc-ping.yml
Outdated
| 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 changes
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
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 |
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. |
|
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 |
|
This hasn't reached consensus, and most times, dependency changes get TSC reviewed anyway. Closing. |
Split from #53149
Fixes nodejs/security-wg#1329