From d53bd2ee7360819d77ea33d2770f0532aac2b61b Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Fri, 10 Jul 2020 17:32:11 +0200 Subject: [PATCH] Use factory for requests session objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `make_session()` factory creates session objects with default values for timeouts, number of retries, etc., which can be overridden when calling it. Create such (long-lived) session objects in the constructors of the individual toddlers using requests to access remote APIs. Pull in module-global functions into the toddler class where necessary and adapt tests to accommodate. Also, mock out the factory function in the `toddler´ fixture for testing. Signed-off-by: Nils Philippsen --- tests/conftest.py | 14 +++ tests/plugins/test_flag_ci_pr.py | 14 +-- tests/plugins/test_flag_commit_build.py | 30 ++--- tests/plugins/test_pdc_retired_packages.py | 58 ++++------ toddlers/plugins/flag_ci_pr.py | 21 +--- toddlers/plugins/flag_commit_build.py | 20 +--- toddlers/plugins/packager_bugzilla_sync.py | 12 -- toddlers/plugins/pdc_retired_packages.py | 126 ++++++++++----------- toddlers/utils/requests.py | 31 +++++ 9 files changed, 162 insertions(+), 164 deletions(-) create mode 100644 toddlers/utils/requests.py diff --git a/tests/conftest.py b/tests/conftest.py index 037630e..ac52b2d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,17 +1,31 @@ +import sys +from unittest.mock import MagicMock + import pytest +from toddlers.utils.requests import make_session + @pytest.fixture def toddler(request, monkeypatch): """Fixture creating a toddler for a class testing it The test class must set the toddler class in its `toddler_cls` attribute. + The fixture will mock out the `make_session` function before creating the + object so that any request objects the toddler creates in its constructor + won't be real. """ test_cls = request.cls if not hasattr(test_cls, "toddler_cls"): raise RuntimeError(f"{test_cls} needs to set `toddler_cls`") toddler_cls = test_cls.toddler_cls + toddler_module = sys.modules[toddler_cls.__module__] + + for name in dir(toddler_module): + obj = getattr(toddler_module, name) + if obj is make_session: + monkeypatch.setattr(toddler_module, name, MagicMock()) toddler_obj = toddler_cls() return toddler_obj diff --git a/tests/plugins/test_flag_ci_pr.py b/tests/plugins/test_flag_ci_pr.py index c2474c6..768e6ae 100644 --- a/tests/plugins/test_flag_ci_pr.py +++ b/tests/plugins/test_flag_ci_pr.py @@ -1,5 +1,5 @@ import logging -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock import fedora_messaging.api import pytest @@ -85,11 +85,11 @@ class TestFlagCIPRToddler: "the PR id from it" ) - @patch("toddlers.plugins.flag_ci_pr.requests_session") - def test_process_request_failed(self, mock_requests, toddler, caplog): - mock_requests.request.return_value = MagicMock( + def test_process_request_failed(self, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=False, status_code=401, text="invalid" ) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 @@ -115,11 +115,11 @@ class TestFlagCIPRToddler: caplog.records[-1].message == "Request to https://pagure.io returned: 401" ) - @patch("toddlers.plugins.flag_ci_pr.requests_session") - def test_process_valid(self, mock_requests, toddler, caplog): - mock_requests.request.return_value = MagicMock( + def test_process_valid(self, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=True, status_code=200, text="invalid" ) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 diff --git a/tests/plugins/test_flag_commit_build.py b/tests/plugins/test_flag_commit_build.py index 5c7a779..2661196 100644 --- a/tests/plugins/test_flag_commit_build.py +++ b/tests/plugins/test_flag_commit_build.py @@ -114,12 +114,12 @@ class TestFlagCommitBuildToddler: assert toddler.process(config=config, message=msg) is None assert caplog.records[-1].message == "No # in the git_url: foobar" - @patch("toddlers.plugins.flag_commit_build.requests_session") @patch("toddlers.plugins.flag_commit_build.koji") - def test_process_flag_no_task_id(self, mock_koji, mock_requests, toddler, caplog): - mock_requests.request.return_value = MagicMock( + def test_process_flag_no_task_id(self, mock_koji, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=False, status_code=401, text="invalid" ) + client = Mock() client.getBuild = Mock( return_value={ @@ -132,6 +132,7 @@ class TestFlagCommitBuildToddler: } ) mock_koji.ClientSession = Mock(return_value=client) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 @@ -158,12 +159,12 @@ class TestFlagCommitBuildToddler: == "Request to https://src.fedoraproject.org returned: 401" ) - @patch("toddlers.plugins.flag_commit_build.requests_session") @patch("toddlers.plugins.flag_commit_build.koji") - def test_process_flag_failed(self, mock_koji, mock_requests, toddler, caplog): - mock_requests.request.return_value = MagicMock( + def test_process_flag_failed(self, mock_koji, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=False, status_code=401, text="invalid" ) + client = Mock() client.getBuild = Mock( return_value={ @@ -176,6 +177,7 @@ class TestFlagCommitBuildToddler: } ) mock_koji.ClientSession = Mock(return_value=client) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 @@ -202,14 +204,12 @@ class TestFlagCommitBuildToddler: == "Request to https://src.fedoraproject.org returned: 401" ) - @patch("toddlers.plugins.flag_commit_build.requests_session") @patch("toddlers.plugins.flag_commit_build.koji") - def test_process_flag_failed_cancel( - self, mock_koji, mock_requests, toddler, caplog - ): - mock_requests.request.return_value = MagicMock( + def test_process_flag_failed_cancel(self, mock_koji, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=False, status_code=401, text="invalid" ) + client = Mock() client.getBuild = Mock( return_value={ @@ -222,6 +222,7 @@ class TestFlagCommitBuildToddler: } ) mock_koji.ClientSession = Mock(return_value=client) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 @@ -248,12 +249,12 @@ class TestFlagCommitBuildToddler: == "Request to https://src.fedoraproject.org returned: 401" ) - @patch("toddlers.plugins.flag_commit_build.requests_session") @patch("toddlers.plugins.flag_commit_build.koji") - def test_process_flag_pass(self, mock_koji, mock_requests, toddler, caplog): - mock_requests.request.return_value = MagicMock( + def test_process_flag_pass(self, mock_koji, toddler, caplog): + toddler.requests_session.request.return_value = MagicMock( ok=True, status_code=200, text="woohoo!" ) + client = Mock() client.getBuild = Mock( return_value={ @@ -266,6 +267,7 @@ class TestFlagCommitBuildToddler: } ) mock_koji.ClientSession = Mock(return_value=client) + caplog.set_level(logging.INFO) msg = fedora_messaging.api.Message() msg.id = 123 diff --git a/tests/plugins/test_pdc_retired_packages.py b/tests/plugins/test_pdc_retired_packages.py index aaf69ed..6e8e043 100644 --- a/tests/plugins/test_pdc_retired_packages.py +++ b/tests/plugins/test_pdc_retired_packages.py @@ -61,8 +61,8 @@ class TestPDCRetiredPackagesToddler: process_dg.assert_called_once_with({"config": "foobar"}, client, msg) - @patch("toddlers.plugins.pdc_retired_packages._is_retired_in_dist_git") - def test_process_dist_git(self, retired_in_dg, toddler): + def test_process_dist_git(self, toddler): + retired_in_dg = toddler._is_retired_in_dist_git = MagicMock() page_component_branches = [ { "id": 44, @@ -122,10 +122,8 @@ class TestPDCRetiredPackagesToddler: ] ) - @patch("toddlers.plugins.pdc_retired_packages._is_retired_in_dist_git") - def test_process_dist_git_invalid_pdc_namespace( - self, retired_in_dg, caplog, toddler - ): + def test_process_dist_git_invalid_pdc_namespace(self, caplog, toddler): + retired_in_dg = toddler._is_retired_in_dist_git = MagicMock() caplog.set_level(logging.DEBUG) page_component_branches = [ { @@ -189,9 +187,9 @@ class TestPDCRetiredPackagesToddler: in caplog.text ) - @patch("toddlers.plugins.pdc_retired_packages._retire_branch") - @patch("toddlers.plugins.pdc_retired_packages._is_retired_in_dist_git") - def test_process_dist_git_full_distgit(self, retired_in_dg, retire_branch, toddler): + def test_process_dist_git_full_distgit(self, toddler): + retired_in_dg = toddler._is_retired_in_dist_git = MagicMock() + retire_branch = toddler._retire_branch = MagicMock() page_component_branches = [ { "id": 44, @@ -497,12 +495,11 @@ class TestPDCRetiredPackagesToddler: assert caplog.records[-1].message == '"rpms/0ad" not active in PDC in master' - @patch("requests.head") - def test__process_single_package_package_not_retired(self, req, toddler, caplog): + def test__process_single_package_package_not_retired(self, toddler, caplog): caplog.set_level(logging.INFO) resp = Mock() resp.status_code = 404 - req.return_value = resp + toddler.requests_session.head.return_value = resp page_component_branches = { "count": 1, @@ -553,13 +550,12 @@ class TestPDCRetiredPackagesToddler: == "Seems not to actually be retired, possibly a merge commit?" ) - @patch("toddlers.plugins.pdc_retired_packages._retire_branch") - @patch("requests.head") - def test__process_single_package(self, req, retire, toddler, caplog): + def test__process_single_package(self, toddler, caplog): + toddler._retire_branch = MagicMock() caplog.set_level(logging.INFO) resp = Mock() resp.status_code = 200 - req.return_value = resp + toddler.requests_session.head.return_value = resp page_component_branches = { "count": 1, @@ -605,40 +601,36 @@ class TestPDCRetiredPackagesToddler: msg, ) - retire.assert_called_once_with(client, page_component_branches["results"][0]) + toddler._retire_branch.assert_called_once_with( + client, page_component_branches["results"][0] + ) - @patch("toddlers.plugins.pdc_retired_packages.requests_session.head") - def test__is_retired_in_dist_git(self, req): + def test__is_retired_in_dist_git(self, toddler): resp = Mock() resp.status_code = 200 - req.return_value = resp + toddler.requests_session.head.return_value = resp - assert toddlers.plugins.pdc_retired_packages._is_retired_in_dist_git( - "rpms", "guake", "f33" - ) - req.assert_called_once_with( + assert toddler._is_retired_in_dist_git("rpms", "guake", "f33") + toddler.requests_session.head.assert_called_once_with( "https://src.fedoraproject.org//rpms/guake/raw/f33/f/dead.package" ) - @patch("toddlers.plugins.pdc_retired_packages.requests_session.head") - def test__is_retired_in_dist_git_internal_error(self, req): + def test__is_retired_in_dist_git_internal_error(self, toddler): resp = Mock() resp.status_code = 500 - req.return_value = resp + toddler.requests_session.head.return_value = resp with pytest.raises( ValueError, match=r"The connection to dist_git failed. Retirement status could " "not be determined. The status code was: 500. The content was: .*", ): - toddlers.plugins.pdc_retired_packages._is_retired_in_dist_git( - "rpms", "guake", "f33" - ) - req.assert_called_once_with( + toddler._is_retired_in_dist_git("rpms", "guake", "f33") + toddler.requests_session.head.assert_called_once_with( "https://src.fedoraproject.org//rpms/guake/raw/f33/f/dead.package" ) - def test_retire_branch(self, caplog): + def test_retire_branch(self, toddler, caplog): caplog.set_level(logging.INFO) today = datetime.datetime.utcnow().date() eol = (datetime.datetime.utcnow() + datetime.timedelta(days=60)).date() @@ -653,7 +645,7 @@ class TestPDCRetiredPackagesToddler: "critical_path": False, } - toddlers.plugins.pdc_retired_packages._retire_branch(pdc, branch) + toddler._retire_branch(pdc, branch) assert ( caplog.records[-1].message diff --git a/toddlers/plugins/flag_ci_pr.py b/toddlers/plugins/flag_ci_pr.py index c4e9043..6b57d29 100644 --- a/toddlers/plugins/flag_ci_pr.py +++ b/toddlers/plugins/flag_ci_pr.py @@ -10,24 +10,12 @@ Authors: Pierre-Yves Chibon import hashlib import logging -import requests -from requests.packages.urllib3.util import retry - from ..base import ToddlerBase +from ..utils.requests import make_session _log = logging.getLogger(__name__) -timeout = (30, 30) -retries = 3 -requests_session = requests.Session() -retry_conf = retry.Retry(total=retries, connect=retries, read=retries, backoff_factor=1) -retry_conf.BACKOFF_MAX = 5 -requests_session.mount("http://", requests.adapters.HTTPAdapter(max_retries=retry_conf)) -requests_session.mount( - "https://", requests.adapters.HTTPAdapter(max_retries=retry_conf) -) - done_states = { "passed": {"api": "success", "human": "passed"}, "error": {"api": "error", "human": "errored"}, @@ -49,6 +37,9 @@ class FlagCIPR(ToddlerBase): "org.centos.*.ci.dist-git-pr.test.running", ] + def __init__(self): + self.requests_session = make_session() + def accepts_topic(self, topic): """Returns a boolean whether this toddler is interested in messages from this specific topic. @@ -135,8 +126,8 @@ class FlagCIPR(ToddlerBase): _log.info("payload: %s" % data) - req = requests_session.request( - method="POST", url=flag_url, headers=headers, data=data, + req = self.requests_session.request( + method="POST", url=flag_url, headers=headers, data=data ) _log.info("Request to %s returned: %s" % (dist_git_url, req.status_code)) if not req.ok: diff --git a/toddlers/plugins/flag_commit_build.py b/toddlers/plugins/flag_commit_build.py index 76239da..b62c085 100644 --- a/toddlers/plugins/flag_commit_build.py +++ b/toddlers/plugins/flag_commit_build.py @@ -10,24 +10,13 @@ Authors: Pierre-Yves Chibon import logging import koji -import requests -from requests.packages.urllib3.util import retry from ..base import ToddlerBase +from ..utils.requests import make_session _log = logging.getLogger(__name__) -timeout = (30, 30) -retries = 3 -requests_session = requests.Session() -retry_conf = retry.Retry(total=retries, connect=retries, read=retries, backoff_factor=1) -retry_conf.BACKOFF_MAX = 5 -requests_session.mount("http://", requests.adapters.HTTPAdapter(max_retries=retry_conf)) -requests_session.mount( - "https://", requests.adapters.HTTPAdapter(max_retries=retry_conf) -) - # see koji.TASK_STATES for all values done_states = { 1: "completed", @@ -56,6 +45,9 @@ class FlagCommitBuild(ToddlerBase): "org.fedoraproject.*.buildsys.build.state.change", ] + def __init__(self): + self.requests_session = make_session() + def accepts_topic(self, topic): """Returns a boolean whether this toddler is interested in messages from this specific topic. @@ -141,8 +133,8 @@ class FlagCommitBuild(ToddlerBase): _log.debug("payload: %s" % data) - req = requests_session.request( - method="POST", url=flag_url, headers=headers, data=data, + req = self.requests_session.request( + method="POST", url=flag_url, headers=headers, data=data ) _log.info("Request to %s returned: %s" % (dist_git_url, req.status_code)) if not req.ok: diff --git a/toddlers/plugins/packager_bugzilla_sync.py b/toddlers/plugins/packager_bugzilla_sync.py index 629737f..cc4ecd0 100644 --- a/toddlers/plugins/packager_bugzilla_sync.py +++ b/toddlers/plugins/packager_bugzilla_sync.py @@ -12,8 +12,6 @@ import argparse import logging import sys -import requests -from requests.packages.urllib3.util import retry import toml try: @@ -28,16 +26,6 @@ from ..utils import fedora_account _log = logging.getLogger(__name__) -timeout = (30, 30) -retries = 3 -requests_session = requests.Session() -retry_conf = retry.Retry(total=retries, connect=retries, read=retries, backoff_factor=1) -retry_conf.BACKOFF_MAX = 5 -requests_session.mount("http://", requests.adapters.HTTPAdapter(max_retries=retry_conf)) -requests_session.mount( - "https://", requests.adapters.HTTPAdapter(max_retries=retry_conf) -) - class PackagerBugzillaSync(ToddlerBase): """ Listens to messages sent by playtime (which lives in toddlers) to sync diff --git a/toddlers/plugins/pdc_retired_packages.py b/toddlers/plugins/pdc_retired_packages.py index 249e38f..0c3de1d 100644 --- a/toddlers/plugins/pdc_retired_packages.py +++ b/toddlers/plugins/pdc_retired_packages.py @@ -12,23 +12,12 @@ from datetime import datetime import logging import pdc_client -import requests -from requests.packages.urllib3.util import retry from ..base import ToddlerBase +from ..utils.requests import make_session _log = logging.getLogger(__name__) -timeout = (30, 30) -retries = 3 -requests_session = requests.Session() -retry_conf = retry.Retry(total=retries, connect=retries, read=retries, backoff_factor=1) -retry_conf.BACKOFF_MAX = 5 -requests_session.mount("http://", requests.adapters.HTTPAdapter(max_retries=retry_conf)) -requests_session.mount( - "https://", requests.adapters.HTTPAdapter(max_retries=retry_conf) -) - class PDCRetiredPackages(ToddlerBase): """ Listens to messages sent by playtime (which lives in toddlers) to sync @@ -42,6 +31,9 @@ class PDCRetiredPackages(ToddlerBase): "org.fedoraproject.*.git.receive", ] + def __init__(self): + self.requests_session = make_session() + def accepts_topic(self, topic): """Returns a boolean whether this toddler is interested in messages from this specific topic. @@ -71,8 +63,8 @@ class PDCRetiredPackages(ToddlerBase): _log.debug("Considering {0}".format(branch_str)) retired_in_dist_git = False try: - retired_in_dist_git = _is_retired_in_dist_git( - namespace=_pdc_to_namespace(branch["type"]), + retired_in_dist_git = self._is_retired_in_dist_git( + namespace=self._pdc_to_namespace(branch["type"]), repo=branch["global_component"], branch=branch["name"], ) @@ -81,7 +73,7 @@ class PDCRetiredPackages(ToddlerBase): continue if retired_in_dist_git: - _retire_branch(pdc, branch) + self._retire_branch(pdc, branch) def _process_single_package(self, config, pdc, message): """Handle an incoming bus message. @@ -110,7 +102,7 @@ class PDCRetiredPackages(ToddlerBase): repo = msg["commit"]["repo"] namespace = msg["commit"]["namespace"] try: - component_type = _namespace_to_pdc(namespace) + component_type = self._namespace_to_pdc(namespace) except ValueError as err: _log.info("Ignoring namespace '%s', error: %s", namespace, err) return @@ -150,66 +142,62 @@ class PDCRetiredPackages(ToddlerBase): "file": "dead.package", } _log.info("Checking for file: %s", fileurl) - resp = requests.head(fileurl, timeout=15) + resp = self.requests_session.head(fileurl, timeout=15) if resp.status_code != 200: _log.info("Seems not to actually be retired, possibly a merge commit?") return - _retire_branch(pdc, branch) + self._retire_branch(pdc, branch) + def _namespace_to_pdc(self, namespace): + """ Internal method to translate a dist-git namespace to a PDC + component type. """ + namespace_to_pdc = { + "rpms": "rpm", + "modules": "module", + "container": "container", + } + if namespace not in namespace_to_pdc: + raise ValueError('The namespace "{0}" is not supported'.format(namespace)) + else: + return namespace_to_pdc[namespace] -def _namespace_to_pdc(namespace): - """ Internal method to translate a dist-git namespace to a PDC - component type. """ - namespace_to_pdc = { - "rpms": "rpm", - "modules": "module", - "container": "container", - } - if namespace not in namespace_to_pdc: - raise ValueError('The namespace "{0}" is not supported'.format(namespace)) - else: - return namespace_to_pdc[namespace] + def _pdc_to_namespace(self, pdc_type): + """ Internal method to translate a PDC component type to a dist-git + namespace. """ + pdc_to_namespace = { + "rpm": "rpms", + "module": "modules", + "container": "container", + } + if pdc_type not in pdc_to_namespace: + raise ValueError('The PDC type "{0}" is not supported'.format(pdc_type)) + else: + return pdc_to_namespace[pdc_type] - -def _pdc_to_namespace(pdc_type): - """ Internal method to translate a PDC component type to a dist-git - namespace. """ - pdc_to_namespace = { - "rpm": "rpms", - "module": "modules", - "container": "container", - } - if pdc_type not in pdc_to_namespace: - raise ValueError('The PDC type "{0}" is not supported'.format(pdc_type)) - else: - return pdc_to_namespace[pdc_type] - - -def _is_retired_in_dist_git(namespace, repo, branch): - base = "https://src.fedoraproject.org/" - # Check to see if they have a dead.package file in dist-git - url = "{base}/{namespace}/{repo}/raw/{branch}/f/dead.package" - response = requests_session.head( - url.format(base=base, namespace=namespace, repo=repo, branch=branch,) - ) - - # If there is a dead.package, then the branch is retired in dist_git - if response.status_code in [200, 404]: - return response.status_code == 200 - else: - raise ValueError( - "The connection to dist_git failed. Retirement status could not " - "be determined. The status code was: {0}. The content was: " - "{1}".format(response.status_code, response.content) + def _is_retired_in_dist_git(self, namespace, repo, branch): + base = "https://src.fedoraproject.org/" + # Check to see if they have a dead.package file in dist-git + url = "{base}/{namespace}/{repo}/raw/{branch}/f/dead.package" + response = self.requests_session.head( + url.format(base=base, namespace=namespace, repo=repo, branch=branch) ) + # If there is a dead.package, then the branch is retired in dist_git + if response.status_code in [200, 404]: + return response.status_code == 200 + else: + raise ValueError( + "The connection to dist_git failed. Retirement status could not " + "be determined. The status code was: {0}. The content was: " + "{1}".format(response.status_code, response.content) + ) -def _retire_branch(pdc, branch): - """ Internal method for retiring a branch in PDC. """ - today = datetime.utcnow().date() - for sla in branch["slas"]: - sla_eol = datetime.strptime(sla["eol"], "%Y-%m-%d").date() - if sla_eol > today: - _log.info("Setting eol of branch: %s to %s", branch, today) - pdc["component-branch-slas"][sla["id"]]._ += {"eol": str(today)} + def _retire_branch(self, pdc, branch): + """ Internal method for retiring a branch in PDC. """ + today = datetime.utcnow().date() + for sla in branch["slas"]: + sla_eol = datetime.strptime(sla["eol"], "%Y-%m-%d").date() + if sla_eol > today: + _log.info("Setting eol of branch: %s to %s", branch, today) + pdc["component-branch-slas"][sla["id"]]._ += {"eol": str(today)} diff --git a/toddlers/utils/requests.py b/toddlers/utils/requests.py new file mode 100644 index 0000000..aef1383 --- /dev/null +++ b/toddlers/utils/requests.py @@ -0,0 +1,31 @@ +"""Common requests session object.""" + +import requests +from requests.packages.urllib3.util import retry + + +__all__ = ["make_session"] + +RETRY_DEFAULTS = { + "total": 3, + "backoff_factor": 1, + "BACKOFF_MAX": 5, +} + + +def make_session(**retry_args): + for k, v in RETRY_DEFAULTS.items(): + retry_args.setdefault(k, v) + + BACKOFF_MAX = retry_args.pop("BACKOFF_MAX") + + retry_conf = retry.Retry(**retry_args) + retry_conf.BACKOFF_MAX = BACKOFF_MAX + + http_adapter = requests.adapters.HTTPAdapter(max_retries=retry_conf) + + session = requests.Session() + for prefix in ("http://", "https://"): + session.mount(prefix, http_adapter) + + return session