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

feat: support forks in e cherry-pick #345

Closed
wants to merge 1 commit into from

Conversation

dsanders11
Copy link
Member

Closes #318.

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the actual code, but amusingly I don't actually think we want this to work.

A large part of e cherry-pick is the follow up patch conflict fix that only works for maintainers (not forks).

E.g. electron/electron#33244

@dsanders11
Copy link
Member Author

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

@MarshallOfSound
Copy link
Member

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.

@dsanders11
Copy link
Member Author

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.

@MarshallOfSound
Copy link
Member

@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 🤷

@MarshallOfSound
Copy link
Member

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
Copy link
Member Author

@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?

@codebytere
Copy link
Member

@MarshallOfSound do we still want to move this forward potentially? or is this DOA given the above security constraints/considerations?

@MarshallOfSound
Copy link
Member

Unless someone spends the time making Patch Up work for forked PRs I don't think landing this is worth it unfortunately

@dsanders11
Copy link
Member Author

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 dsanders11 closed this Jan 31, 2023
@MarshallOfSound
Copy link
Member

@dsanders11 patch-conflict-fixer isn't Patch Up 🙃 Confusing but they're two different things

@dsanders11
Copy link
Member Author

Confusing but they're two different things

@MarshallOfSound oh, that did indeed confuse me. Does PatchUp have a repo somewhere?

@MarshallOfSound
Copy link
Member

Nope, it's literally a single JS file in e/e

https://github.com/electron/electron/blob/main/script/push-patch.js

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.

Add support for forks to cherry-pick command
3 participants