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

Support using spr from a fork #110

Conversation

joneshf
Copy link
Contributor

@joneshf joneshf commented Jul 10, 2022

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.

Created using spr 1.3.4-beta.1
@joneshf
Copy link
Contributor Author

joneshf commented Jul 10, 2022

Whoops. Wasn't supposed to make a new PR. Ignore this.

@joneshf joneshf closed this Jul 10, 2022
@joneshf joneshf deleted the spr/joneshf/support-using-spr-from-a-fork-1 branch July 10, 2022 20:51
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.

1 participant