From 2e51779d57f5def8c4ba79522e08a84e9eeb2d3a Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 10 Oct 2018 23:55:32 -0400 Subject: [PATCH] refactor: Exchange NotADirectoryError for better exception This patch set replaces raising NotADirectoryError after trying to parse a repository for its root path (normalize_repo_path in pegleg.engine.util.git) with a better exception (exceptions.GitInvalidRepoException). It is better because a folder can still not be a repo, so raising the first exception isn't apropos. Next, this patch set changes where the exception is raised -- which is in normalize_repo_path itself, which is more appropriate as the function is used in many places and so there should be intrinsic error handling so as to avoid having to wrap it every time. Change-Id: I918d8c293f1140eb80c83499dba2c23af232b79e --- doc/source/exceptions.rst | 5 +++++ pegleg/engine/exceptions.py | 7 ++++++- pegleg/engine/util/git.py | 21 ++++++++------------- tests/unit/engine/test_site_repository.py | 5 +++-- tests/unit/engine/util/test_git.py | 8 +++++--- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/doc/source/exceptions.rst b/doc/source/exceptions.rst index 75d3bcd0..f23ef1d2 100644 --- a/doc/source/exceptions.rst +++ b/doc/source/exceptions.rst @@ -56,3 +56,8 @@ Git Exceptions :members: :show-inheritance: :undoc-members: + +.. autoexception:: pegleg.engine.exceptions.GitInvalidRepoException + :members: + :show-inheritance: + :undoc-members: diff --git a/pegleg/engine/exceptions.py b/pegleg/engine/exceptions.py index 808aa4e2..ae5adaf2 100644 --- a/pegleg/engine/exceptions.py +++ b/pegleg/engine/exceptions.py @@ -81,4 +81,9 @@ class GitSSHException(BaseGitException): class GitConfigException(BaseGitException): """Exception that occurs when reading Git repo config fails.""" - message = ("Failed to read Git config file for repo_url=%(repo_url)s") + message = ("Failed to read Git config file for repo path: %(repo_path)s") + + +class GitInvalidRepoException(BaseGitException): + """Exception raised when an invalid repository is detected.""" + message = ("The repository path or URL is invalid: %(repo_path)s") diff --git a/pegleg/engine/util/git.py b/pegleg/engine/util/git.py index f6b0d26b..d2e1feb1 100644 --- a/pegleg/engine/util/git.py +++ b/pegleg/engine/util/git.py @@ -17,7 +17,6 @@ import os import tempfile from urllib.parse import urlparse -import click from git import exc as git_exc from git import Git from git import Repo @@ -61,7 +60,6 @@ def git_handler(repo_url, path to ``repo_url``. :raises ValueError: If ``repo_url`` isn't a valid URL or doesn't begin with a valid protocol (http, https or ssh) for cloning. - :raises NotADirectoryError: If ``repo_url`` isn't a valid directory path. """ @@ -91,13 +89,9 @@ def git_handler(repo_url, else: LOG.debug('Treating repo_url=%s as an already-cloned repository. ' 'Attempting to checkout ref=%s', repo_url, ref) - try: - # get absolute path of what is probably a directory - repo_url, _ = normalize_repo_path(repo_url) - except Exception: - msg = "The repo_url=%s is not a valid Git repo" % repo_url - LOG.error(msg) - raise NotADirectoryError(msg) + + # Normalize the repo path. + repo_url, _ = normalize_repo_path(repo_url) repo = Repo(repo_url, search_parent_directories=True) if repo.is_dirty(untracked_files=True): @@ -434,7 +428,8 @@ def normalize_repo_path(repo_url_or_path): :param repo_url_or_path: URL of remote Git repo or path to local Git repo. :returns: Tuple of root Git path or URL, additional subpath included (e.g. "deployment_files") - :rtype: tuple + :rtype: tuple[str, str] + :raises GitInvalidRepoException: If the repo is invalid. """ @@ -459,8 +454,8 @@ def normalize_repo_path(repo_url_or_path): repo_url_or_path = os.path.abspath(repo_url_or_path) if not repo_url_or_path or not is_repository(repo_url_or_path): - raise click.ClickException( - "Specified site repo path=%s exists but is not a valid Git " - "repository" % orig_repo_path) + msg = "The repo_path=%s is not a valid Git repo" % (orig_repo_path) + LOG.error(msg) + raise exceptions.GitInvalidRepoException(repo_path=repo_url_or_path) return repo_url_or_path, sub_path diff --git a/tests/unit/engine/test_site_repository.py b/tests/unit/engine/test_site_repository.py index 7fb48ece..605f3a89 100644 --- a/tests/unit/engine/test_site_repository.py +++ b/tests/unit/engine/test_site_repository.py @@ -21,6 +21,7 @@ import click import pytest from pegleg import config +from pegleg.engine import exceptions from pegleg.engine import repository from pegleg.engine import util @@ -238,10 +239,10 @@ def test_process_repositories_with_local_site_path_exists_not_repo(*_): """Validate that when the site repo already exists but isn't a repository that an error is raised. """ - with pytest.raises(click.ClickException) as exc: + with pytest.raises(exceptions.GitInvalidRepoException) as exc: _test_process_repositories_inner( expected_extra_repos=TEST_REPOSITORIES) - assert "is not a valid Git repository" in str(exc.value) + assert "The repository path or URL is invalid" in str(exc.value) def test_process_repositories_with_repo_username(): diff --git a/tests/unit/engine/util/test_git.py b/tests/unit/engine/util/test_git.py index e732814f..e718fafe 100644 --- a/tests/unit/engine/util/test_git.py +++ b/tests/unit/engine/util/test_git.py @@ -400,9 +400,11 @@ def test_git_clone_invalid_url_type_raises_value_error(): @pytest.mark.skipif( not test_utils.is_connected(), reason='git clone requires network connectivity.') -def test_git_clone_invalid_local_repo_url_raises_notadirectory_error(): - url = False - with pytest.raises(NotADirectoryError): +@mock.patch.object(git, 'is_repository', autospec=True, return_value=False) +@mock.patch('os.path.exists', return_value=True, autospec=True) +def test_git_clone_invalid_local_repo_url_raises_invalid_repo_exc(*args): + url = 'blah' + with pytest.raises(exceptions.GitInvalidRepoException): git.git_handler(url, ref='master')