Merge pull request #1565 from javatarz/master

nejch · web-flow · commit d98d948f997e · 2021-08-31T00:30:06.000+02:00
feat: allow global retry_transient_errors
diff --git 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
@@ -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
@@ -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
@@ -9,15 +9,26 @@ 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,
     )
 
 
 # Todo: parametrize, but check what tests it's really useful for
 @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
@@ -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
@@ -30,14 +30,99 @@ 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):
         with pytest.raises(GitlabHttpError):
             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):
