Make it clear that this is not suitable for forked PRs#140
Make it clear that this is not suitable for forked PRs#140rschristian merged 2 commits intopreactjs:masterfrom
Conversation
The README does not mention that the action doesn't work for fork PRs. Another developer added this to our repo and I later 'fixed' the fact that the comments weren't showing up by elevating the permissions, not knowing that there were security implications. The 2-step workaround in preactjs#54 should really be the primary installation method with a 'by the way if you know what you are doing, you can do it in one step' note at the bottom of the readme ... I can attempt that write up but I'd need to delve into this action more, and I think maybe there are others who know how to do this better
The 2-step method is extremely fragile and has a massive surface area for issues. I've built methods of doing that and let me just say that it's been a constant source of headaches even when we own the config; having two separate actions you drop into two separate workflows that you can't easily debug sounds like a nightmare, personally. I don't think that's a good idea, even if the current setup has limitations. |
This is an alternative to preactjs#140
We're currently attempting to do that here: |
Good luck, it hasn't been fun (IME -- hopefully your experience differs). If it can be of any use, Preact has an example of doing this here:
This bit took a long time to figure out: |
rschristian
left a comment
rschristian
left a comment
There was a problem hiding this comment.
Hopefully this will suffice, apologies for the delay. Super busy these days unfortunately
2 participants
The README does not mention that the action doesn't work for fork PRs. Another developer added this to our repo and I later 'fixed' the fact that the comments weren't showing up by elevating the permissions, not knowing that there were security implications. The 2-step workaround in #54 should really be the primary installation method with a 'by the way if you know what you are doing, you can do it in one step' note at the bottom of the readme ... I can attempt that write up but I'd need to delve into this action more, and I think maybe there are others who know how to do this better