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

Add git clone command #49

Merged
merged 3 commits into from
Jul 12, 2021
Merged

Conversation

Macavirus
Copy link
Contributor

Partially addresses #48

This makes no actual changes to any commands, but adds the ability to call Clone function.

As there is still some ambiguity around how we should address the actual turbolift clone command, I will address that in a followup PR.

@sledigabel @rnorth would you prefer to preserve current behaviour in clone.go to fork by default? I think not forking by default is more user-friendly in general, but it sounds like the Skyscanner use case requires bulk forking.

My suggestion would be to make the new Clone command default, and place ForkAndClone behind a different cobra command.

@sledigabel
Copy link
Contributor

Your suggestion makes a lot of sense and I don't have any objection for this to be the default behaviour.
On the other hand I don't think forkAndClone should be a separate command, since it's partially the behaviour of clone.
Perhaps a simple flag such as --fork to make the behaviour explicitly forking?

@rnorth
Copy link
Collaborator

rnorth commented May 16, 2021

Argh I should have put this comment here. But yes, I agree with @sledigabel!

Copy link
Collaborator

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

I think this looks fine to me if we’re just implementing the ‘back half’ of the feature. I’d suggest, though, doing the modifications to the clone cobra command as well.

@sledigabel
Copy link
Contributor

Agreed with @rnorth, let's add the whole thing into this PR rather than breaking it down into multiple PRs as we have agreed this will be the new default.

@sledigabel
Copy link
Contributor

@Macavirus how do you want to proceed? do you want to amend this PR and change the default behaviour of clone? I'm happy to add on to your PR if you're swamped, this feature would be a great addition.

@rnorth
Copy link
Collaborator

rnorth commented Jul 12, 2021

@sledigabel shall we discuss one of us picking this up some time?

@sledigabel
Copy link
Contributor

GMTA, I have a draft for this :-) publishing soon

@sledigabel
Copy link
Contributor

Merging this and will iterate from there.

Thanks a lot for your contribution @Macavirus !

@sledigabel sledigabel merged commit 78ab876 into Skyscanner:main Jul 12, 2021
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

3 participants