From 330ba9b9c4b0222a19d7f964e4124176cdea2a6c Mon Sep 17 00:00:00 2001 From: Ozan Unsal Date: Wed, 19 Jan 2022 17:06:32 +0100 Subject: [PATCH] Do not clone the same repository multiple times, re-use already cloned repository Clone the directory to the compose tmp directory Update the test_scm in order to create real Compose object. Mock objects are not allowed to create/delete files for preventing multiple clones JIRA: RHELCMP-5250 Signed-off-by: Ozan Unsal --- pungi/wrappers/scm.py | 69 ++++++++++++++++++++++++++++++++++++------- tests/test_scm.py | 23 +++++++++++---- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/pungi/wrappers/scm.py b/pungi/wrappers/scm.py index 5c4b37fb..6d79d86f 100644 --- a/pungi/wrappers/scm.py +++ b/pungi/wrappers/scm.py @@ -19,7 +19,9 @@ from __future__ import absolute_import import os import shutil import glob +import threading import six +import tempfile from six.moves import shlex_quote from six.moves.urllib.request import urlretrieve from fnmatch import fnmatch @@ -29,6 +31,8 @@ from kobo.shortcuts import run, force_list from pungi.util import explode_rpm_package, makedirs, copy_all, temp_dir, retry from .kojiwrapper import KojiWrapper +scm_lock = threading.Lock() + class ScmBase(kobo.log.LoggingBase): def __init__(self, logger=None, command=None, compose=None): @@ -372,10 +376,33 @@ def get_file_from_scm(scm_dict, target_path, compose=None): scm = _get_wrapper(scm_type, logger=logger, command=command, compose=compose) files_copied = [] - for i in force_list(scm_file): - with temp_dir(prefix="scm_checkout_") as tmp_dir: - scm.export_file(scm_repo, i, scm_branch=scm_branch, target_dir=tmp_dir) - files_copied += copy_all(tmp_dir, target_path) + branch = scm_branch if scm_branch else "master" + delete_after_flag = False + with scm_lock: + if compose and scm_repo: + repo = scm_repo.rsplit("/")[-1] + tmp_dir = compose.paths.work.tmp_dir() + tmp_dir = os.path.join(tmp_dir, repo, branch) + else: + tmp_dir = tempfile.mkdtemp(prefix="scm_checkout_") + delete_after_flag = True + + if not os.path.isdir(tmp_dir): + makedirs(tmp_dir) + + for i in force_list(scm_file): + # Check the files which are included with subdirectories + check_file = os.path.join(tmp_dir, i[i.rfind("/") + 1 :]) + if ( + type(scm_dict) is not dict + or command is not None + or not compose + or not os.path.isfile(check_file) + ): + scm.export_file(scm_repo, i, scm_branch=scm_branch, target_dir=tmp_dir) + files_copied += copy_all(tmp_dir, target_path) + if delete_after_flag: + shutil.rmtree(tmp_dir) return files_copied @@ -459,14 +486,34 @@ def get_dir_from_scm(scm_dict, target_path, compose=None): logger = compose._logger if compose else None scm = _get_wrapper(scm_type, logger=logger, command=command, compose=compose) + branch = scm_branch if scm_branch else "master" + delete_after_flag = False - with temp_dir(prefix="scm_checkout_") as tmp_dir: - scm.export_dir(scm_repo, scm_dir, scm_branch=scm_branch, target_dir=tmp_dir) + with scm_lock: + if compose and scm_repo: + repo = scm_repo.rsplit("/")[-1] + tmp_dir = compose.paths.work.tmp_dir() + tmp_dir = os.path.join(tmp_dir, repo, branch) + else: + tmp_dir = tempfile.mkdtemp(prefix="scm_checkout_") + delete_after_flag = True + + if not os.path.isdir(tmp_dir): + makedirs(tmp_dir) + scm.export_dir(scm_repo, scm_dir, scm_branch=scm_branch, target_dir=tmp_dir) + elif ( + type(scm_dict) is not dict + or command is not None + or not scm_repo + or not compose + ): + scm.export_dir(scm_repo, scm_dir, scm_branch=scm_branch, target_dir=tmp_dir) files_copied = copy_all(tmp_dir, target_path) - # Make sure the directory has permissions set to 755. This is a workaround - # for a problem where sometimes the directory will be 700 and it will not - # be accessible via httpd. - os.chmod(target_path, 0o755) - + # Make sure the directory has permissions set to 755. This is a workaround + # for a problem where sometimes the directory will be 700 and it will not + # be accessible via httpd. + os.chmod(target_path, 0o755) + if delete_after_flag: + shutil.rmtree(tmp_dir) return files_copied diff --git a/tests/test_scm.py b/tests/test_scm.py index f6307967..e45c32f4 100644 --- a/tests/test_scm.py +++ b/tests/test_scm.py @@ -16,14 +16,17 @@ import six from pungi.wrappers import scm from tests.helpers import touch from kobo.shortcuts import run +from pungi.compose import Compose class SCMBaseTest(unittest.TestCase): def setUp(self): self.destdir = tempfile.mkdtemp() + self.tmp_dir = tempfile.mkdtemp() def tearDown(self): shutil.rmtree(self.destdir) + shutil.rmtree(self.tmp_dir) def assertStructure(self, returned, expected): # Check we returned the correct files @@ -143,7 +146,8 @@ class GitSCMTestCase(SCMBaseTest): @mock.patch("pungi.wrappers.scm.run") def test_get_file_function(self, run): - compose = mock.Mock(conf={}) + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({}, self.tmp_dir) def process(cmd, workdir=None, **kwargs): touch(os.path.join(workdir, "some_file.txt")) @@ -338,6 +342,8 @@ class GitSCMTestCaseReal(SCMBaseTest): shutil.rmtree(self.gitRepositoryLocation) def test_get_file_function(self): + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({}, self.tmp_dir) sourceFileLocation = random.choice(list(self.files.keys())) sourceFilename = os.path.basename(sourceFileLocation) destinationFileLocation = os.path.join(self.destdir, "other_file.txt") @@ -348,7 +354,7 @@ class GitSCMTestCaseReal(SCMBaseTest): "file": sourceFilename, }, os.path.join(self.destdir, destinationFileLocation), - compose=self.compose, + compose=compose, ) self.assertEqual(destinationFileActualLocation, destinationFileLocation) self.assertTrue(os.path.isfile(destinationFileActualLocation)) @@ -361,6 +367,8 @@ class GitSCMTestCaseReal(SCMBaseTest): self.assertEqual(sourceFileContent, destinationFileContent) def test_get_file_function_with_overwrite(self): + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({}, self.tmp_dir) sourceFileLocation = random.choice(list(self.files.keys())) sourceFilename = os.path.basename(sourceFileLocation) destinationFileLocation = os.path.join(self.destdir, "other_file.txt") @@ -375,7 +383,7 @@ class GitSCMTestCaseReal(SCMBaseTest): "file": sourceFilename, }, os.path.join(self.destdir, destinationFileLocation), - compose=self.compose, + compose=compose, overwrite=True, ) self.assertEqual(destinationFileActualLocation, destinationFileLocation) @@ -603,7 +611,8 @@ class KojiSCMTestCase(SCMBaseTest): @mock.patch("pungi.wrappers.scm.KojiWrapper") def test_doesnt_get_dirs(self, KW, dl): - compose = mock.Mock(conf={"koji_profile": "koji"}) + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({"koji_profile": "koji"}, self.tmp_dir) with self.assertRaises(RuntimeError) as ctx: scm.get_dir_from_scm( @@ -628,7 +637,8 @@ class KojiSCMTestCase(SCMBaseTest): @mock.patch("pungi.wrappers.scm.KojiWrapper") def test_get_from_build(self, KW, dl): - compose = mock.Mock(conf={"koji_profile": "koji"}) + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({"koji_profile": "koji"}, self.tmp_dir) def download(src, dst): touch(dst) @@ -659,7 +669,8 @@ class KojiSCMTestCase(SCMBaseTest): @mock.patch("pungi.wrappers.scm.KojiWrapper") def test_get_from_latest_build(self, KW, dl): - compose = mock.Mock(conf={"koji_profile": "koji"}) + with mock.patch("pungi.compose.ComposeInfo"): + compose = Compose({"koji_profile": "koji"}, self.tmp_dir) def download(src, dst): touch(dst)