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

chore(action/PR): Use pull_request_target workflow #92

Merged
merged 6 commits into from
Nov 27, 2022

Conversation

amithm7
Copy link
Contributor

@amithm7 amithm7 commented Nov 22, 2022

pull_request_target workflow would be correct workflow from the base repo to run on a PR.

For PR's coming from fork repo's branch, `github.head_ref`
gives a branch name which couldn't be checked out from this
repo (origin).
So, now we checkout the PR branch's head commit. Accordingly, changes are pushed out to fork remote too.
Copy link
Contributor

@ignoramous ignoramous left a comment

Choose a reason for hiding this comment

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

thanks. minor comments only.

.github/workflows/pr.yml Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
Use `GITHUB_OUTPUT` env. var. as recommended by GitHub
Remove checking for changes before push, as it is not necessary.
@amithm7
Copy link
Contributor Author

amithm7 commented Nov 25, 2022

fixed.

Now, another issue has emerged. Action is not able to run git push to this repo or it's fork(s).

remote: Permission to ***/serverless-dns.git denied to github-actions[bot].

This has worked in past: GitHub Action run created a commit by github-actions[bot]: 415d654 (#43) and also in a private repository I tested (3 days ago).

Did some repository security(?) settings change?

Maybe Workflow permission are blocking?: ad-m/github-push-action#96 (comment)

Workflow permission from my test repo, in which git push worked:
image

@ignoramous
Copy link
Contributor

Hm... The permissions haven't changed, and they look like they do in the screenshot you shared (rw permission).

@amithm7
Copy link
Contributor Author

amithm7 commented Nov 25, 2022

Does the current PR have "allow changes from maintainers" disabled?

No, that is enabled.

What I find unexpected here is that the workflow file being run is from the HEAD of this PR (since 2nd commit of this PR).

image

No action was run on the first commit. Where I expected the use workflow file from base repo (main).

Maybe it is a quirk of running the PR's workflow file? (different GitHub Token?)


Deno-deploy's action also pushes code commited by action bot: https://github.com/serverless-dns/serverless-dns/blob/6b9a2e9f3c043a80edbddf0665877da974052c7e/.github/workflows/deno-deploy.yml#L111-L120

@amithm7
Copy link
Contributor Author

amithm7 commented Nov 25, 2022

So,

pull_request event does run workflow file from the PR's head.

If PR is from fork:

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflows-in-forked-repositories

The GITHUB_TOKEN has read-only permissions in forked repositories.

Maybe it is a quirk of running the PR's workflow file? (different GitHub Token?)

Close enough. Different GitHub Token if PR comes from fork vs. origin.

There is also pull_request_target which runs workflow from origin repo, but that comes with a warning of checking out fork repo.

@ignoramous
Copy link
Contributor

ignoramous commented Nov 25, 2022

Well, this is a bit too over the head for me (: github-actions has always been quite a bit complicated due to the nature of the scenarios it runs in...

What's the course of action for us here (I'd imagine pull_request_target, but that comes with scary warnings, I see...) Would a simpler thing be to run the linter and formatter on commits rather than on PRs (but the fork may have actions disabled, so that's there too)?

@amithm7 amithm7 marked this pull request as draft November 26, 2022 16:27
@amithm7
Copy link
Contributor Author

amithm7 commented Nov 27, 2022

Would a simpler thing be to run the linter and formatter on commits rather than on PRs

Actions on fork repo are disabled (default). If run on this repo, it may add unnecessary commits to the main branch.

pull_request_target would be what we want. I think the only insecure thing we are doing here is npm install, for eslint. So, instead we could get eslint from base repo -> checkout PR's HEAD -> run linter -> commit to PR's HEAD.

@amithm7
Copy link
Contributor Author

amithm7 commented Nov 27, 2022

Have changed the workflow to pull_request_target workflow. Which would run the correct workflow from base repo, instead of a PR's (possibly modified) workflow.

Push to fork is still broken as GITHUB_TOKEN owned by github-actions[bot] can't as a repository maintainer, to push to fork. This probably requires creating PAT to act as a maintainer.

@amithm7 amithm7 marked this pull request as ready for review November 27, 2022 12:24
@amithm7 amithm7 changed the title chore(action/PR): fix the branch to fetch & push chore(action/PR): Use pull_request_target workflow Nov 27, 2022
@ignoramous ignoramous merged commit 62bc6db into serverless-dns:main Nov 27, 2022
@amithm7 amithm7 deleted the patch-2 branch November 28, 2022 07:51
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.

None yet

2 participants