From 9f4412098f44e341cd6734b83ba23f10576d524e Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 27 2020 11:07:33 +0000 Subject: [PATCH 1/3] Simplify interface of get_distgit_files Funtion get_distgit_files is responsible for fetching files from a remote repository. Therefor, it requres a repository URL as an argument rather than accepting relative argumnets and redirecting to get_distgit_url to construct the URL by itself. This change is also easier for testing get_distgit_files. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 853cbf9..b38f834 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -37,7 +37,7 @@ import http.client import concurrent.futures from freshmaker import log, conf from freshmaker.kojiservice import koji_service -from freshmaker.utils import sorted_by_nvr, get_distgit_files +from freshmaker.utils import sorted_by_nvr, get_distgit_files, get_distgit_url import koji @@ -300,9 +300,10 @@ class ContainerImage(dict): name = repository try: + repo_url = get_distgit_url(namespace, name, ssh=False) files = get_distgit_files( - namespace, name, commit, ["content_sets.yml", "container.yaml"], - ssh=False, logger=log) + repo_url, commit, ["content_sets.yml", "container.yaml"], + logger=log) except OSError as e: self.log_error("Error while fetching dist-git repo files: %s" % e) return data diff --git a/freshmaker/utils.py b/freshmaker/utils.py index 8aeae0a..a45ee36 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -173,18 +173,19 @@ def retry(timeout=conf.net_timeout, interval=conf.net_retry_interval, wait_on=Ex return wrapper -def get_distgit_url(namespace, name, ssh, user): +def get_distgit_url(namespace, name, ssh=True, user=None): """ Returns the dist-git repository URL. :param str namespace: Namespace in which the repository is located, for example "rpms", "containers", "modules", ... :param str name: Name of the repository inside the namespace. - :param bool ssh: If True, SSH auth will be used when fetching the files. + :param bool ssh: indicate whether SSH auth will be used when fetching the + files. Default is True. :param str user: If set, overrides the default user for SSH auth. - - :rtype: str + Otherwise, username specified in config ``git_user`` will be used. :return: The dist-git repository URL. + :rtype: str """ if ssh: if user is None: @@ -201,9 +202,7 @@ def get_distgit_url(namespace, name, ssh, user): @retry(logger=log) -def get_distgit_files( - namespace, name, commit_or_branch, files, ssh=True, user=None, - logger=None): +def get_distgit_files(repo_url, commit_or_branch, files, logger=None): """ Fetches the `files` from dist-git repository defined by `namespace`, `name` and `commit_or_branch` and returns them. @@ -212,21 +211,14 @@ def get_distgit_files( preferred method to get the files from dist-git in case the full clone of repository is not needed. - :param str namespace: Namespace in which the repository is located, for - example "rpms", "containers", "modules", ... - :param str name: Name of the repository inside the namespace. + :param str repo_url: the repository URL. :param str commit_or_branch: Commit hash or branch name. - :param list files: List of strings defining the files to fetch. - :param bool ssh: If True, SSH auth will be used when fetching the files. - :param str user: If set, overrides the default user for SSH auth. + :param list[str] files: List of files to fetch. :param freshmaker.log logger: Logger instance. - - :rtype: dict :return: Dictionary with file name as key and file content as value. If the file does not exist in a dist-git repo, None is used as value. + :rtype: dict[str, str or None] """ - repo_url = get_distgit_url(namespace, name, ssh, user) - # Use the "git archive" to get the files in tarball and then extract # them and return in dict. We need to go file by file, because the # "git archive" would fail completely in case any file does not exist diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 5839547..d4219db 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -34,7 +34,7 @@ from freshmaker.lightblue import ContainerRepository from freshmaker.lightblue import LightBlue from freshmaker.lightblue import LightBlueRequestError from freshmaker.lightblue import LightBlueSystemError -from freshmaker.utils import sorted_by_nvr +from freshmaker.utils import sorted_by_nvr, get_distgit_url from freshmaker import log from tests import helpers @@ -168,9 +168,10 @@ class TestGetAdditionalDataFromDistGit(helpers.FreshmakerTestCase): "rpms/foo-docker", "branch", "commit") self.assertEqual(ret["generate_pulp_repos"], False) + repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) self.get_distgit_files.assert_called_once_with( - 'rpms', 'foo-docker', "commit", - ["content_sets.yml", "container.yaml"], logger=log, ssh=False) + repo_url, "commit", + ["content_sets.yml", "container.yaml"], logger=log) def test_generate_os_error(self): self.get_distgit_files.side_effect = OSError( @@ -187,9 +188,10 @@ class TestGetAdditionalDataFromDistGit(helpers.FreshmakerTestCase): "Error while fetching dist-git repo files: Got an error (128) from git: " "fatal: reference is not a tree: 4d42e2009cec70d871c65de821396cd750d523f1") + repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) self.get_distgit_files.assert_called_once_with( - 'rpms', 'foo-docker', 'commit', - ["content_sets.yml", "container.yaml"], logger=log, ssh=False) + repo_url, 'commit', + ["content_sets.yml", "container.yaml"], logger=log) def test_generate_no_namespace(self): self.get_distgit_files.return_value = { @@ -202,9 +204,10 @@ class TestGetAdditionalDataFromDistGit(helpers.FreshmakerTestCase): "foo-docker", "branch", "commit") self.assertEqual(ret["generate_pulp_repos"], False) + repo_url = get_distgit_url('rpms', 'foo-docker', ssh=False) self.get_distgit_files.assert_called_once_with( - 'rpms', 'foo-docker', "commit", - ["content_sets.yml", "container.yaml"], logger=log, ssh=False) + repo_url, "commit", + ["content_sets.yml", "container.yaml"], logger=log) def test_generate_no_pulp_repos(self): self.get_distgit_files.return_value = { From 427bbd81e869cdbda3a3c8650433fdbe8d06ae8d Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 27 2020 11:07:33 +0000 Subject: [PATCH 2/3] Write tests for get_distgit_files This also fixes that the function returns files content as str rather than bytes. Set 1 to NET_TIMEOUT for tests so that functions decorated with retry run faster so that tests run faster as a result. Signed-off-by: Chenxiong Qi --- diff --git a/conf/config.py b/conf/config.py index 931e907..c6002bd 100644 --- a/conf/config.py +++ b/conf/config.py @@ -243,7 +243,7 @@ class TestConfiguration(BaseConfiguration): MESSAGING_SENDER = 'in_memory' # Global network-related values, in seconds - NET_TIMEOUT = 3 + NET_TIMEOUT = 1 NET_RETRY_INTERVAL = 1 KOJI_CONTAINER_SCRATCH_BUILD = True diff --git a/freshmaker/utils.py b/freshmaker/utils.py index a45ee36..394dc64 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -232,7 +232,7 @@ def get_distgit_files(repo_url, commit_or_branch, files, logger=None): tar_bytes = io.BytesIO(tar_data.encode()) tar = tarfile.open(fileobj=tar_bytes) for member in tar.getmembers(): - ret[member.name] = tar.extractfile(member).read() + ret[member.name] = tar.extractfile(member).read().decode() except OSError as e: if "path not found" in str(e): ret[os.path.basename(f)] = None diff --git a/tests/test_utils.py b/tests/test_utils.py index b052863..4c43133 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -20,13 +20,16 @@ # # Written by Jan Kaluza +import shutil +import tempfile + from unittest.mock import patch import pytest from freshmaker import conf from freshmaker.models import ArtifactType -from freshmaker.utils import get_rebuilt_nvr, sorted_by_nvr +from freshmaker.utils import get_rebuilt_nvr, sorted_by_nvr, get_distgit_files from tests import helpers @@ -71,3 +74,53 @@ class TestSortedByNVR(helpers.FreshmakerTestCase): expected = ["bar-1-2", "foo-1-1", "foo-1-10"] ret = sorted_by_nvr(lst, reverse=True) self.assertEqual(ret, list(reversed(expected))) + + +class TestGetDistGitFiles(object): + """Test get_distgit_files""" + + @classmethod + def setup_class(cls): + import os + import subprocess + cls.repo_dir = tempfile.mkdtemp() + with open(os.path.join(cls.repo_dir, 'a.txt'), 'w') as f: + f.write('hello') + with open(os.path.join(cls.repo_dir, 'b.txt'), 'w') as f: + f.write('world') + git_cmds = [ + ['git', 'init'], + ['git', 'add', 'a.txt', 'b.txt'], + ['git', 'config', 'user.name', 'tester'], + ['git', 'config', 'user.email', 'tester@localhost'], + ['git', 'commit', '-m', 'initial commit for test'], + ] + for cmd in git_cmds: + subprocess.check_call(cmd, cwd=cls.repo_dir) + + cls.repo_url = 'file://' + cls.repo_dir + + @classmethod + def teardown_class(cls): + shutil.rmtree(cls.repo_dir) + + @pytest.mark.parametrize('files,expected', [ + [['a.txt'], {'a.txt': 'hello'}], + [['a.txt', 'b.txt'], {'a.txt': 'hello', 'b.txt': 'world'}], + ]) + def test_get_files(self, files, expected): + result = get_distgit_files(self.repo_url, 'master', files) + assert expected == result + + @patch('freshmaker.utils._run_command') + def test_error_path_not_found(self, run_command): + run_command.side_effect = OSError('path not found') + result = get_distgit_files(self.repo_url, 'master', ['a.txt']) + assert {'a.txt': None} == result + + @patch('freshmaker.utils._run_command') + @patch('time.sleep') + def test_unhandled_error_occurs(self, sleep, run_command): + run_command.side_effect = ValueError + with pytest.raises(ValueError): + get_distgit_files(self.repo_url, 'master', ['a.txt']) From c96c619273c20c12d61cfa0ea6ff52f0e1f02398 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 27 2020 11:07:33 +0000 Subject: [PATCH 3/3] Ensure file descriptors are closed in get_distgit_files Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/utils.py b/freshmaker/utils.py index 394dc64..6a658cf 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -229,10 +229,11 @@ def get_distgit_files(repo_url, commit_or_branch, files, logger=None): cmd = ['git', 'archive', '--remote=%s' % repo_url, commit_or_branch, f] tar_data = _run_command(cmd, logger=logger, log_output=False) - tar_bytes = io.BytesIO(tar_data.encode()) - tar = tarfile.open(fileobj=tar_bytes) - for member in tar.getmembers(): - ret[member.name] = tar.extractfile(member).read().decode() + with io.BytesIO(tar_data.encode()) as tar_bytes: + with tarfile.open(fileobj=tar_bytes) as tar: + for member in tar.getmembers(): + with tar.extractfile(member) as fd: + ret[member.name] = fd.read().decode() except OSError as e: if "path not found" in str(e): ret[os.path.basename(f)] = None