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

meta: ping TSC for manual deps changes #53492

Closed
wants to merge 7 commits into from

Conversation

RedYetiDev
Copy link
Member

Split from #53149
Fixes nodejs/security-wg#1329

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jun 17, 2024
@RedYetiDev RedYetiDev added the security Issues and PRs related to security. label Jun 17, 2024
@RedYetiDev
Copy link
Member Author

CC @nodejs/security-wg
CC @nodejs/tsc

.github/workflows/tsc-ping.yml Outdated Show resolved Hide resolved
.github/workflows/tsc-ping.yml Outdated Show resolved Hide resolved
.github/workflows/tsc-ping.yml Outdated Show resolved Hide resolved
.github/workflows/tsc-ping.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 20 to 21
with:
labels: blocked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with:
labels: blocked

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marco-ippolito
Copy link
Member

marco-ippolito commented Jun 17, 2024

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?

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 🤷

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jun 17, 2024

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'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.

Is there any reason you want to propose this change?

@marco-ippolito


EDIT: I see @marco-ippolito already commented, it didn't load earlier, sorry

@RedYetiDev RedYetiDev changed the title meta: require TSC approval for dependency changes meta: ping TSC for manual deps changes Jun 17, 2024
@MoLow
Copy link
Member

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
Copy link
Member

MoLow commented Jun 17, 2024

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 🤷

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jun 17, 2024

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
Copy link
Member

PR to update contributing guide with additional guidance - #53499

@marco-ippolito
Copy link
Member

marco-ippolito commented Jun 20, 2024

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jun 20, 2024

I'll fix the commit message when I get a chance, apologies for the delay

@RedYetiDev
Copy link
Member Author

This hasn't reached consensus, and most times, dependency changes get TSC reviewed anyway. Closing.

@RedYetiDev RedYetiDev closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project. security Issues and PRs related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ping TSC on deps update not from GithubBot
6 participants