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_target #41

Closed
domenkozar opened this issue May 25, 2021 · 4 comments · Fixed by #331
Closed

Use pull_request_target #41

domenkozar opened this issue May 25, 2021 · 4 comments · Fixed by #331
Labels
documentation Improvements or additions to documentation

Comments

@domenkozar
Copy link

For contributions from forked repositories, pull_request_target is needed so that GITHUB_TOKEN has write access to the repo.

That means though that the default checked out code is from base, so it needs to cherry-pick things correctly.

@korthout
Copy link
Owner

korthout commented May 25, 2021

Interesting, I was unaware this existed. For completeness, here's a link to the docs.

I think this will work out of the box for your repository (and forked repos) by just changing the event trigger in your workflow definition from:

on:
  pull_request:
    types: [closed]

to:

on:
  pull_request_target:
    types: [closed]

Please let me know how that goes. If it does work, I'll update the README here.

@korthout
Copy link
Owner

Next to changing the trigger, the suggested commands to manually cherry-pick the commits also don't work for forked pull requests.

It starts with fetching the target branch, which might not be available on the forked repo. As far as I can tell at this time, you'll need to add the main repository as an additional remote (git remote main <mainrepo.git>), fetch the target from that remote, refer to that target when adding the worktree.

It may also be that the baseref is not available on the forked repo, so a git fetch main master might be necessary before determining the ancref.

I dislike having to add the main repo as an additional remote, because it changes the local config of that user and will probably fail if it was already added earlier. However, I have not yet found a better solution.

@korthout
Copy link
Owner

@domenkozar This trigger seems to have been working correctly for you. Did you run into any problems?

If not, I'll just consider this a documentation improvement.

@domenkozar
Copy link
Author

It works well, although I'm not sure that manual instructions still work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants