From 3b1d3a41da7e7228f3a465d06902db8af564153e Mon Sep 17 00:00:00 2001 From: Karun Japhet Date: Sat, 28 Aug 2021 11:41:59 +0530 Subject: [PATCH] feat: allow global retry_transient_errors setup `retry_transient_errors` can now be set through the Gitlab instance and global configuration Documentation for API usage has been updated and missing tests have been added. --- docs/api-usage.rst | 11 +++ gitlab/client.py | 12 ++-- gitlab/config.py | 14 ++++ tests/unit/conftest.py | 15 +++- tests/unit/test_config.py | 70 +++++++++++++++++++ tests/unit/test_gitlab_http_methods.py | 95 ++++++++++++++++++++++++-- 6 files changed, 206 insertions(+), 11 deletions(-) diff --git a/docs/api-usage.rst b/docs/api-usage.rst index e911664b7..2f7166e89 100644 --- a/docs/api-usage.rst +++ b/docs/api-usage.rst @@ -391,6 +391,17 @@ default an exception is raised for these errors. gl = gitlab.gitlab(url, token, api_version=4) gl.projects.list(all=True, retry_transient_errors=True) +The default ``retry_transient_errors`` can also be set on the ``Gitlab`` object +and overridden by individual API calls. + +.. code-block:: python + + import gitlab + import requests + gl = gitlab.gitlab(url, token, api_version=4, retry_transient_errors=True) + gl.projects.list(all=True) # retries due to default value + gl.projects.list(all=True, retry_transient_errors=False) # does not retry + Timeout ------- diff --git a/gitlab/client.py b/gitlab/client.py index ef5b0da2a..47fae8167 100644 --- a/gitlab/client.py +++ b/gitlab/client.py @@ -52,6 +52,8 @@ class Gitlab(object): pagination (str): Can be set to 'keyset' to use keyset pagination order_by (str): Set order_by globally user_agent (str): A custom user agent to use for making HTTP requests. + retry_transient_errors (bool): Whether to retry after 500, 502, 503, or + 504 responses. Defaults to False. """ def __init__( @@ -70,6 +72,7 @@ def __init__( pagination: Optional[str] = None, order_by: Optional[str] = None, user_agent: str = gitlab.const.USER_AGENT, + retry_transient_errors: bool = False, ) -> None: self._api_version = str(api_version) @@ -79,6 +82,7 @@ def __init__( self._url = "%s/api/v%s" % (self._base_url, api_version) #: Timeout to use for requests to gitlab server self.timeout = timeout + self.retry_transient_errors = retry_transient_errors #: Headers that will be used in request to GitLab self.headers = {"User-Agent": user_agent} @@ -246,6 +250,7 @@ def from_config( pagination=config.pagination, order_by=config.order_by, user_agent=config.user_agent, + retry_transient_errors=config.retry_transient_errors, ) def auth(self) -> None: @@ -511,7 +516,6 @@ def http_request( files: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, obey_rate_limit: bool = True, - retry_transient_errors: bool = False, max_retries: int = 10, **kwargs: Any, ) -> requests.Response: @@ -531,9 +535,6 @@ def http_request( timeout (float): The timeout, in seconds, for the request obey_rate_limit (bool): Whether to obey 429 Too Many Request responses. Defaults to True. - retry_transient_errors (bool): Whether to retry after 500, 502, - 503, or 504 responses. Defaults - to False. max_retries (int): Max retries after 429 or transient errors, set to -1 to retry forever. Defaults to 10. **kwargs: Extra options to send to the server (e.g. sudo) @@ -598,6 +599,9 @@ def http_request( if 200 <= result.status_code < 300: return result + retry_transient_errors = kwargs.get( + "retry_transient_errors", self.retry_transient_errors + ) if (429 == result.status_code and obey_rate_limit) or ( result.status_code in [500, 502, 503, 504] and retry_transient_errors ): diff --git a/gitlab/config.py b/gitlab/config.py index 9363b6487..ba14468c5 100644 --- a/gitlab/config.py +++ b/gitlab/config.py @@ -206,6 +206,20 @@ def __init__( except Exception: pass + self.retry_transient_errors = False + try: + self.retry_transient_errors = self._config.getboolean( + "global", "retry_transient_errors" + ) + except Exception: + pass + try: + self.retry_transient_errors = self._config.getboolean( + self.gitlab_id, "retry_transient_errors" + ) + except Exception: + pass + def _get_values_from_helper(self) -> None: """Update attributes that may get values from an external helper program""" for attr in HELPER_ATTRIBUTES: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 64df0517a..f58c77a75 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -9,7 +9,18 @@ def gl(): "http://localhost", private_token="private_token", ssl_verify=True, - api_version=4, + api_version="4", + ) + + +@pytest.fixture +def gl_retry(): + return gitlab.Gitlab( + "http://localhost", + private_token="private_token", + ssl_verify=True, + api_version="4", + retry_transient_errors=True, ) @@ -17,7 +28,7 @@ def gl(): @pytest.fixture def gl_trailing(): return gitlab.Gitlab( - "http://localhost/", private_token="private_token", api_version=4 + "http://localhost/", private_token="private_token", api_version="4" ) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index cd61b8d4a..a62106b27 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -86,6 +86,31 @@ """ +def global_retry_transient_errors(value: bool) -> str: + return u"""[global] +default = one +retry_transient_errors={} +[one] +url = http://one.url +private_token = ABCDEF""".format( + value + ) + + +def global_and_gitlab_retry_transient_errors( + global_value: bool, gitlab_value: bool +) -> str: + return u"""[global] + default = one + retry_transient_errors={global_value} + [one] + url = http://one.url + private_token = ABCDEF + retry_transient_errors={gitlab_value}""".format( + global_value=global_value, gitlab_value=gitlab_value + ) + + @mock.patch.dict(os.environ, {"PYTHON_GITLAB_CFG": "/some/path"}) def test_env_config_present(): assert ["/some/path"] == config._env_config() @@ -245,3 +270,48 @@ def test_config_user_agent(m_open, path_exists, config_string, expected_agent): cp = config.GitlabConfigParser() assert cp.user_agent == expected_agent + + +@mock.patch("os.path.exists") +@mock.patch("builtins.open") +@pytest.mark.parametrize( + "config_string,expected", + [ + pytest.param(valid_config, False, id="default_value"), + pytest.param( + global_retry_transient_errors(True), True, id="global_config_true" + ), + pytest.param( + global_retry_transient_errors(False), False, id="global_config_false" + ), + pytest.param( + global_and_gitlab_retry_transient_errors(False, True), + True, + id="gitlab_overrides_global_true", + ), + pytest.param( + global_and_gitlab_retry_transient_errors(True, False), + False, + id="gitlab_overrides_global_false", + ), + pytest.param( + global_and_gitlab_retry_transient_errors(True, True), + True, + id="gitlab_equals_global_true", + ), + pytest.param( + global_and_gitlab_retry_transient_errors(False, False), + False, + id="gitlab_equals_global_false", + ), + ], +) +def test_config_retry_transient_errors_when_global_config_is_set( + m_open, path_exists, config_string, expected +): + fd = io.StringIO(config_string) + fd.close = mock.Mock(return_value=None) + m_open.return_value = fd + + cp = config.GitlabConfigParser() + assert cp.retry_transient_errors == expected diff --git a/tests/unit/test_gitlab_http_methods.py b/tests/unit/test_gitlab_http_methods.py index f1bc9cd84..5a3584e5c 100644 --- a/tests/unit/test_gitlab_http_methods.py +++ b/tests/unit/test_gitlab_http_methods.py @@ -30,7 +30,7 @@ def resp_cont(url, request): def test_http_request_404(gl): @urlmatch(scheme="http", netloc="localhost", path="/api/v4/not_there", method="get") def resp_cont(url, request): - content = {"Here is wh it failed"} + content = {"Here is why it failed"} return response(404, content, {}, None, 5, request) with HTTMock(resp_cont): @@ -38,6 +38,91 @@ def resp_cont(url, request): gl.http_request("get", "/not_there") +@pytest.mark.parametrize("status_code", [500, 502, 503, 504]) +def test_http_request_with_only_failures(gl, status_code): + call_count = 0 + + @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get") + def resp_cont(url, request): + nonlocal call_count + call_count += 1 + return response(status_code, {"Here is why it failed"}, {}, None, 5, request) + + with HTTMock(resp_cont): + with pytest.raises(GitlabHttpError): + gl.http_request("get", "/projects") + + assert call_count == 1 + + +def test_http_request_with_retry_on_method_for_transient_failures(gl): + call_count = 0 + calls_before_success = 3 + + @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get") + def resp_cont(url, request): + nonlocal call_count + call_count += 1 + status_code = 200 if call_count == calls_before_success else 500 + return response( + status_code, + {"Failure is the stepping stone to success"}, + {}, + None, + 5, + request, + ) + + with HTTMock(resp_cont): + http_r = gl.http_request("get", "/projects", retry_transient_errors=True) + + assert http_r.status_code == 200 + assert call_count == calls_before_success + + +def test_http_request_with_retry_on_class_for_transient_failures(gl_retry): + call_count = 0 + calls_before_success = 3 + + @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get") + def resp_cont(url, request): + nonlocal call_count + call_count += 1 + status_code = 200 if call_count == calls_before_success else 500 + return response( + status_code, + {"Failure is the stepping stone to success"}, + {}, + None, + 5, + request, + ) + + with HTTMock(resp_cont): + http_r = gl_retry.http_request("get", "/projects") + + assert http_r.status_code == 200 + assert call_count == calls_before_success + + +def test_http_request_with_retry_on_class_and_method_for_transient_failures(gl_retry): + call_count = 0 + calls_before_success = 3 + + @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get") + def resp_cont(url, request): + nonlocal call_count + call_count += 1 + status_code = 200 if call_count == calls_before_success else 500 + return response(status_code, {"Here is why it failed"}, {}, None, 5, request) + + with HTTMock(resp_cont): + with pytest.raises(GitlabHttpError): + gl_retry.http_request("get", "/projects", retry_transient_errors=False) + + assert call_count == 1 + + def test_get_request(gl): @urlmatch(scheme="http", netloc="localhost", path="/api/v4/projects", method="get") def resp_cont(url, request): @@ -66,7 +151,7 @@ def resp_cont(url, request): def test_get_request_404(gl): @urlmatch(scheme="http", netloc="localhost", path="/api/v4/not_there", method="get") def resp_cont(url, request): - content = {"Here is wh it failed"} + content = {"Here is why it failed"} return response(404, content, {}, None, 5, request) with HTTMock(resp_cont): @@ -150,7 +235,7 @@ def test_post_request_404(gl): scheme="http", netloc="localhost", path="/api/v4/not_there", method="post" ) def resp_cont(url, request): - content = {"Here is wh it failed"} + content = {"Here is why it failed"} return response(404, content, {}, None, 5, request) with HTTMock(resp_cont): @@ -186,7 +271,7 @@ def resp_cont(url, request): def test_put_request_404(gl): @urlmatch(scheme="http", netloc="localhost", path="/api/v4/not_there", method="put") def resp_cont(url, request): - content = {"Here is wh it failed"} + content = {"Here is why it failed"} return response(404, content, {}, None, 5, request) with HTTMock(resp_cont): @@ -226,7 +311,7 @@ def test_delete_request_404(gl): scheme="http", netloc="localhost", path="/api/v4/not_there", method="delete" ) def resp_cont(url, request): - content = {"Here is wh it failed"} + content = {"Here is why it failed"} return response(404, content, {}, None, 5, request) with HTTMock(resp_cont):