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!(clone): Use better cloning policy #148

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Conversation

sledigabel
Copy link
Contributor

@sledigabel sledigabel commented Oct 9, 2024

Description of the change

BREAKING CHANGE

  • Deprecation of the --no-fork flag in favour of --fork to force forking.
  • Default behaviour is branching if the user has permission to do so, otherwise turbolift will fork the repository.
  • Unit tests for GH mocks include the type of calls it makes in the argument list.

Context

The all-or-nothing approach of Turbolift around the clone policy (either forking or branching) is causing a lot of friction and requires the user to think about it.
gh goes around the problem by cloning normally but checking at push time whether the user has write permission on the target repository and prompt the user what to do.

We use a similar mechanism to decide at clone time what the clone policy should be, branching or forking.
The user still has the option to force the forking if they desire.

Change content

  • Introduced the new forking policy and changed tests around that.
  • Added a new unit test specifically for no write access
  • Refactored testing to have more clarity on what calls are made to GH
  • Changed documentation

Related to

Fixes #146.

Added a new function IsPushable to check if a user has push permissions
based on the viewerPermission API query.

Added new util file in the github package to include helper functions
and facilitate unit testing on command outputs.
- Deprecated the `nofork` flag in favour of a `fork` one
- Default Clone behaviour is branching instead of forking
- Modified tests to reflect the changes in the clone command.
Updated the README to clarify the usage of the `turbolift clone` command.
This commit fixes the logic for forking and cloning repositories.

Also changed the testing logic to include the type of call for GH.
This is because the testing was just on the argument and was missing
context when running the unit test.
@sledigabel sledigabel requested review from rnorth and Dan7-7-7 October 23, 2024 11:07
cmd/clone/clone.go Outdated Show resolved Hide resolved
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.

Provisional 👍 !

Updated the IsPushable function to use the full repository name instead
of the repository directory path.

The gh command now uses a repository as a parameter, since the clone has
not taken place yet.
Added a new test to verify behavior when permission check fails.

Updated the log message to correctly display the repository name when
determining push permissions.
@sledigabel sledigabel marked this pull request as ready for review October 24, 2024 15:01
@sledigabel sledigabel requested a review from rnorth October 25, 2024 08:18
Copy link
Collaborator

@Dan7-7-7 Dan7-7-7 left a comment

Choose a reason for hiding this comment

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

LGTM

rnorth
rnorth previously approved these changes Nov 4, 2024
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.

LGTM

Co-authored-by: Richard North <[email protected]>
@sledigabel sledigabel merged commit b4a9e55 into main Nov 4, 2024
5 checks passed
@sledigabel sledigabel deleted the use_gh_to_decide_forking branch November 4, 2024 15:10
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.

Be smarter about forking vs branching
3 participants