Give control of repo clone location to user (#97)

CHANGELOG.md

@@ -6,7 +6,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-This package does not yet have a release
+## [0.11.0] - 2021-05-14
+
+### Changed
+
+* Removed `repo_name` parameter of the `get_updated_repo` function to give the user control of exactly where to clone the repo contents to. The `clone_dir` argument is now the directory into which the contents of the repo will be cloned.
 
 ## [0.10.0] - 2021-05-07
 

docs/index.md

@@ -30,7 +30,7 @@ repo_name = 'pygitops'
 
 repo_url = build_github_repo_url(service_account_name, service_account_token, repo_owner, repo_name)
 
-repo = get_updated_repo(repo_url, '/repos', repo_name)
+repo = get_updated_repo(repo_url, f'/repos/{repo_name}')
 ```
 
 ### Using GitPython

docs/updating-repo.md

@@ -23,9 +23,9 @@ Below is an example which clones the [Columbo repository][columbo-repo] into the
 ```python
 from pygitops.operations import get_updated_repo
 
-repo = get_updated_repo('https://github.com/wayfair-incubator/columbo.git', '~/repos', 'columbo')
+repo = get_updated_repo('https://github.com/wayfair-incubator/columbo.git', '~/repos/columbo')
 ```
 
-Again, the value of this function is that you don't have to know if the Columbo repo has already been cloned. If you run `repo = get_updated_repo('https://github.com/wayfair-incubator/columbo.git', '~/repos', 'columbo')` again, it will not clone the repo again, but will only pull updates from the default branch.
+Again, the value of this function is that you don't have to know if the Columbo repo has already been cloned. If you run `repo = get_updated_repo('https://github.com/wayfair-incubator/columbo.git', '~/repos/columbo')` again, it will not clone the repo again, but will only pull updates from the default branch.
 
 [columbo-repo]: https://github.com/wayfair-incubator/columbo

pygitops/_util.py

@@ -6,6 +6,7 @@
 
 from filelock import FileLock, Timeout
 from git import PushInfo, Repo
+from git.exc import InvalidGitRepositoryError
 
 from pygitops.exceptions import PyGitOpsError
 
@@ -103,3 +104,17 @@ def push_error_present(push_info: PushInfo) -> bool:
     Check for presence of the error flag in the returned flags bitmask.
     """
     return bool(push_info.flags & push_info.ERROR)
+
+
+def is_git_repo(path: Path) -> bool:
+    """
+    Determine if a given path is a valid git repository.
+
+    :param path: Directory to inspect
+    :return: True if the contents of a directory at given path contains a valid git repository
+    """
+    try:
+        Repo(path).git_dir
+        return True
+    except InvalidGitRepositoryError:
+        return False

pygitops/operations.py

@@ -9,6 +9,7 @@
 
 from pygitops._util import checkout_pull_branch as _checkout_pull_branch
 from pygitops._util import get_lockfile_path as _get_lockfile_path
+from pygitops._util import is_git_repo as _is_git_repo
 from pygitops._util import lock_repo as _lock_repo
 from pygitops._util import push_error_present as _push_error_present
 from pygitops.exceptions import PyGitOpsError, PyGitOpsStagedItemsError
@@ -152,42 +153,42 @@ def feature_branch(repo: Repo, branch_name: str) -> Iterator[None]:
             )
 
 
-def get_updated_repo(
-    repo_url: str, clone_dir: PathOrStr, repo_name: str, **kwargs
-) -> Repo:
+def get_updated_repo(repo_url: str, clone_dir: PathOrStr, **kwargs) -> Repo:
     """
     Clone the default branch of the target repository, returning a repo object.
 
     If repo is already present, we will pull in the latest changes to the default branch of the repo.
 
     :param repo_url: URL of the Github repository to be cloned.
-    :param clone_dir: The root directory where repositories are cloned.
-    :param repo_name: The name of the repository to be cloned.
+    :param clone_dir: The empty directory to clone repository content to.
     :raises PyGitOpsError: There was an error cloning the repository.
     """
     # make sure it's actually a Path if our user passed a str
     clone_dir = Path(clone_dir)
 
-    dest_repo_path = clone_dir / repo_name
-    git_lockfile_path = _get_lockfile_path(repo_name)
+    # if clone dir does not exist, create it, and all parent dirs
+    if not clone_dir.exists():
+        clone_dir.mkdir(parents=True)
+
+    git_lockfile_path = _get_lockfile_path(str(clone_dir))
 
     # Lock the following operation such that only one process will attempt to clone the repo at a time.
     with FileLock(str(git_lockfile_path)):
         try:
             # if the repo already exists, don't clone it
-            if dest_repo_path.is_dir():
-                repo = Repo(dest_repo_path)
+            if _is_git_repo(clone_dir):
+                repo = Repo(clone_dir)
                 # pull down latest changes from `branch` if provided in kwargs, deferring to repo default branch
                 branch = kwargs.get("branch") or get_default_branch(repo)
                 _checkout_pull_branch(repo, branch)
                 return repo
 
-            return Repo.clone_from(repo_url, dest_repo_path, **kwargs)
+            return Repo.clone_from(repo_url, clone_dir, **kwargs)
         except GitError as e:
             clean_repo_url = _scrub_github_auth(repo_url)
             scrubbed_error_message = _scrub_github_auth(str(e))
             raise PyGitOpsError(
-                f"Error cloning repo {clean_repo_url} into destination path {dest_repo_path}: {scrubbed_error_message}"
+                f"Error cloning or updating repo {clean_repo_url} into destination path {clone_dir}: {scrubbed_error_message}"
             ) from e
 
 

tests/test_operations.py

@@ -68,7 +68,7 @@ def test_stage_commit_push_changes__push_failure__raises_pygitops_error(
             )
 
 
-def test_stage_commit_push_changes__add_new_file__change_persisted(mocker, tmp_path):
+def test_stage_commit_push_changes__add_new_file__change_persisted(tmp_path):
     """
     Configure 'local' and 'remote' repositories with initial content.
 
@@ -97,7 +97,7 @@ def test_stage_commit_push_changes__add_new_file__change_persisted(mocker, tmp_p
             remote_repo.heads[SOME_FEATURE_BRANCH].checkout()
 
 
-def test_stage_commit_push_changes__remove_old_file__change_persisted(mocker, tmp_path):
+def test_stage_commit_push_changes__remove_old_file__change_persisted(tmp_path):
     """
     Configure 'local' and 'remote' repositories with initial content.
 
@@ -178,9 +178,7 @@ def test_stage_commit_push_changes__with_staged_files__adds_only_requested_file(
         assert [other_file] == local_repo.untracked_files
 
 
-def test_stage_commit_push_changes__no_files_to_stage__raises_pygitops_error(
-    mocker, tmp_path
-):
+def test_stage_commit_push_changes__no_files_to_stage__raises_pygitops_error(tmp_path):
 
     repos = _initialize_multiple_empty_repos(tmp_path)
     local_repo = repos.local_repo
@@ -301,7 +299,7 @@ def test_feature_branch__conditional_branch_checkout(
     assert feature_branch_mock.checkout.called == checkout_expected
 
 
-def test_feature_branch__exception_within_context__cleanup_occurs(mocker, tmp_path):
+def test_feature_branch__exception_within_context__cleanup_occurs(mocker):
 
     some_branch_name = "some-feature-branch"
     origin_mock = mocker.Mock(refs=[GIT_BRANCH_MASTER])
@@ -327,7 +325,7 @@ def test_feature_branch__exception_within_context__cleanup_occurs(mocker, tmp_pa
     local_master_branch.checkout.assert_called_once()
 
 
-def test_feature_branch__nested_calls__raises_pygitops_error(mocker, tmp_path):
+def test_feature_branch__nested_calls__raises_pygitops_error(mocker):
     """Make sure the feature_branch context manager locks the repo correctly."""
 
     some_branch_name = "some-feature-branch"
@@ -362,47 +360,44 @@ def test_get_updated_repo__git_error_raised_by_repo__raises_pygitops_error(mocke
     mocker.patch("pygitops.operations.Repo.clone_from", side_effect=GitError)
 
     with pytest.raises(PyGitOpsError):
-        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH, SOME_REPO_NAME)
+        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH)
 
 
 def test_get_updated_repo__repo_dne__fresh_clone_performed(mocker, tmp_path):
 
     clone_from_mock = mocker.patch("pygitops.operations.Repo.clone_from")
-    expected_clone_path = tmp_path / SOME_REPO_NAME
 
-    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path, SOME_REPO_NAME)
+    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path)
 
-    clone_from_mock.assert_called_once_with(SOME_CLONE_REPO_URL, expected_clone_path)
+    clone_from_mock.assert_called_once_with(SOME_CLONE_REPO_URL, tmp_path)
 
 
 def test_get_updated_repo__repo_dne__kwargs_passed_to_clone_from(mocker, tmp_path):
 
     clone_from_mock = mocker.patch("pygitops.operations.Repo.clone_from")
-    expected_clone_path = tmp_path / SOME_REPO_NAME
 
     some_kwargs = {"some_arg": "some-value", "another_arg": "another-value"}
 
-    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path, SOME_REPO_NAME, **some_kwargs)
+    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path, **some_kwargs)
 
     clone_from_mock.assert_called_once_with(
-        SOME_CLONE_REPO_URL, expected_clone_path, **some_kwargs
+        SOME_CLONE_REPO_URL, tmp_path, **some_kwargs
     )
 
 
 def test_get_updated_repo__repo_exists_locally__repo_update_performed_against_default_branch(
     mocker, tmp_path
 ):
 
-    (tmp_path / SOME_REPO_NAME).mkdir()
-
+    Repo.init(tmp_path)
     master_branch_mock = mocker.Mock()
     repo_mock = mocker.Mock(
         heads={"master": master_branch_mock},
         git=mocker.Mock(symbolic_ref=mocker.Mock(return_value=SOME_HEAD_REF)),
     )
     mocker.patch("pygitops.operations.Repo", return_value=repo_mock)
 
-    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path, SOME_REPO_NAME)
+    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path)
 
     master_branch_mock.checkout.assert_called_once()
     repo_mock.remotes.origin.pull.assert_called_once()
@@ -412,23 +407,31 @@ def test_get_updated_repo__repo_exists_locally__repo_update_performed_against_pr
     mocker, tmp_path
 ):
 
-    (tmp_path / SOME_REPO_NAME).mkdir()
     repo_mock = mocker.Mock()
-
+    Repo.init(tmp_path)
     get_default_branch_mock = mocker.patch("pygitops.operations.get_default_branch")
     _checkout_pull_branch_mock = mocker.patch(
         "pygitops.operations._checkout_pull_branch"
     )
     mocker.patch("pygitops.operations.Repo", return_value=repo_mock)
 
-    get_updated_repo(
-        SOME_CLONE_REPO_URL, tmp_path, SOME_REPO_NAME, branch=SOME_FEATURE_BRANCH
-    )
+    get_updated_repo(SOME_CLONE_REPO_URL, tmp_path, branch=SOME_FEATURE_BRANCH)
 
     get_default_branch_mock.assert_not_called()
     _checkout_pull_branch_mock.assert_called_once_with(repo_mock, SOME_FEATURE_BRANCH)
 
 
+def test_get_updated_repo__clone_dirs_dne__clone_dirs_created(mocker, tmp_path):
+
+    clone_dir = tmp_path / "some_parent" / "repo"
+    repo_mock = mocker.Mock()
+    mocker.patch("pygitops.operations.Repo", return_value=repo_mock)
+
+    get_updated_repo(SOME_CLONE_REPO_URL, clone_dir)
+
+    assert clone_dir.exists()
+
+
 def test_get_updated_repo__file_operations__repo_not_present(tmp_path):
     """
     A fresh clone of a repository should pull down remote content
@@ -439,13 +442,15 @@ def test_get_updated_repo__file_operations__repo_not_present(tmp_path):
 
     # initialize remote repo
     remote_path = tmp_path / "remote"
+    local_path = tmp_path / "local"
+    local_path.mkdir()
     remote_repo = _initialize_repo_with_content(remote_path)
 
     # write and commit some content in the remote repo
     _commit_content(remote_repo, SOME_INITIAL_CONTENT)
 
     # clone remote repo to local
-    local_repo = get_updated_repo(str(remote_path), tmp_path, SOME_LOCAL_REPO_NAME)
+    local_repo = get_updated_repo(str(remote_path), local_path)
 
     filepath = f"{local_repo.working_tree_dir}/{SOME_CONTENT_FILENAME}"
 
@@ -460,17 +465,19 @@ def test_get_updated_repo__local_repo_on_disk__remote_default_branch_changes__re
 ):
 
     remote_path = tmp_path / "remote"
+    local_path = tmp_path / "local"
+    local_path.mkdir()
     remote_repo = _initialize_repo_with_content(remote_path)
 
-    local_repo = get_updated_repo(str(remote_path), tmp_path, SOME_LOCAL_REPO_NAME)
+    local_repo = get_updated_repo(str(remote_path), local_path)
 
     assert local_repo.active_branch.name != SOME_DEFAULT_BRANCH_NAME
 
     # create and update head ref on remote repo
     default_head = remote_repo.create_head(SOME_DEFAULT_BRANCH_NAME)
     remote_repo.head.set_reference(default_head)
 
-    local_repo = get_updated_repo(str(remote_path), tmp_path, SOME_LOCAL_REPO_NAME)
+    local_repo = get_updated_repo(str(remote_path), local_path)
     assert local_repo.active_branch.name == SOME_DEFAULT_BRANCH_NAME
 
 
@@ -486,20 +493,22 @@ def test_get_updated_repo__file_operations__repo_present_locally(tmp_path):
 
     # initialize remote repo
     remote_path = tmp_path / "remote"
+    local_path = tmp_path / "local"
+    local_path.mkdir()
     remote_repo = _initialize_repo_with_content(remote_path)
 
     # write and commit some content in the remote repo
     _commit_content(remote_repo, SOME_INITIAL_CONTENT)
 
     # setup the preconditions outlined in our test case description
-    local_repo = get_updated_repo(str(remote_path), tmp_path, SOME_LOCAL_REPO_NAME)
+    local_repo = get_updated_repo(str(remote_path), local_path)
     local_repo.create_head(SOME_FEATURE_BRANCH)
     local_repo.heads[SOME_FEATURE_BRANCH].checkout()
     _commit_content(remote_repo, SOME_NEW_CONTENT)
 
     # act, asserting expected state before and after operation
     with _assert_get_updated_repo_state(local_repo):
-        get_updated_repo(str(remote_path), tmp_path, SOME_LOCAL_REPO_NAME)
+        get_updated_repo(str(remote_path), local_path)
 
 
 def test_get_updated_repo__error__login_not_in_error(mocker):
@@ -510,7 +519,7 @@ def test_get_updated_repo__error__login_not_in_error(mocker):
     )
 
     with pytest.raises(PyGitOpsError) as exc_info:
-        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH, SOME_REPO_NAME)
+        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH)
 
     assert SOME_SERVICE_ACCOUNT_TOKEN not in str(exc_info.value)
 
@@ -522,23 +531,24 @@ def test_get_updated_repo__error__login_scrubbed(mocker):
     )
 
     with pytest.raises(PyGitOpsError) as exc_info:
-        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH, SOME_REPO_NAME)
+        get_updated_repo(SOME_CLONE_REPO_URL, SOME_CLONE_PATH)
 
     exception_text = str(exc_info.value)
     assert "https://***:***@" in exception_text
     assert SOME_SERVICE_ACCOUNT_TOKEN not in exception_text
 
 
-def test_get_updated_repo__clone_dir_as_str(mocker):
+def test_get_updated_repo__clone_dir_as_str(mocker, tmp_path):
 
     clone_from_mock = mocker.patch(
         "pygitops.operations.Repo.clone_from",
     )
 
-    get_updated_repo(SOME_CLONE_REPO_URL, "clone_dir", SOME_REPO_NAME)
+    mocker.patch("pygitops.operations._is_git_repo", mocker.Mock(return_value=False))
+    get_updated_repo(SOME_CLONE_REPO_URL, str(tmp_path))
 
     assert clone_from_mock.called
-    assert clone_from_mock.call_args[0][1] == PosixPath("clone_dir/some-repo-name")
+    assert clone_from_mock.call_args[0][1] == PosixPath(str(tmp_path))
 
 
 def test_get_default_branch__match_not_present__raises_pygitops_error(mocker):
@@ -572,7 +582,7 @@ def test_get_default_branch__match_index_error__raises_pygitops_error(mocker):
         get_default_branch(repo_mock)
 
 
-def test_get_default_branch__default_branch_returned(mocker, tmp_path):
+def test_get_default_branch__default_branch_returned(tmp_path):
 
     repos = _initialize_multiple_empty_repos(tmp_path)
     remote_repo = repos.remote_repo
@@ -582,9 +592,7 @@ def test_get_default_branch__default_branch_returned(mocker, tmp_path):
     assert get_default_branch(local_repo) == remote_repo.heads[0].name
 
 
-def test_get_default_branch__remote_head_changes__new_default_branch_returned(
-    mocker, tmp_path
-):
+def test_get_default_branch__remote_head_changes__new_default_branch_returned(tmp_path):
 
     repos = _initialize_multiple_empty_repos(tmp_path)
     remote_repo = repos.remote_repo