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

Feature request: Add pull_request_target event type #12

Open
eruizalo opened this issue Nov 2, 2022 · 6 comments
Open

Feature request: Add pull_request_target event type #12

eruizalo opened this issue Nov 2, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@eruizalo
Copy link

eruizalo commented Nov 2, 2022

It would be nice to use this action in PRs coming from forks. In order to do this the easiest way is to use the pull_request_target event, which has write permissions. This approach is more difficult as you don't know the PR number by the GitHub context, but there are some mechanisms to fetch that.

I'm just a newbie in typescript and my created GitHub actions are small and ad-hoc to my project, but if there is any way I could help I will be glad to do so.

@gkampitakis
Copy link
Owner

hey 👋 thanks a lot for the interest. Haven't worked on this action for a long time and I am also not really familiar with how the pr from a fork differs. So I probably need some help as well 😅

Have you tested this flow?

This approach is more difficult as you don't know the PR number by the GitHub context, but there are some mechanisms to fetch that.

how could you obtain that number?

@eruizalo
Copy link
Author

eruizalo commented Nov 4, 2022

Hi there!

thanks to you for this action, I was thinking about making it myself until I saw yours.

I have tested this flow from my fork: eruizalo/doric#16

Error: Action only supports pull requests

It fails because you are checking the event type, which can only be a pull_request:

if (eventName !== 'pull_request') {

I thought at first this couldn't be so difficult, but now I am realising this might be more complex than I thought.
As long as we need to write in the PR we need the GITHUB_TOKEN, but the permissions are different for forked repos (only read): https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

pull_request event only has read permissions when working on forks, so if we try to write in a PR using this event it will fail (I crashed myself with this using another action). This is done this way to avoid security issues and could be avoided by two different approaches:

  • I think GITHUB_TOKEN can be granted this permissions, but it is not recommended
  • using pull_request_target event

I think I blundered the other day when I said the GitHub context were different because I was thinking about one approach I did for commenting on a PR using workflow_run event... I would have to test this in order to see if we would be able to get the PR number the same way you are fetching it right now.

Give me some days to find this out.

If the fork context does not contain the PR number I would do the same as other actions are doing: request it as a param. There are some actions that could obtain this info or maybe the developer can do something to provide it (In order to comment a PR using workflow_run event I had to upload an artifact from the PR event and download it at the workflow_run... but we will discuss this only if we have no other option)

@eruizalo
Copy link
Author

eruizalo commented Nov 8, 2022

Hello again @gkampitakis,

it seems pull_request and pull_request_target contains very similar GitHub context info, I tried to fix your repo adding this event and testing it in my repo and it kind of worked.

But... the GITHUB_SHA for both events are different so I had to force it in the checkout:

Not sure what would you do to check this case (maybe throw an error if current commit and base branch commit are the same.

@gkampitakis
Copy link
Owner

Hey 👋 had a quick look at this but not sure I have the time to work on this, unfortunately. Maybe it is doable but def needs some testing. I am happy to review any pr and help on the way

@gkampitakis gkampitakis added the enhancement New feature or request label Nov 20, 2022
eruizalo added a commit to eruizalo/github-action-todo-commenter that referenced this issue Mar 7, 2023
@eruizalo
Copy link
Author

eruizalo commented Mar 7, 2023

Hi @gkampitakis I'm really sorry but I do not really know about typescript. I think it should work as I pushed in my branch, but it seems it does not work if I don't regenerate lib/index.js. How can I do it?

@gkampitakis
Copy link
Owner

I believe npm run bundle should be enough.

 "bundle": "tsc && ncc build ./dist/index.js -m -o ./lib",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants