From 0066427084683e1f5252a7a3d08f275d0055d6bf Mon Sep 17 00:00:00 2001 From: Simon Westphahl Date: Wed, 7 Dec 2022 13:54:49 +0100 Subject: [PATCH] Correctly set the repo remote URL I8e1b5b26f03cb75727d2b2e3c9310214a3eac447 introduced a regression that prevented us from re-cloning a repo that no longer exists on the file system (e.g. deleted by an operator) but where we still have the cached `Repo` object. The problem was that we only updated the remote URL of the repo object after we wrote it to the Git config. Unfortunately, if the repo no longer existed on the file system we would attempt to re-clone it with a possibly outdated remote URL. `test_set_remote_url` is a regression test for the issue described above. `test_set_remote_url_invalid` verifies that the original issue is fixes, where we updated the remote URL attribute of the repo object, but fail to update the Git config. Change-Id: I311842ccc7af38664c28177450ea9e80e1371638 --- tests/unit/test_merger_repo.py | 55 ++++++++++++++++++++++++++++++++++ zuul/merger/merger.py | 15 ++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_merger_repo.py b/tests/unit/test_merger_repo.py index fd78726ab3..7806db347d 100644 --- a/tests/unit/test_merger_repo.py +++ b/tests/unit/test_merger_repo.py @@ -16,6 +16,8 @@ import datetime import logging import os +import shutil +from unittest import mock import git import testtools @@ -678,6 +680,59 @@ class TestMergerRepo(ZuulTestCase): new_result = work_repo_underlying.commit('testtag') self.assertEqual(new_commit, new_result) + def test_set_remote_url_clone(self): + """Test that we always use the new Git URL for cloning. + + This is a regression test to make sure we always use the new + Git URL when a clone of the repo is necessary before updating + the config. + """ + parent_path = os.path.join(self.upstream_root, 'org/project1') + work_repo = Repo(parent_path, self.workspace_root, + 'none@example.org', 'User Name', '0', '0') + + # Simulate an invalid/outdated remote URL with the repo no + # longer existing on the file system. + work_repo.remote_url = "file:///dev/null" + shutil.rmtree(work_repo.local_path) + + # Setting a valid remote URL should update the attribute and + # clone the repository. + work_repo.setRemoteUrl(parent_path) + self.assertEqual(work_repo.remote_url, parent_path) + self.assertTrue(os.path.exists(work_repo.local_path)) + + def test_set_remote_url_invalid(self): + """Test that we don't store the Git URL when failing to set it. + + This is a regression test to make sure we will always update + the Git URL after a previously failed attempt. + """ + parent_path = os.path.join(self.upstream_root, 'org/project1') + work_repo = Repo(parent_path, self.workspace_root, + 'none@example.org', 'User Name', '0', '0') + + # Set the Git remote URL to an invalid value. + invalid_url = "file:///dev/null" + repo = work_repo.createRepoObject(None) + work_repo._git_set_remote_url(repo, invalid_url) + work_repo.remote_url = invalid_url + + # Simulate a failed attempt to update the remote URL + with mock.patch.object(work_repo, "_git_set_remote_url", + side_effect=RuntimeError): + with testtools.ExpectedException(RuntimeError): + work_repo.setRemoteUrl(parent_path) + + # Make sure we cleared out the remote URL. + self.assertIsNone(work_repo.remote_url) + + # Setting a valid remote URL should update the attribute and + # clone the repository. + work_repo.setRemoteUrl(parent_path) + self.assertEqual(work_repo.remote_url, parent_path) + self.assertTrue(os.path.exists(work_repo.local_path)) + class TestMergerWithAuthUrl(ZuulTestCase): config_file = 'zuul-github-driver.conf' diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py index d400c253bb..1e2de27b8d 100644 --- a/zuul/merger/merger.py +++ b/zuul/merger/merger.py @@ -757,8 +757,19 @@ class Repo(object): return log = get_annotated_logger(self.log, zuul_event_id) log.debug("Set remote url to %s", redact_url(url)) - self._git_set_remote_url(self.createRepoObject(zuul_event_id), url) - self.remote_url = url + try: + # Update the remote URL as it is used for the clone if the + # repo doesn't exist. + self.remote_url = url + self._git_set_remote_url( + self.createRepoObject(zuul_event_id), self.remote_url) + except Exception: + # Clear out the stored remote URL so we will always set + # the Git URL after a failed attempt. This prevents us from + # using outdated credentials that might still be stored in + # the Git config as part of the URL. + self.remote_url = None + raise def mapLine(self, commit, filename, lineno, zuul_event_id=None): repo = self.createRepoObject(zuul_event_id)