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.

Sign up for GitHub

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

Jump to bottom

Ping TSC on deps update not from GithubBot #1329

Open
marco-ippolito opened this issue Jun 13, 2024 · 12 comments
Open

Ping TSC on deps update not from GithubBot #1329

marco-ippolito opened this issue Jun 13, 2024 · 12 comments
Labels
good first issue stale

Comments

@marco-ippolito
Copy link
Member

marco-ippolito commented Jun 13, 2024 •
edited
Loading

We receive dependency update such as nodejs/node#53406 that are created by users that might be hard and risky to review. We should probably ping TSC whenever we receive changes in the deps folder not from a GithubBot
@UlisesGascon
Copy link
Member

I strongly agree

@RafaelGSS
Copy link
Member

Context: https://openjs-foundation.slack.com/archives/C03Q9MS3KFB/p1718290908194829?thread_ts=1718036013.366919&cid=C03Q9MS3KFB

@Uzlopak
Copy link

Uzlopak commented Jun 13, 2024 •
edited
Loading

To create a comment in a PR in nodejs/node repository, even when providing a PR from a fork you need to listen on a pull_request_target event . This can have security implications. Maybe @lirantal wants to comment on that.

I dont think we need to write a new action.

So something like the following could be enough? Untested(!)

on:
  pull_request_target:
    paths:
      - 'deps/**'

jobs:
  ping-tsc:
    runs-on: ubuntu-latest
    name: Ping the TSC
    steps:
      - name: Ping the TSC
        if: >
          github.event.pull_request.user.login != 'nodejs-github-bot'
        uses: thollander/actions-comment-pull-request@v2
        with:
          message: Pinging @tsc because of an irregular change in the deps folder
          comment_tag: ping-tsc

@Uzlopak
Copy link

Uzlopak commented Jun 13, 2024

Maybe it makes also sense to add a "blocked" label to the PR.

@richardlau
Copy link
Member

We should probably ping TSC whenever we receive changes in the deps folder not from a GithubBot

Pinging the TSC for changes to deps should be easy (add them as a CODEOWNER and then our bot should then ping the team in a comment to the PR) but we have no logic AFAIK for checking who originated the PR.

@RedYetiDev
Copy link
Member

I can make a workflow if you'd like

@RedYetiDev RedYetiDev mentioned this issue Jun 14, 2024
Closed
@RedYetiDev
Copy link
Member

RedYetiDev commented Jun 17, 2024 •
edited
Loading

nodejs/node#53149 migrates the requesting reviews to GitHub actions, and also implements this in the process

@RedYetiDev RedYetiDev mentioned this issue Jun 17, 2024
Closed
mhdawson added a commit to mhdawson/io.js that referenced this issue Jun 18, 2024
@mhdawson
doc: add additional guidance for PRs to deps
f8d57f9 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
mhdawson added a commit to mhdawson/io.js that referenced this issue Jun 18, 2024
@mhdawson
doc: add additional guidance for PRs to deps
ec3ed97 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
@mhdawson mhdawson mentioned this issue Jun 18, 2024
Closed
@mhdawson
Copy link
Member

PR - to add guidance to contributing docs - nodejs/node#53499

mhdawson added a commit to nodejs/node that referenced this issue Jun 20, 2024
@mhdawson
doc: add additional guidance for PRs to deps
53e9106 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@mhdawson
Copy link
Member

Thinking about this a bit, instead of a ping to the TSC, I think adding a comment to the PR with a warning to collaborators that updates to dependencies should generally be generated by the automation and pointing to https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies for more information would be more beneficial.

@RedYetiDev
Copy link
Member

I can modify my PR if you'd like

sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
@mhdawson @sophoniie
doc: add additional guidance for PRs to deps
aee48f6 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit to nodejs/node that referenced this issue Jun 21, 2024
@mhdawson @targos
doc: add additional guidance for PRs to deps
30858ec 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
@mhdawson @bmeck
doc: add additional guidance for PRs to deps
734ebf9 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
marco-ippolito pushed a commit to nodejs/node that referenced this issue Jul 19, 2024
@mhdawson @marco-ippolito
doc: add additional guidance for PRs to deps
e7f9980 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
marco-ippolito pushed a commit to nodejs/node that referenced this issue Jul 19, 2024
@mhdawson @marco-ippolito
doc: add additional guidance for PRs to deps
c13600f 
- add additional guidance based in discussion related
  to recent PR to dependency and discussion within the
  security-wg slack channel.

Refs: nodejs/security-wg#1329

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #53499
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@github-actions GitHub Actions
Copy link
Contributor

This issue has been inactive for 90 days. It will be closed in 14 days unless there is further activity or the stale label is taken off.

@github-actions github-actions bot added the stale label Sep 19, 2024
@RafaelGSS RafaelGSS removed the stale label Sep 19, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue has been inactive for 90 days. It will be closed in 14 days unless there is further activity or the stale label is taken off.

@github-actions github-actions bot added the stale label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

meta: ping TSC for manual deps changes RedYetiDev/node
7 participants
@Uzlopak @UlisesGascon @richardlau @mhdawson @RafaelGSS @marco-ippolito @RedYetiDev