Skip to content

chore: run github actions on pull_request instead of push #1035

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 15, 2022
Merged

chore: run github actions on pull_request instead of push #1035

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02fafd5
chore: run github actions on pull_request instead of push
coadler Apr 15, 2022
a51a338
pull_request_target too
coadler Apr 15, 2022
3d35bb0
remove pull_request_target
coadler Apr 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/coder.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ name: coder

on:
push:
branches:
- main
tags:
- "*"
Comment on lines +5 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pull_request:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has to be pull_request_target for external contributors to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


workflow_dispatch:

Expand Down