build-tools
build-tools copied to clipboard
electron
feat: support forks in `e cherry-pick`
@MarshallOfSound, does maintainer_can_modify not allow the bot to do its thing?
I think there's a real use-case for non-maintainers to want to use e cherry-pick, I know I had the need and wrote this code before I even saw that @samuelmaddock had opened the issue. If maintainer_can_modify doesn't let the bot work, I think we can find some middle ground with e cherry-pick which will at least make it easier for non-maintainers to do cherry picks from upstream.
I don't believe maintainer_can_modify allows apps to push to your fork (untested) but the actual issue is the app token won't be provided to the forked CI build for security reasons. So it won't have a token to push it, I'd hate to make it easy for these PRs to be made only for 80% of them to need to be rebased / conflict merged anyway.
The following assumes maintainer_can_modify can be tested and does allow bots with write access on a repo to push commits to fork PRs. If not, I think we could abuse something like @electron-bot to be the "maintainer" to do the commit?
Could we redesign PatchUp bot a bit to make this viable? I don't have the full picture here, but from what I've scrapped together, what if the bot listened to CircleCI webhooks looking for jobs failing, then it gets the artifacts for the job, looks for the patches/update-patches.patch artifact, and if it exists, push the commit to the fork? Then PatchUp bot would be a more general solution to the problem.
@dsanders11 There is no webhook, it all happens from the CI machine.
https://github.com/electron/electron/blob/main/.circleci/build_config.yml#L279-L302
what if the bot listened to CircleCI webhooks looking for jobs failing, then it gets the artifacts for the job, looks for the patches/update-patches.patch artifact, and if it exists, push the commit to the fork? Then PatchUp bot would be a more general solution to the problem.
This would be a potential security hole as it would allow a forked PR to apply an arbitrary git patch coming from an authenticated user (in this case a github app) which on various systems may be considered trusted. I'm relatively confident in saying there's no safe way to do this without drastic rewrites / rearchitectures of how we do cherry picks.
It's unfortunate but the reality is we can't afford forks the same level of trust / ease of use as our maintainer branches 🤷
Also in a similar vein.
If not, I think we could abuse something like @electron-bot to be the "maintainer" to do the commit?
We are actively trying to reduce the usage of that account and replacing it with much more scoped GitHub apps where we can
@dsanders11 There is no webhook, it all happens from the CI machine.
Yes, I know it doesn't currently use a webhook, I was suggesting it could be changed to.
This would be a potential security hole as it would allow a forked PR to apply an arbitrary git patch coming from an authenticated user (in this case a github app) which on various systems may be considered trusted. I'm relatively confident in saying there's no safe way to do this without drastic rewrites / rearchitectures of how we do cherry picks.
@MarshallOfSound Can you expand on this? Not sure I follow what the security risk would be there. The commit would be made on the fork's branch, and they're already giving permission to allow maintainers to make commits there. Is the concern that the commit would be originating from the GitHub app, which makes it look trusted to Electron infra stuff? With "squash and merge" isn't that commit just an implementation detail which will fall away once merged? If the concern is the author of the commit, could a separate, considered untrusted, GitHub app make those commits for forks?
@MarshallOfSound do we still want to move this forward potentially? or is this DOA given the above security constraints/considerations?
Unless someone spends the time making Patch Up work for forked PRs I don't think landing this is worth it unfortunately
Since electron/patch-conflict-fixer looks to be creating a temp branch and then deleting it, probably not much hope that approach would work with forks. Will close this, but hopefully something changes in the future that makes it viable.
@dsanders11 patch-conflict-fixer isn't Patch Up 🙃 Confusing but they're two different things
Confusing but they're two different things
@MarshallOfSound oh, that did indeed confuse me. Does PatchUp have a repo somewhere?
Nope, it's literally a single JS file in e/e
https://github.com/electron/electron/blob/main/script/push-patch.js