From 73fbf264ca99b1a80c9e29e21048451716b630e7 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 31 Aug 2018 02:27:49 +0100 Subject: [PATCH] Allow "dirty" local repositories to be safely modified This patch set rolls back previously introduced behavior in https://review.openstack.org/#/c/584482/ which forbids users from basically performing any Pegleg command that references a dirty local repository. This is annoying, forcing users to create temporary commits before executing a Pegleg command. Fortunately with https://review.openstack.org/#/c/577886/ Pegleg copies over all repositories to temporary folders, within which dirty repos can have their changes temporarily committed, allowing different references to then be safely checked out, without ever modifying any local repositories. Change-Id: I2142ae434f8ad57d0ab81cb104e21d952dc23148 --- doc/source/exceptions.rst | 4 +- src/bin/pegleg/pegleg/engine/exceptions.py | 14 ++- src/bin/pegleg/pegleg/engine/util/git.py | 34 +++--- .../pegleg/tests/unit/engine/util/test_git.py | 101 ++++++++++-------- 4 files changed, 76 insertions(+), 77 deletions(-) diff --git a/doc/source/exceptions.rst b/doc/source/exceptions.rst index c79dc063..e79e2b57 100644 --- a/doc/source/exceptions.rst +++ b/doc/source/exceptions.rst @@ -45,8 +45,8 @@ Git Exceptions :members: :show-inheritance: :undoc-members: - * - GitDirtyRepoException - - .. autoexception:: pegleg.engine.exceptions.GitDirtyRepoException + * - GitConfigException + - .. autoexception:: pegleg.engine.exceptions.GitConfigException :members: :show-inheritance: :undoc-members: diff --git a/src/bin/pegleg/pegleg/engine/exceptions.py b/src/bin/pegleg/pegleg/engine/exceptions.py index 7c65a2a5..808aa4e2 100644 --- a/src/bin/pegleg/pegleg/engine/exceptions.py +++ b/src/bin/pegleg/pegleg/engine/exceptions.py @@ -51,7 +51,7 @@ class GitAuthException(BaseGitException): self._ssh_key_path = ssh_key_path self._message = ('Failed to authenticate for repo %s with ssh-key at ' - 'path %s.' % (self._repo_url, self._ssh_key_path)) + 'path %s' % (self._repo_url, self._ssh_key_path)) super(GitAuthException, self).__init__(self._message) @@ -62,7 +62,7 @@ class GitProxyException(BaseGitException): def __init__(self, location): self._location = location - self._message = ('Could not resolve proxy [%s].' % self._location) + self._message = ('Could not resolve proxy [%s]' % self._location) super(GitProxyException, self).__init__(self._message) @@ -73,14 +73,12 @@ class GitSSHException(BaseGitException): def __init__(self, ssh_key_path): self._ssh_key_path = ssh_key_path - self._message = ('Failed to find specified SSH key: %s.' % + self._message = ('Failed to find specified SSH key: %s' % (self._ssh_key_path)) super(GitSSHException, self).__init__(self._message) -class GitDirtyRepoException(BaseGitException): - """Exception that occurs when trying to checkout ref from dirty repo.""" - message = ("Failed to checkout ref=%(ref)s from repo_url=%(repo_url)s. " - "Please manually clean all tracked/untracked files from repo " - "before proceeding.") +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") diff --git a/src/bin/pegleg/pegleg/engine/util/git.py b/src/bin/pegleg/pegleg/engine/util/git.py index 344a190b..b80a9f94 100644 --- a/src/bin/pegleg/pegleg/engine/util/git.py +++ b/src/bin/pegleg/pegleg/engine/util/git.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import atexit import logging import os import tempfile @@ -27,11 +26,7 @@ from pegleg.engine import exceptions LOG = logging.getLogger(__name__) -__all__ = [ - 'git_handler', -] - -__MODIFIED_REPOS = [] +__all__ = ('git_handler', ) def git_handler(repo_url, ref=None, proxy_server=None, auth_key=None): @@ -98,11 +93,15 @@ def git_handler(repo_url, ref=None, proxy_server=None, auth_key=None): repo = Repo(repo_url, search_parent_directories=True) if repo.is_dirty(untracked_files=True): - LOG.error('The locally cloned repo_url=%s is dirty. Manual clean ' - 'up of tracked/untracked files required.', repo_url) - # Raise an exception and force the user to clean up the repo. - # This is the safest approach to avoid data loss/corruption. - raise exceptions.GitDirtyRepoException(ref=ref, repo_url=repo_url) + # NOTE(felipemonteiro): This code should never be executed on a + # real local repository. Wrapper logic around this module will + # only perform this functionality against a temporary replica of + # local repositories, making the below operations safe. + LOG.info('Replica of local repo=%s is dirty. Committing all ' + 'tracked/untracked changes to ref=%s', + repo_name(repo_url), ref) + repo.git.add(all=True) + repo.index.commit('Temporary Pegleg commit') try: # Check whether the ref exists locally. @@ -365,8 +364,8 @@ def repo_name(repo_url_or_path): if config_reader.has_section(section): repo_url = config_reader.get_value(section, option) return repo_url.split('/')[-1].split('.git')[0] - raise click.ClickException( - "Repo=%s is not a valid Git repository" % repo_url_or_path) + + raise exceptions.GitConfigException(repo_url=repo_url_or_path) def normalize_repo_path(repo_path): @@ -403,12 +402,3 @@ def normalize_repo_path(repo_path): "repository" % orig_repo_path) return repo_path, sub_path - - -@atexit.register -def clean_repo(): - global __MODIFIED_REPOS - - for r in __MODIFIED_REPOS: - repo = Repo(r) - repo.head.reset(index=True, working_tree=True) diff --git a/src/bin/pegleg/tests/unit/engine/util/test_git.py b/src/bin/pegleg/tests/unit/engine/util/test_git.py index 80559017..d4b46f05 100644 --- a/src/bin/pegleg/tests/unit/engine/util/test_git.py +++ b/src/bin/pegleg/tests/unit/engine/util/test_git.py @@ -19,6 +19,7 @@ import requests import tempfile import fixtures +from git import Repo import mock import pytest @@ -337,6 +338,61 @@ def test_git_checkout_none_ref_checks_out_master(mock_log): _validate_git_clone(git_dir, 'master') +@pytest.mark.skipif( + not is_connected(), reason='git clone requires network connectivity.') +@mock.patch.object(git, 'LOG', autospec=True) +def test_git_checkout_dirty_repo_tracked_file_committed(mock_log): + """Validate a dirty tracked file is committed.""" + # Clone the openstack-helm repo and automatically checkout patch 73. + ref = 'refs/changes/54/457754/73' + repo_url = 'https://github.com/openstack/openstack-helm' + git_dir = git.git_handler(repo_url, ref) + _validate_git_clone(git_dir, ref) + + # Simulate a dirty repo by removing the first file in it we encounter. + dirty_file = next( + os.path.join(git_dir, x) for x in os.listdir(git_dir) + if os.path.isfile(os.path.join(git_dir, x))) + os.remove(dirty_file) + + # Sanity check that the repo has a dirty tracked file. + repo = Repo(git_dir) + assert repo.is_dirty() + + git.git_handler(git_dir, ref) + + # Validate that the tracked file is committed. + repo = Repo(git_dir) + assert not repo.is_dirty() + + +@pytest.mark.skipif( + not is_connected(), reason='git clone requires network connectivity.') +@mock.patch.object(git, 'LOG', autospec=True) +def test_git_checkout_dirty_repo_untracked_file_committed(mock_log): + """Validate a dirty untracked file is committed.""" + # Clone the openstack-helm repo and automatically checkout patch 73. + ref = 'refs/changes/54/457754/73' + repo_url = 'https://github.com/openstack/openstack-helm' + git_dir = git.git_handler(repo_url, ref) + _validate_git_clone(git_dir, ref) + + # Simulate a dirty repo by adding in a new untracked file. + with open(os.path.join(git_dir, test_utils.rand_name('test_file')), + 'w') as f: + f.write('whatever') + + # Sanity check that the repo has an untracked file. + repo = Repo(git_dir) + assert len(repo.untracked_files) + + git.git_handler(git_dir, ref) + + # Validate that the untracked file is committed. + assert not len(repo.untracked_files) + assert not repo.is_dirty(untracked_files=True) + + @pytest.mark.skipif( not is_connected(), reason='git clone requires network connectivity.') @mock.patch.object(git, 'LOG', autospec=True) @@ -357,51 +413,6 @@ def test_git_clone_existing_directory_raises_exc_for_invalid_ref(mock_log): _assert_check_out_from_local_repo(mock_log, git_dir) -@pytest.mark.skipif( - not is_connected(), reason='git clone requires network connectivity.') -@mock.patch.object(git, 'LOG', autospec=True) -def test_git_checkout_dirty_repo_tracked_file_raises_exception(mock_log): - """Validate Git throws an error when attempting to checkout a ref from - a dirty repo, with modified tracked file. - """ - # Clone the openstack-helm repo and automatically checkout patch 73. - ref = 'refs/changes/54/457754/73' - repo_url = 'https://github.com/openstack/openstack-helm' - git_dir = git.git_handler(repo_url, ref) - _validate_git_clone(git_dir, ref) - - # Simulate a dirty repo by removing the first file in it we encounter. - dirty_file = next( - os.path.join(git_dir, x) for x in os.listdir(git_dir) - if os.path.isfile(os.path.join(git_dir, x))) - os.remove(dirty_file) - - with pytest.raises(exceptions.GitDirtyRepoException): - git.git_handler(git_dir, ref) - - -@pytest.mark.skipif( - not is_connected(), reason='git clone requires network connectivity.') -@mock.patch.object(git, 'LOG', autospec=True) -def test_git_checkout_dirty_repo_untracked_file_raises_exception(mock_log): - """Validate Git throws an error when attempting to checkout a ref from - a dirty repo, with untracked file. - """ - # Clone the openstack-helm repo and automatically checkout patch 73. - ref = 'refs/changes/54/457754/73' - repo_url = 'https://github.com/openstack/openstack-helm' - git_dir = git.git_handler(repo_url, ref) - _validate_git_clone(git_dir, ref) - - # Simulate a dirty repo by adding in a new untracked file. - with open(os.path.join(git_dir, test_utils.rand_name('test_file')), - 'w') as f: - f.write('whatever') - - with pytest.raises(exceptions.GitDirtyRepoException): - git.git_handler(git_dir, ref) - - @pytest.mark.skipif( not is_connected(), reason='git clone requires network connectivity.') def test_git_clone_empty_url_raises_value_error():