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):