-
-
Notifications
You must be signed in to change notification settings - Fork 124
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
docs(README): note pull_request_target for fork-based flow #24
Conversation
And looks like it needs to be changed in your workflow :-)
|
As noted here: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ With proposed doc PR in #24
@mikehardy Would you like to rebase this on the latest master so we can try if the token works correctly? |
Sure - I used the web UI for the patch workflow, let me clone local rebase and push |
boom! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool! 🎉
README.md
Outdated
@@ -4,6 +4,8 @@ This is a [Github Action](https://github.com/features/actions) that ensures that | |||
|
|||
This is helpful when you're using [semantic-release](https://github.com/semantic-release/semantic-release) with the Conventional Commits preset. When using the "Squash and merge" strategy, Github will suggest to use the PR title as the commit message. With this action you can validate that the PR title will lead to a correct commit message. | |||
|
|||
Note that if you have a fork-based workflow you will want to use [`pull_request_target`](https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/) in place of `pull_request` so the API token is valid for status reporting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense that we just use pull_request_target
in the snippet below or is there any drawback to this?
If so, I guess we could move this section to the bottom of the README and re-word it to something like:
Note that the usage of
pull_request_target
as the event trigger is necessary for a fork-based workflow so the API token is valid for status reporting.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 good question. I think pull_request_target is probably the best. We're not comparing code so it's okay the code used for the check is from the branch point (which is how pull_request_target protects secrets etc). It's just the title.
I can't think of any drawback to having pull_request_target be the default, but without it this fails on fork-based workflows.
So I think pull_request_target is likely the best default yes
Switch the actual default in the snippet instead of just explaining Then move explanation for those curious to the bottom
Okay @amannn I agree with both your suggestions and just implemented them. I was using the Web UI to do it quickly so now there are 3 commits where one would do but I assume you can squash :-). |
Perfect, thanks a lot for your help! |
Thinking about integrating this but did a deep-dive on the issues first, seems like one of the biggies is solved now but not noted yet since it's new info, here's a maybe-useful edit?