Bump gitpython from 3.1.18 to 3.1.29 (#272)

* Bump gitpython from 3.1.18 to 3.1.29 Bumps [gitpython](https://github.com/gitpython-developers/GitPython) from 3.1.18 to 3.1.29. - [Release notes](https://github.com/gitpython-developers/GitPython/releases) - [Changelog](https://github.com/gitpython-developers/GitPython/blob/main/CHANGES) - [Commits](gitpython-developers/GitPython@3.1.18...3.1.29) --- updated-dependencies: - dependency-name: gitpython dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Handle git.Repo.working_dir being None with gitpython 3.1.29 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Josh <[email protected]> Co-authored-by: Josh Woodward <[email protected]>
wayfair-incubator · Oct 17, 2022 · 834b476 · 834b476
1 parent d45e98f
commit 834b476
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.16.1] - 2022-10-16
+
+### Added
+
+* Define new exception `PyGitOpsWorkingDirError` extending from `PyGitOpsError`.
+  * Exception raised in cases where `git.Repo.working_dir` is None (see below).
+
+### Changed
+
+* Adjust repo working_dir path operations to comply with `gitpython` type hint changes.
+  * Between `gitpython==3.1.18` -> `3.1.29`, `git.Repo.working_dir` was typed as `Optional`.
+  * Since many `os` and `pathlib` operations rely on `Repo.working_dir` and expect `Union[str, os.PathLike]`, code touching `repo.working_dir` was updated to comply with this difference. 
+
 ## [0.16.0] - 2022-07-27
 
 ### Added

diff --git a/pygitops/_util.py b/pygitops/_util.py
@@ -1,14 +1,15 @@
 import logging
 import os
 from contextlib import contextmanager
+from os import PathLike
 from pathlib import Path
-from typing import Iterator
+from typing import Iterator, Union
 
 from filelock import FileLock, Timeout
 from git import PushInfo, Repo
 from git.exc import InvalidGitRepositoryError
 
-from pygitops.exceptions import PyGitOpsError
+from pygitops.exceptions import PyGitOpsError, PyGitOpsWorkingDirError
 
 _logger = logging.getLogger(__name__)
 _lockfile_path = Path("lockfiles")
@@ -24,7 +25,7 @@ def lock_repo(repo: Repo) -> Iterator[None]:
     :param repo: The repo to lock on.
     """
 
-    repo_name = os.path.basename(os.path.normpath(repo.working_dir))
+    repo_name = os.path.basename(os.path.normpath(repo_working_dir(repo)))
     lockfile_name = str(get_lockfile_path(repo_name))
 
     lock = FileLock(lockfile_name)
@@ -123,3 +124,25 @@ def is_git_repo(path: Path) -> bool:
         return True
     except InvalidGitRepositoryError:
         return False
+
+
+def repo_working_dir(repo: Repo) -> Union[str, PathLike]:
+    """
+    Between gitpython 3.1.18 and 3.1.29, `git.Repo.working_dir` is now typed as Optional.
+
+    The `os.path` and `pathlib.Path` operations are not typed to support `Optional`.
+    Calling this function to access this property will handle cases where `repo.working_dir` is None, although it should never happen in practice.
+
+    :param repo: The repo whose `working_dir` we are interested in.
+    :raises PyGitOpsWorkingDirError: Raise error if `working_dir` is unexpectedly None.
+    :return str: `working_dir` of the repo
+    """
+    working_dir = repo.working_dir
+
+    # Can replace with assignment operator when python 3.7 support is dropped
+    if working_dir is None:
+        raise PyGitOpsWorkingDirError(
+            f"The working_dir for repo {repo} is unexpectedly None"
+        )
+
+    return working_dir
diff --git a/pygitops/exceptions.py b/pygitops/exceptions.py
@@ -8,3 +8,7 @@ class PyGitOpsValueError(PyGitOpsError):
 
 class PyGitOpsStagedItemsError(PyGitOpsError):
     """There were no items to stage for commit."""
+
+
+class PyGitOpsWorkingDirError(PyGitOpsError):
+    """There was an error with the filesystem, namely `git.Repo.working_dir` is unexpectedly None."""
diff --git a/pygitops/operations.py b/pygitops/operations.py
@@ -12,6 +12,7 @@
 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._util import repo_working_dir as _repo_working_dir
 from pygitops.exceptions import PyGitOpsError, PyGitOpsStagedItemsError
 from pygitops.remote_git_utils import _scrub_github_auth
 from pygitops.types import PathOrStr
@@ -43,7 +44,7 @@ def stage_commit_push_changes(
     :raises PyGitOpsError: There was an error staging, committing, or pushing code.
     """
     index = repo.index
-    workdir_path = Path(repo.working_dir)
+    workdir_path = Path(_repo_working_dir(repo))
 
     # We will determine items_to_stage if the parameter was not provided.
     if items_to_stage is None:

diff --git a/requirements.txt b/requirements.txt
@@ -1,2 +1,2 @@
 filelock~=3.4
-GitPython==3.1.18
+GitPython==3.1.29
diff --git a/setup.cfg b/setup.cfg
@@ -3,7 +3,7 @@ name = pygitops
 url = https://github.com/wayfair-incubator/pygitops
 author = Josh Woodward
 author_email = [email protected]
-version = 0.16.0
+version = 0.16.1
 description = Wrapper for low-level git logic. Useful for systems automating git operations.
 long_description = file: README.md
 long_description_content_type = text/markdown

diff --git a/tests/test_operations.py b/tests/test_operations.py
@@ -7,7 +7,7 @@
 from git import Actor, GitCommandError, GitError, Repo
 
 from pygitops._constants import GIT_BRANCH_MAIN, GIT_BRANCH_MASTER
-from pygitops._util import checkout_pull_branch
+from pygitops._util import checkout_pull_branch, repo_working_dir
 from pygitops.exceptions import PyGitOpsError, PyGitOpsStagedItemsError
 from pygitops.operations import (
     feature_branch,
@@ -68,7 +68,7 @@ def test_stage_commit_push_changes__push_failure__raises_pygitops_error(
 
     with feature_branch(cloned_repo, SOME_FEATURE_BRANCH):
         # Write to remote from the clone, then pull from local
-        test_file_path = Path(cloned_repo.working_dir) / SOME_CONTENT_FILENAME
+        test_file_path = Path(repo_working_dir(cloned_repo)) / SOME_CONTENT_FILENAME
         test_file_path.write_text(SOME_INITIAL_CONTENT)
 
         with pytest.raises(PyGitOpsError, match=SOME_FEATURE_BRANCH):
@@ -78,17 +78,17 @@ def test_stage_commit_push_changes__push_failure__raises_pygitops_error(
 
 
 def _delete_existing_file(local_repo: Repo, filename: str) -> None:
-    test_second_file_path = Path(local_repo.working_dir) / filename
+    test_second_file_path = Path(repo_working_dir(local_repo)) / filename
     test_second_file_path.unlink()
 
 
 def _modify_existing_file(local_repo: Repo, filename: str, content: str) -> None:
-    test_file_path = Path(local_repo.working_dir) / filename
+    test_file_path = Path(repo_working_dir(local_repo)) / filename
     test_file_path.write_text(content)
 
 
 def _add_new_file(local_repo: Repo, filename: str) -> None:
-    test_other_file_path = Path(local_repo.working_dir) / filename
+    test_other_file_path = Path(repo_working_dir(local_repo)) / filename
     test_other_file_path.touch()
 
 
@@ -188,7 +188,7 @@ def test_stage_commit_push_changes__add_new_file__change_persisted(tmp_path):
 
     with feature_branch(local_repo, SOME_FEATURE_BRANCH):
 
-        test_file_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+        test_file_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
         test_file_path.write_text(SOME_INITIAL_CONTENT)
 
         # act, asserting expected state before and after operation
@@ -217,7 +217,7 @@ def test_stage_commit_push_changes__remove_old_file__change_persisted(tmp_path):
     cloned_repo = repos.cloned_repo
 
     with feature_branch(cloned_repo, SOME_FEATURE_BRANCH):
-        test_file_path = Path(cloned_repo.working_dir) / SOME_CONTENT_FILENAME
+        test_file_path = Path(repo_working_dir(cloned_repo)) / SOME_CONTENT_FILENAME
         test_file_path.write_text(SOME_INITIAL_CONTENT)
 
         stage_commit_push_changes(
@@ -231,7 +231,7 @@ def test_stage_commit_push_changes__remove_old_file__change_persisted(tmp_path):
         with _assert_branch_state_stage_commit_push_changes__file_removed(
             remote_repo, local_repo
         ):
-            removal_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+            removal_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
             removal_path.unlink()
 
             # Commit the removal
@@ -260,9 +260,9 @@ def test_stage_commit_push_changes__with_staged_files__adds_only_requested_file(
 
     with feature_branch(local_repo, SOME_FEATURE_BRANCH):
 
-        test_file_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+        test_file_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
         test_file_path.write_text(SOME_INITIAL_CONTENT)
-        other_file_path = Path(local_repo.working_dir) / other_file
+        other_file_path = Path(repo_working_dir(local_repo)) / other_file
         other_file_path.write_text("other test content")
 
         # act, asserting expected state before and after operation
@@ -300,7 +300,7 @@ def test_stage_commit_push_changes__force_push_flag__changes_pushed(tmp_path):
     repos = _initialize_multiple_empty_repos(tmp_path)
     local_repo = repos.local_repo
     with feature_branch(local_repo, SOME_FEATURE_BRANCH):
-        test_file_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+        test_file_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
         test_file_path.write_text("content one")
         stage_commit_push_changes(
             local_repo, SOME_FEATURE_BRANCH, SOME_ACTOR, SOME_COMMIT_MESSAGE
@@ -807,7 +807,7 @@ def _assert_get_updated_repo_state(repo):
 def _assert_branch_state_stage_commit_push_changes__file_added(remote_repo, local_repo):
     """Run assertions before and after staging, committing, and pushing for a given repo pair."""
 
-    local_content_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+    local_content_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
 
     # The file should exist only on local before pushing
     assert SOME_INITIAL_CONTENT in local_content_path.read_text()
@@ -826,7 +826,7 @@ def _assert_branch_state_stage_commit_push_changes__file_removed(
     remote_repo, local_repo
 ):
     """Run assertions before and after staging, committing, and pushing for a given repo pair."""
-    local_content_path = Path(local_repo.working_dir) / SOME_CONTENT_FILENAME
+    local_content_path = Path(repo_working_dir(local_repo)) / SOME_CONTENT_FILENAME
 
     # The file should exist only on local before pushing
     assert SOME_INITIAL_CONTENT in local_content_path.read_text()
@@ -872,8 +872,8 @@ def _initialize_multiple_empty_repos(
 
     # Simplify things by assuring local copies are cloned from the remote.
     # Because they are clones, they will have 'origin' correctly configured
-    local_repo = Repo.clone_from(remote_repo.working_dir, local_path)
-    cloned_repo = Repo.clone_from(remote_repo.working_dir, clone_path)
+    local_repo = Repo.clone_from(repo_working_dir(remote_repo), local_path)
+    cloned_repo = Repo.clone_from(repo_working_dir(remote_repo), clone_path)
 
     return MultipleTestingRepos(
         remote_repo=remote_repo, local_repo=local_repo, cloned_repo=cloned_repo
@@ -889,7 +889,7 @@ def _get_diff(repo: Repo) -> str:
     """
 
     index = repo.index
-    workdir_path = Path(repo.working_dir)
+    workdir_path = Path(repo_working_dir(repo))
 
     untracked_file_paths = [Path(f) for f in repo.untracked_files]
     items_to_stage = untracked_file_paths + [Path(f.a_path) for f in index.diff(None)]

diff --git a/tests/test_util.py b/tests/test_util.py
@@ -13,8 +13,9 @@
     is_git_repo,
     lock_repo,
     push_error_present,
+    repo_working_dir,
 )
-from pygitops.exceptions import PyGitOpsError
+from pygitops.exceptions import PyGitOpsError, PyGitOpsWorkingDirError
 
 SOME_REPO_NAME = "some-repo-name"
 
@@ -193,3 +194,19 @@ def test_is_git_repo__not_a_git_repo__returns_false(tmp_path):
 def test_is_git_repo__is_a_git_repo__returns_true(tmp_path):
     Repo.init(tmp_path)
     assert is_git_repo(tmp_path)
+
+
+def test_repo_working_dir__working_dir_none__raises_pygitops_working_dir_error(mocker):
+
+    repo = mocker.Mock(working_dir=None)
+    with pytest.raises(PyGitOpsWorkingDirError):
+        repo_working_dir(repo)
+
+
+def test_repo_working_dir__working_dir_present__expected_working_dir_returned(tmp_path):
+
+    repo_path = tmp_path / SOME_REPO_NAME
+    repo_path.mkdir()
+    repo = Repo.init(repo_path)
+
+    assert repo_working_dir(repo) == str(repo_path)