Better documentation and/or handling of forks

[tunetheweb]

First up, thanks for the excellent GitHub action!

However, I was surprised recently to find this doesn't work on pull requests from forks and fails silently, with only this error message hidden in the GitHub action logs to indicate it didn't complete:

    - Processing: /github/workspace/src/static/images/test.png
->> Generating markdown…
->> Committing files…
	 *  Head SHA: 41a31c3fc57433f287e5091d538e79504bb4310c
	 *  Tree 5849f57c3558964e83b965ed66b32332a8e077ac
	 *  Converting images to blobs…
(node:1) UnhandledPromiseRejectionWarning: HttpError: Resource not accessible by integration
    at response.text.then.message (/usr/local/src/image-actions/node_modules/@octokit/request/lib/request.js:56:27)
    at processTicksAndRejections (internal/process/task_queues.js:86:5)
(node:1) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:1) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Digging into GitHub actions I understand why – because upstream repos don't have access to change downstream branches – and on second look, I did find this mentioned on the README:

The GITHUB_TOKEN secret is automatically generated by GitHub. This automatic token is scoped only to the repository that is currently running the action.

However this was not clear to me, and imagine it isn't to others either and accepting pull requests from forks is really what GitHub is all about.

I've a few suggestions to improve this:

Making people more aware of this restriction

First of all I would suggest expanding on that line in the README to explain what it means to forks. I'm happy to throw in a suggested pull request with wording for this if you'd like?

We could also add advice to only run this GitHub action on pull requests from and to the same repo, as pointless run other than that so a waste of time and compute. Not sure exactly how to do this but happy to look into the appropriate if: line to add to the suggested .yml config if you want?

How can we fix this?

I presume getting forked repos to implement a Personal Access Token would work, but not sure that's a great idea to be honest. It only works for regular contributors who will go to this effort of setting up a PAT – and regular contributors might as well work off of the main repo.

A better option might be to allow the action to also run on pushes. Then could run it on master after the PR has been approved. Ideally that would allow the option to commit directly to the push branch (master in this case - for those that are crazy enough to want that) or to open a new pull request with the compressed images after merging (probably better!).

Alternatively changing the action to only do the compression, and not handle the commit, would allow the Checkout GitHub Action to handle the commit (though this does an extra config line to checkout the correct head branch), or something like the Create Pull Request GitHub Action to create a new pull request, after merging pull requests from forks. Might mean less code for you to maintain, more flexibility, but at the cost of more complicated workflow set up. Or alternatively make this optional for those (like me!) that want the power of handling the commit differently. Probably need to do that to maintain backwards compatibility to be honest.

Anyway, quite a lot in there, so maybe better split into a number of issues but thought best to start a discussion first so let me know your thoughts.

[benschwarz]

Wow, thanks so much for your detailed assessment here @bazzadp !

I agree with pretty much everything you've said, including the "right" ways to handle this.

I think the original GitHub actions documentation that this was built against didn't include anything about forks, but even if it did, it totally wasn't a consideration that was made.

First of all I would suggest expanding on that line in the README to explain what it means to forks. I'm happy to throw in a suggested pull request with wording for this if you'd like?

That would be fantastic.

We could also add advice to only run this GitHub action on pull requests from and to the same repo, as pointless run other than that so a waste of time and compute. Not sure exactly how to do this but happy to look into the appropriate if: line to add to the suggested .yml config if you want?

This would be a better and safer default 👍 If you've got the capacity to pick this up as well, I'd appreciate it.

I presume getting forked repos to implement a Personal Access Token would work, but not sure that's a great idea to be honest.

I agree, that's too much and it's just too hard.

A better option might be to allow the action to also run on pushes.

Maybe so, although I have concerns that this makes it too easy for people to blindly trust this against their master branch. I'd prefer to not make that easy to do.

Alternatively changing the action to only do the compression, and not handle the commit, would allow the Checkout GitHub Action to handle the commit (though this does an extra config line to checkout the correct head branch), or something like the Create Pull Request GitHub Action to create a new pull request, after merging pull requests from forks.

I think this would be a good improvement. I'm certain there are people who could benefit from being able to run image-actions as part of their CI (and I know that @chabad360 was aiming at this workflow too).

If this were a configurable option where people could choose their preferred behavior I'd be all in support of that. 👍

As I mentioned earlier if you've the capacity to help with all or some of these items it'd be really appreciated. Otherwise feel free to extract them as separate issues so we can track against them.

Thanks again ✨

[tunetheweb]

Cool. Will have a look.

[benschwarz]

According to the GitHub actions environment variable docs, GITHUB_HEAD_REF and GITHUB_BASE_REF are set only for forked repos. 💭

[tunetheweb]

According to the GitHub actions environment variable docs, GITHUB_HEAD_REF and GITHUB_BASE_REF are set only for forked repos. 💭

OK, but what does that matter?

It's easy to stop this running for forks by adding this to the Action YML as suggested in the PR:

    # Only run on Pull Requests within the same repository, and not from forks
    if: github.event.pull_request.head.repo.full_name == github.repository

The issue is I want this to run after forks are merged.

[benschwarz]

OK, but what does that matter?

I was just leaving notes, more than anything else 😄

[claudenirmf]

Folks, thank you very much this amazing action and the very informative discussion. However, I still have a question. If someone forks my repo, then the forked repo has this action as well. In this case, isn't the action supposed to run on the forked repo while performing the pull request back to the original repo?

I'm sorry if I was too confusing or if I missed something.

[tunetheweb]

What I do is:

• For same repo PRs compress and add to the open PR
• For fork PRs do not run the action (since it won't work), and then run it on merge to main (or master) and open a new PR for the compressed images.

I documented that here: https://github.com/calibreapp/image-actions#process-pull-requests-from-forked-repositories

And if curious to see a real-life implementation of this, then look at this workflow: https://github.com/HTTPArchive/almanac.httparchive.org/blob/main/.github/workflows/compress-images.yml