chore: run github actions on pull_request instead of push #1035
Conversation
| branches: | ||
| - main | ||
| tags: | ||
| - "*" |
There was a problem hiding this comment.
Is there a reason to filter this? It's always confused me.
There was a problem hiding this comment.
If we don't limit branches for push, the workflow will run twice on every commit since both push and pull_request match
| tags: | ||
| - "*" | ||
|
|
||
| pull_request: |
There was a problem hiding this comment.
I think this has to be pull_request_target for external contributors to work.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Looks like we do pull_request on code-server and it works for external contributors:
https://github.com/coder/code-server/blob/main/.github/workflows/ci.yaml#L7-L9
There was a problem hiding this comment.
For those not familiar, I was curious what the difference was and found a great explanation here: nf-core/tools#765 (comment)
There was a problem hiding this comment.
Would pull_request_target mean that changes in github actions won't run for the PR?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It seems to me it means it runs the workflow defined in the base (the repo/branch the pr is targeting). This seems 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 with workflow_dispatch.
There was a problem hiding this comment.
There was a problem hiding this comment.
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 like pull_request is the way to go.
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## main #1035 +/- ##
==========================================
- Coverage 67.54% 67.38% -0.16%
==========================================
Files 259 259
Lines 15059 15059
Branches 151 151
==========================================
- Hits 10172 10148 -24
- Misses 3873 3893 +20
- Partials 1014 1018 +4
Continue to review full report at Codecov.
|
This might fix fork pull requests