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

Ping TSC on deps update not from GithubBot #1329

Open
marco-ippolito opened this issue Jun 13, 2024 · 10 comments · May be fixed by nodejs/node#53492
Open

Ping TSC on deps update not from GithubBot #1329

marco-ippolito opened this issue Jun 13, 2024 · 10 comments · May be fixed by nodejs/node#53492

Comments

@marco-ippolito
Copy link
Member

marco-ippolito commented Jun 13, 2024

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

@Uzlopak
Copy link

Uzlopak commented Jun 13, 2024

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

RedYetiDev commented Jun 17, 2024

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

mhdawson added a commit to mhdawson/io.js that referenced this issue Jun 18, 2024
- 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
- 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
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
- 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

SophonieBouye pushed a commit to SophonieBouye/node that referenced this issue Jun 20, 2024
- 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
- 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
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants