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.
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:
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:
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.