Is there a reason to filter this? It's always confused me.

If we don't limit branches for push, the workflow will run twice on every commit since both push and pull_request match

I think this has to bepull_request_target for external contributors to work.

I was gonna test it with just pull_request first and see if we need to add pull_request_target, i can just probably add it now

Looks like we dopull_request on code-server and it works for external contributors:

https://github.com/coder/code-server/blob/main/.github/workflows/ci.yaml#L7-L9

For those not familiar, I was curious what the difference was and found a great explanation here:nf-core/tools#765 (comment)

Wouldpull_request_target mean that changes in github actions won't run for the PR?

I'm not sure to be honest. Based on this below:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow.

I'm interpreting this meaning the event/github actions will run as if it were running in the context of the fork (rather than the context of the repo you're PR'ing into). Does that sound right to you?

It seems to me it means it runs the workflow defined in the base (the repo/branch the pr is targeting). Thisseems to mean it ignores changes to the workflow scripts in PRs until they're merged. This may be fine tbh, since we can test ourselves withworkflow_dispatch.

More details here:https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Avoid using pull_request_target if the workflow doesn’t need write repository permissions and doesn’t use any repository secrets. They can simply use the pull_request trigger instead.

For CI, I don't think we need write permissions or secrets so it sounds likepull_request is the way to go.

This seems to mean it ignores changes to the workflow scripts in PRs until they're merged.

Ah, I see what you're saying. Yeah, I agree.