Change some workflows using pull_request to use pull_request_target instead
#112161
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
|
anything else in here that needs to change? https://github.com/search?q=repo%3Adotnet%2Fruntime%20pull_request%20path%3A*.yml&type=code |
Nope. @jeffhandley listed all in the chat and indicated which ones needed to be updated and which ones were ok. |
|
/backport to release/9.0-staging |
|
/backport to release/8.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13147243831 |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/13147246296 |
|
@carlossanlop backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Change workflows to use pull_request_target instead of pull_request event
Applying: Add CODEOWNERS entry
Using index info to reconstruct a base tree...
M .github/CODEOWNERS
Falling back to patching base and 3-way merge...
Auto-merging .github/CODEOWNERS
CONFLICT (content): Merge conflict in .github/CODEOWNERS
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Add CODEOWNERS entry
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
There was a problem hiding this comment.
Sharing some post-merge feedback. But this really only applies to main and I don't think needs to be backported.
I also realized our workflows aren't all using an if condition to ensure they're being run from dotnet/runtime, and therefore the workflows will run on folks' forks too. aspnetcore-sync.yml checks for dotnet/runtime while locker.yml checks just for the repo owner of dotnet. Either of those approaches works. We should check each workflow to confirm we want them to run on forks. And we should include that guidance in the README as well.
| General guidance: | ||
|
|
||
| - Please make sure to include the @dotnet/runtime-infrastructure group as a reviewer of your PRs. | ||
| - Do not use the `pull_request` event. Use `pull_request_target` instead, as documented in [Workflows in forked repositories](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#workflows-in-forked-repositories) and [pull_request_target](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target). |
There was a problem hiding this comment.
I recommend elaborating further to describe the nuance, as it isn't an absolute statement that pull_request_target should always be used. Here's what I suggest:
For workflows that are triggered by pull requests, refer to GitHub's documentation for the
pull_requestandpull_request_targetevents. Thepull_request_targetevent is the more common use case in this repository as it runs the workflow in the context of the target branch instead of in the context of the pull request's fork or branch. However, workflows that need to consume the contents of the pull request need to use thepull_requestevent. There are security considerations with each of the events though.Refer to GitHub's Workflows in forked repositories and pull_request_target documentation for more information.
| @@ -112,3 +112,4 @@ | |||
| /docs/area-owners.* @jeffhandley | |||
| /docs/issue*.md @jeffhandley | |||
| /.github/policies/ @jeffhandley @mkArtakMSFT | |||
| /.github/workflows/ @dotnet/runtime-infrastructure | |||
There was a problem hiding this comment.
Works for me not being explicitly named in the codeowners here, but I'm happy to consult on these at any time so don't hesitate to hit me up for reviews.
There was a problem hiding this comment.
I thought about adding you explicitly but you're part of the runtime-infrastructure group. I think you're more experienced than everyone else so it makes sense to have you mentioned like in the line above.
* main: JIT: Set PGO data inconsistent when flow disappears in cast expansion (dotnet#112147) [H/3] Fix handling H3_NO_ERROR (dotnet#112125) Change some workflows using `pull_request` to use `pull_request_target` instead (dotnet#112161) Annotate ConfiguredCancelableAsyncEnumerable T with allows ref struct and update extensions (dotnet#111953) Delete copy of performance pipelines in previous location (dotnet#112113) Optimize BigInteger.Divide (dotnet#96895) Use current STJ in HostModel and remove unnecessary audit suppressions (dotnet#109852) JIT: Unify handling of InstParam argument during inlining (dotnet#112119) Remove unneeded DiagnosticSource content (dotnet#112116) Improve compare-and-branch sequences produced by Emitter (dotnet#111797) Jit: Conditional Escape Analysis and Cloning (dotnet#111473) Re-enable HKDF-SHA3 on Azure Linux Remove fstream usage from corehost (dotnet#111859)
…t` instead (dotnet#112161) * Change workflows to use pull_request_target instead of pull_request event * Add CODEOWNERS entry * Add initial readme
…t` instead (dotnet#112161) * Change workflows to use pull_request_target instead of pull_request event * Add CODEOWNERS entry * Add initial readme
From https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target :
There are other workflows that are also using pull_request, but we discussed them offline and they're ok using it: jit_format.yml, markdownlint.yml, and all under src/native/external.
I also reduced the scope of the two existing workflows that check labeling.