Skip to content
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.

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

Use pull_request instead of pull_request_target #49

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

MattijsKneppers
Copy link
Contributor

@MattijsKneppers MattijsKneppers commented Oct 16, 2024

This PR changes the GitHub Action trigger from pull_request_target to pull_request.

pull_request_target seemed to be recommended for working with forks as laid out in this article:

In order to protect public repositories for malicious users we run all pull request workflows raised from repository forks with a read-only token and no access to secrets. This makes common workflows like labeling or commenting on pull requests very difficult. In order to solve this, we’ve added a new pull_request_target event (...)

However this has had the effect that for PRs coming from a branch of this repository as opposed to a forked repository, tests ran on the base of the PR instead of taking the changes of the PR into account:

(...) we’ve added a new pull_request_target event, which behaves in an almost identical way to the pull_request event with the same set of filters and payload. However, instead of running against the workflow and code from the merge commit, the event runs against the workflow and code from the base of the pull request.

This explains why it was possible to merge PR #45 while the tests failed after the merge.

The effect this PR has on PRs coming from forks is not clear at this time but since that happens rarely we accept to address that once we run into actual issues in that scenario.

@MattijsKneppers MattijsKneppers merged commit 7dab8d1 into main Oct 17, 2024
4 checks passed
@MattijsKneppers MattijsKneppers deleted the mkp-fix-actions-triggers branch October 17, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant