From 3e9e68476f25def55584ec5790342cbc89a8d1e0 Mon Sep 17 00:00:00 2001 From: Jack Pierce Date: Fri, 14 May 2021 12:20:59 -0400 Subject: [PATCH] Give control of repo clone location to user (#97) --- CHANGELOG.md | 6 ++- docs/index.md | 2 +- docs/updating-repo.md | 4 +- pygitops/_util.py | 15 ++++++++ pygitops/operations.py | 23 ++++++------ tests/test_operations.py | 80 ++++++++++++++++++++++------------------ tests/test_util.py | 12 +++++- 7 files changed, 90 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d03ada..9179a50 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/index.md b/docs/index.md index a422782..6fd8846 100644 --- a/docs/index.md +++ b/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 diff --git a/docs/updating-repo.md b/docs/updating-repo.md index f4f473b..e8ce60b 100644 --- a/docs/updating-repo.md +++ b/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 diff --git a/pygitops/_util.py b/pygitops/_util.py index 2f85781..845c5b1 100644 --- a/pygitops/_util.py +++ b/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 diff --git a/pygitops/operations.py b/pygitops/operations.py index 320b8e6..ef9ed7d 100755 --- a/pygitops/operations.py +++ b/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 diff --git a/tests/test_operations.py b/tests/test_operations.py index 60ee898..408b61f 100644 --- a/tests/test_operations.py +++ b/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,30 +360,28 @@ 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 ) @@ -393,8 +389,7 @@ def test_get_updated_repo__repo_exists_locally__repo_update_performed_against_de 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}, @@ -402,7 +397,7 @@ def test_get_updated_repo__repo_exists_locally__repo_update_performed_against_de ) 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,9 +465,11 @@ 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 @@ -470,7 +477,7 @@ def test_get_updated_repo__local_repo_on_disk__remote_default_branch_changes__re 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 diff --git a/tests/test_util.py b/tests/test_util.py index 5d6533d..7e5c119 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -4,12 +4,13 @@ import pytest from filelock import Timeout -from git import PushInfo +from git import PushInfo, Repo from pygitops._util import ( _lockfile_path, checkout_pull_branch, get_lockfile_path, + is_git_repo, lock_repo, push_error_present, ) @@ -147,3 +148,12 @@ def test_pull_branch__branch_not_in_refs__raises_pygitops_error(mocker): with pytest.raises(PyGitOpsError): checkout_pull_branch(repo, "test_branch") + + +def test_is_git_repo__not_a_git_repo__returns_false(tmp_path): + assert is_git_repo(tmp_path) is False + + +def test_is_git_repo__is_a_git_repo__returns_true(tmp_path): + Repo.init(tmp_path) + assert is_git_repo(tmp_path)