tools: add tool that fills pr-url as code review suggestion
#61247
Conversation
|
Review requested:
|
nodejs-github-bot
added
meta
| const res = await fetch( | ||
| new URL(`repos/${REPO}/pulls/${PR_NUMBER}/files`, 'https://api.github.com'), | ||
| { headers }, | ||
| ); |
There was a problem hiding this comment.
Would it be better to use octokit here, rather than manually interacting with the API?
There was a problem hiding this comment.
I'm fine with either way, by default manual requests should have slightly less overhead. Are there benefits from using octikit here?
There was a problem hiding this comment.
Are there benefits from using octikit here?
Primarily that if the API structure changes, Octokit will adjust accordingly without the need for us to make any manual changes
There was a problem hiding this comment.
We pass the X-GitHub-Api-Version header, I think worrying about breaking changes is premature here. On the other hand, introducing Octokit seems like increasing maintenance burden and supply chain surface attack.
There was a problem hiding this comment.
In terms of potential API changes, i'm still gravitating towards direct fetch. The tool relies on .patch having specific formatting without unified patch headers, so we can't blindly allow switching to future API versions. And after API changes come and v2022-11-28 becomes legacy, it's more likely to be dropped from libraries earlier than from API itself.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
avivkeller
left a comment
avivkeller
left a comment
There was a problem hiding this comment.
The pull_request trigger can't access secrets from fork PRs.
| node-version: latest | ||
| - run: ./tools/actions/suggest-fill-prurl.mjs | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GH_USER_TOKEN }} |
There was a problem hiding this comment.
| GH_TOKEN: ${{ secrets.GH_USER_TOKEN }} | |
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
Even so, fork PR tokens don't have the necessary access. I'd recommend changing the trigger to pull_request_target (with all security precautions needed)
There was a problem hiding this comment.
-1 on pull_request_target, it's simply not worth it. Why wouldn't a fork have permission to comment on a PR on a public repo?
There was a problem hiding this comment.
Why wouldn't a fork have permission to comment on a PR on a public repo?
commenting on a PR requires pull-requests: write permissions, which fork PRs are not granted on the pull_request trigger
There was a problem hiding this comment.
@ the suggestion: i used GH_USER_TOKEN specifically to make it posted from @nodejs-github-bot, is this wrong?
@ the pull_request_target: last time i've checked, using
on:
pull_request:
permissions:
pull-requests: writeallowed posting a PR comment. But not 100% sure if it was from fork or in a PR within same repo.
I'll try to test this properly in https://github.com/nodejs/node-auto-test. Would it be okay to sync its main first and overwrite nodejs/node-auto-test@f15bbc9?
There was a problem hiding this comment.
Nobody should be using nodejs/node-auto-test as upstream, so you have free reign for force-pushing its branches.
5 participants
Having to fill
pr-urlin docs is sometimes annoying, since there's no guaranteed way to predict the PR number before pushing the changes.This PR adds tool that searches for
https://github.com/nodejs/node/pull/FILLMEand postshttps://github.com/nodejs/node/pull/12345(where12345is actual PR number) as code review suggestion (```suggestion).To avoid unnecessary flooding, it triggers only on initial open, and only if pr-url was explicitly set as
https://github.com/nodejs/node/pull/FILLMEin the files.This also could be addressed as local tool, or even as ncu feature (e.g.
git node i-feel-luckyto guess the next number, fill and commit).