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

Adds the ability to clone only and not fork #62

Merged
merged 3 commits into from
Aug 10, 2021
Merged

Conversation

sledigabel
Copy link
Contributor

@sledigabel sledigabel commented Jul 12, 2021

This is a follow up of #49.

With this commit, the clone command is defaulting to cloning and
forking only when the user has not push rights onto the repository.

If the --fork flag is specified, it will fork and clone
systematically.

With this commit, the `clone` command is defaulting to cloning and
forking only when the user has not push rights onto the repository.

If the `--fork` flag is specified, it will fork and clone
systematically.
@sledigabel
Copy link
Contributor Author

Just noticed there'll be a bug here:
https://github.com/Skyscanner/turbolift/blob/main/cmd/create_prs/create_prs.go#L77

In the case of a gh fork the fork will be called origin and the upstream will be called upstream.
In the case of a gh clone with a fork, it will be called fork and origin.

After testing, I believe this line can be removed.
This is taken care of by the gh command automatically, it will test if the user has write access.
Testing...

@sledigabel
Copy link
Contributor Author

We have a problem...

the gh client is quite smart (too smart) with its own logic.
the clone is virtually the same as a git clone but the gh pr create is quite advanced.

https://github.com/cli/cli/blob/trunk/pkg/cmd/pr/create/create.go#L532-L590

Basically the logic is:

  • get the pr title body etc.
  • check if the remote is compatible with push
  • check if the repo is pushable (the magic is right there
  • if you can't write to it, then the CLI proposes to create a fork using the API from a TUI.

Unfortunately that TUI is not programmable so bad luck.
I'd suggest two courses of actions:
A. look at the API layout and replicate the same mechanism but at clone time rather than PR time.
B. update the gh cli and add some more options to the gh pr create and avoid this TUI thing altogether to make it more interfaceable.

At this stage, and having looked at this for a little while now, Option A is prob the easiest one.
I actually think it's worth the effort, as it would make it quite a smooth experience.

One downside of using the TUI and the current gh behaviour is that the result forked repo created is named "forked", and the main one is still called "origin". So then it's on the user to basically handle this and push to origin or fork when needed.
Option A would simplify this too, all the pushable repos would end up being called "origin" (after the fork, you end up with a upstream/origin paradigm).

@sledigabel sledigabel marked this pull request as draft July 12, 2021 18:46
@sledigabel
Copy link
Contributor Author

sledigabel commented Jul 25, 2021

Bottom line of this is that it's a lot more complicated than previously anticipated.

After discussing with Richard (@rnorth) a while back, the best course of action for now is to:

  • Systematically fork
  • Propose an alternative to always clone with a flag.

It adds some value, as most users will either have full access to the target repos or no access.
If the users encounters a case where they have a mix of both, forking still works.

We can spend some more time refining the logic behind when using clone and forking. For now the gh client logic is not enough as we don't know ahead of time which one will be needed, and programmatically create the fork.

Adding a `--no-fork` flag and leave the default behaviour as "always
fork". Changing the clone unit test to better represent the two
scenarii.
@sledigabel sledigabel marked this pull request as ready for review July 25, 2021 21:55
@sledigabel sledigabel changed the title Clone does not fork unless specified Adds the ability to clone only and not fork Jul 26, 2021
Copy link

@JimNero009 JimNero009 left a comment

Choose a reason for hiding this comment

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

LGTM.
Adds the ability to unilaterally choose to not fork. Implementation for a conditional 'fork only if needed' turned out to be much more complicated than originally thought, so this provides a solution for the subset of cases where a user has full access to all repos they wish to change, which is valuable.

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.

None yet

2 participants