From 5369efeec1917636479dd4a9fdba66e6869596cb Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 20 Jul 2018 14:30:31 -0400 Subject: [PATCH] git: Raise exception on ref checkout from dirty repo This raises an exception on trying to checkout a ref from a dirty repo in the git_handler module. The parent patch https://review.openstack.org/#/c/582652/ currently forcibly cleans the repo but this is undesirable as it may have local user changes that need to be resolved first. The safest path is for Pegleg to immediately raise an exception on any tracked/untracked files that are detected using the GitPython API. Unit tests are added for both untracked/tracked file cases. Change-Id: I2241bc981dca1999508c3c7e95948fe47a5ddebf --- doc/source/exceptions.rst | 5 ++ src/bin/pegleg/pegleg/engine/exceptions.py | 7 ++ src/bin/pegleg/pegleg/engine/util/git.py | 11 +-- .../pegleg/tests/unit/engine/util/test_git.py | 69 +++++++++++++------ 4 files changed, 65 insertions(+), 27 deletions(-) diff --git a/doc/source/exceptions.rst b/doc/source/exceptions.rst index afa83f0c..c79dc063 100644 --- a/doc/source/exceptions.rst +++ b/doc/source/exceptions.rst @@ -45,6 +45,11 @@ Git Exceptions :members: :show-inheritance: :undoc-members: + * - GitDirtyRepoException + - .. autoexception:: pegleg.engine.exceptions.GitDirtyRepoException + :members: + :show-inheritance: + :undoc-members: * - GitException - .. autoexception:: pegleg.engine.exceptions.GitException :members: diff --git a/src/bin/pegleg/pegleg/engine/exceptions.py b/src/bin/pegleg/pegleg/engine/exceptions.py index 2729f205..7c65a2a5 100644 --- a/src/bin/pegleg/pegleg/engine/exceptions.py +++ b/src/bin/pegleg/pegleg/engine/exceptions.py @@ -77,3 +77,10 @@ class GitSSHException(BaseGitException): (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.") diff --git a/src/bin/pegleg/pegleg/engine/util/git.py b/src/bin/pegleg/pegleg/engine/util/git.py index 64d63781..4b6c3176 100644 --- a/src/bin/pegleg/pegleg/engine/util/git.py +++ b/src/bin/pegleg/pegleg/engine/util/git.py @@ -93,11 +93,12 @@ def git_handler(repo_url, ref, proxy_server=None, auth_key=None): raise NotADirectoryError(msg) repo = Repo(repo_url) - if repo.is_dirty(): - LOG.warning('The locally cloned repo_url=%s is dirty. ' - 'Cleaning up untracked files.', repo_url) - # Reset the index and working tree to match current ref. - repo.head.reset(index=True, working_tree=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) try: # Check whether the ref exists locally. 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 19c4f53c..8e848d58 100644 --- a/src/bin/pegleg/tests/unit/engine/util/test_git.py +++ b/src/bin/pegleg/tests/unit/engine/util/test_git.py @@ -313,28 +313,6 @@ def test_git_clone_delete_repo_and_reclone(mock_log, clean_git_repo): mock_log.debug.assert_any_call('Cloning [%s]', repo_url) -@pytest.mark.skipif( - not is_connected(), reason='git clone requires network connectivity.') -@mock.patch.object(git, 'LOG', autospec=True) -def test_git_clone_clean_dirty_local_repo(mock_log, clean_git_repo): - """Validate that a dirty repo is cleaned before a ref is checked out.""" - 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) - - file_to_rename = os.path.join(git_dir, os.listdir(git_dir)[0]) - os.rename(file_to_rename, file_to_rename + '-renamed') - - git_dir = git.git_handler(git_dir, ref) - _validate_git_clone(git_dir, ref) - - assert mock_log.warning.called - mock_log.warning.assert_any_call( - 'The locally cloned repo_url=%s is dirty. Cleaning up untracked ' - 'files.', git_dir) - - @pytest.mark.skipif( not is_connected(), reason='git clone requires network connectivity.') @mock.patch.object(git, 'LOG', autospec=True) @@ -356,6 +334,53 @@ def test_git_clone_existing_directory_raises_exc_for_invalid_ref( _assert_repo_url_was_cloned(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, clean_git_repo): + """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, clean_git_repo): + """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(clean_git_repo):