spacedentist / spr

Submit pull requests for individual, amendable, rebaseable commits to GitHub
https://getcord.github.io/spr/
MIT License
377 stars 33 forks source link

Support using `spr` from a fork #106

Closed joneshf closed 1 year ago

joneshf commented 2 years ago

We make it possible for outside collaborators to use spr.

The vast majority of the changes here are in adding some tests around the changes in src/config.rs. Aside from that, this is a moderately-sized change "lines of code"-wise. The main thing here is that we're taking in optional values for all of the upstream owner, upstream repo, upstream remote, and upstream trunk. Even though they're all optional, it's unclear what it would mean to only have one of the four and use it. E.g. If we only have upstream owner, and we didn't check the others existed, we might do something like push a branch to origin and try to create a PR against an upstream that didn't exist. Also, even though the origin and upstream trunks should likely be the same, we still take it in just to be certain.

To make most things easier, the owner, repo, and trunk don't need to quantify whether it's for origin or upstream outside of Config. All the external cases where those values are used, we want to use the upstream if it exists and fallback to the origin otherwise. Internally, we do make a distinction just because there are things we do specifically on the origin no matter what.

There are two things we seem to have to make an active decision on: the git remote name, and the pull request head.

For the remote name, it seems we always want to push to origin. And, it seems like we always want to use upstream for landing/updating if it exists and fallback to origin otherwise.

For the pull request head, it seems we need to construct some special syntax if there's an upstream repo. It needs to mention the origin repo, so it ends up looking like <origin>:<branch> instead of just <branch>.

All of these changes together mean that we should be able to use spr from a fork. Importantly, it means that spr should now be able to be used by outside collaborators to contribute to spr itself. The only part that's maybe a bit unclear is whether the stacked workflow will actually work.

The API documentation seems to mention that the base parameter has to exist in the current repository: https://docs.github.com/en/rest/pulls/pulls#create-a-pull-request--parameters. Given the way stacking works with spr, it's unclear whether this will work or not. We're optimistically assuming it will work. But we might have to make some changes after the fact to support this flow.

Finally, we update the documentation to mention that there's a way to use spr from a fork and mention the configuration values in the configuration reference documentation.

Test Plan: There's two ways to view this change as working: recognize that I'm making this change from a fork, and the fact that I can submit it is the proof that it's working; or follow these steps yourself:

  1. Create a fork of a repo that you do not have access to. You can use: https://github.com/joneshf/spr-issue-80 if you need a repo.
  2. Setup the configuration as mentioned in the new docs/user/fork.md file.
  3. Follow the simple workflow in docs/user/simple.md to make a simple change and submit it to the upstream repo.
joneshf commented 2 years ago

Oh hey, it semi-worked!

This PR is some part of the implementation for https://github.com/getcord/spr/issues/80. There's actually a couple of issues with this implementation that make it so I don't feel confident saying this completely closes the issue:

All that said, I'm also aware this is a fairly large PR (though a good chunk of it is tests). If you think this should be broken up, lemme know and I'll do that. I wasn't really sure if you wanted to review a bunch of smaller PRs without getting a sense of how it was going to work altogether.

joneshf commented 2 years ago
  • Namely, the way stacking works with spr is that there's a merge branch the stacked PR is based on. I think the issue will be that the POST /repos/{owner}/{repo}/pulls endpoint wants the base branch to be a branch that exists in the upstream repo. If someone doesn't have push access, they can't really do this. I can't really verify this since I haven't gotten past that first error, and also don't fully understand the jargon GitHub uses in their documentation. But, I have an inkling this is going to be an issue.

Just tried to open a stacked PR by using spr diff atop of this PR (actually just used spr diff --all after this PR was created, but it should be effectively the same). It seems to fail in this way. It's hard to tell since GitHub doesn't seem to give the best of responses, but this was the error printed out:

bb097c4 Extract GitHub remote logic to its own struct
  🛑  GitHub: Validation Failed
      Documentation URL: https://docs.github.com/rest/reference/pulls#create-a-pull-request
      Errors:
      - {"code":"invalid","field":"base","resource":"PullRequest"}

The branches it created were:

So it looks like it's doing what is expected. It's just that this workflow doesn't work with how GitHub allows you to create PRs from a fork. Not a huge deal, but it means that stacking from a fork doesn't really work until a different approach is figured out. I'll update the documentation in this PR to not mention the stacking workflow.

joneshf commented 1 year ago

I think this PR is just too big (and disorganized) to review. I'm going to split it up into smaller chunks. Hopefully that'll be easier on you all.