From 0ecfe340d75b28cb6397947d769fc8b40c306750 Mon Sep 17 00:00:00 2001
From: Martin <maarand@teklia.com>
Date: Fri, 8 Jan 2021 10:46:20 +0100
Subject: [PATCH] retry all api calls in the same try-except to handle them in
the same way
---
arkindex_worker/git.py | 52 ++++--
tests/test_git.py | 414 +++++++++++++++++++++++++++++++----------
2 files changed, 352 insertions(+), 114 deletions(-)
diff --git a/arkindex_worker/git.py b/arkindex_worker/git.py
index 463ec714..4f4f8e41 100644
--- a/arkindex_worker/git.py
+++ b/arkindex_worker/git.py
@@ -6,6 +6,7 @@ from datetime import datetime
from pathlib import Path
import gitlab
+import requests
import sh
from arkindex_worker import logger
@@ -57,23 +58,20 @@ class GitlabHelper:
:param title: title of the merge request
:return: was the branch successfully merged?
"""
- logger.info(f"Creating a merge request for {branch_name}")
- mr = self.project.mergerequests.create(
- {
- "source_branch": branch_name,
- "target_branch": self.branch,
- "title": title,
- }
- )
+ mr = None
# always rebase first, because other workers might have merged already
for i in range(self.max_rebase_tries):
logger.info(f"Trying to merge, try nr: {i}")
- mr.rebase()
- rebase_success = self._wait_for_rebase_to_finish(mr.iid)
- if not rebase_success:
- logger.error("Rebase failed, won't be able to merge!")
- return False
try:
+ if mr is None:
+ mr = self._create_merge_request(branch_name, title)
+
+ mr.rebase()
+ rebase_success = self._wait_for_rebase_to_finish(mr.iid)
+ if not rebase_success:
+ logger.error("Rebase failed, won't be able to merge!")
+ return False
+
mr.merge(should_remove_source_branch=self.delete_source_branch)
logger.info("Merge successful")
return True
@@ -84,8 +82,30 @@ class GitlabHelper:
else:
logger.error(f"Merge was not successful: {e}")
return False
+ except gitlab.GitlabError as e:
+ logger.error(f"Gitlab error: {e}")
+ if 400 <= e.response_code < 500:
+ # 4XX errors shouldn't be fixed by retrying
+ raise e
+ except requests.exceptions.ConnectionError as e:
+ logger.error(f"Server connection error, will wait and retry: {e}")
+ time.sleep(self.rebase_wait_period)
+
return False
+ def _create_merge_request(self, branch_name, title):
+ logger.info(f"Creating a merge request for {branch_name}")
+ # retry_transient_error will retry the request on 50X errors
+ # https://github.com/python-gitlab/python-gitlab/blob/265dbbdd37af88395574564aeb3fd0350288a18c/gitlab/__init__.py#L539
+ mr = self.project.mergerequests.create(
+ {
+ "source_branch": branch_name,
+ "target_branch": self.branch,
+ "title": title,
+ },
+ )
+ return mr
+
def _get_merge_request(self, merge_request_id, include_rebase_in_progress=True):
return self.project.mergerequests.get(
merge_request_id, include_rebase_in_progress=include_rebase_in_progress
@@ -315,7 +335,11 @@ class GitHelper:
self._git.push("-u", "origin", "HEAD")
if self.gitlab_helper:
- self.gitlab_helper.merge(branch_name, f"Merge {branch_name}")
+ try:
+ self.gitlab_helper.merge(branch_name, f"Merge {branch_name}")
+ except Exception as e:
+ logger.error(f"Merge failed: {e}")
+ raise e
else:
logger.info(
"No gitlab_helper defined, not trying to merge the pushed branch"
diff --git a/tests/test_git.py b/tests/test_git.py
index fe20ae83..4b10cddd 100644
--- a/tests/test_git.py
+++ b/tests/test_git.py
@@ -2,9 +2,39 @@
from pathlib import Path
import pytest
+from gitlab import GitlabCreateError, GitlabError
+from requests import ConnectionError
from arkindex_worker.git import GitlabHelper
+PROJECT_ID = 21259233
+MERGE_REQUEST_ID = 7
+SOURCE_BRANCH = "new_branch"
+TARGET_BRANCH = "master"
+MR_TITLE = "merge request title"
+CREATE_MR_RESPONSE_JSON = {
+ "id": 107,
+ "iid": MERGE_REQUEST_ID,
+ "project_id": PROJECT_ID,
+ "title": MR_TITLE,
+ "target_branch": TARGET_BRANCH,
+ "source_branch": SOURCE_BRANCH,
+ # several fields omitted
+}
+
+
+@pytest.fixture
+def fake_responses(responses):
+ responses.add(
+ responses.GET,
+ "https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing",
+ json={
+ "id": PROJECT_ID,
+ # several fields omitted
+ },
+ )
+ return responses
+
def test_clone_done(fake_git_helper):
assert not fake_git_helper.is_clone_finished
@@ -91,23 +121,11 @@ def test_merge__rebase_failed(mocker):
assert merqe_request.merge.call_count == 0
-def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
- project_id = 21259233
- merge_request_id = 7
-
- responses.add(
- responses.GET,
- "https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing",
- json={
- "id": project_id,
- # several fields omitted
- },
- )
+def test_wait_for_rebase_to_finish(fake_responses, fake_gitlab_helper_factory):
+ get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
- get_mr_url = f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}?include_rebase_in_progress=True"
-
- responses.add(
- responses.GET,
+ fake_responses.add(
+ fake_responses.GET,
get_mr_url,
json={
"rebase_in_progress": True,
@@ -115,8 +133,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
},
)
- responses.add(
- responses.GET,
+ fake_responses.add(
+ fake_responses.GET,
get_mr_url,
json={
"rebase_in_progress": True,
@@ -124,8 +142,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
},
)
- responses.add(
- responses.GET,
+ fake_responses.add(
+ fake_responses.GET,
get_mr_url,
json={
"rebase_in_progress": False,
@@ -135,129 +153,325 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
gitlab_helper = fake_gitlab_helper_factory()
- success = gitlab_helper._wait_for_rebase_to_finish(merge_request_id)
+ success = gitlab_helper._wait_for_rebase_to_finish(MERGE_REQUEST_ID)
assert success
- assert len(responses.calls) == 4
+ assert len(fake_responses.calls) == 4
assert gitlab_helper.is_rebase_finished
-def test_merge_request(responses, fake_gitlab_helper_factory, mocker):
- project_id = 21259233
- merge_request_id = 7
- source_branch = "new_branch"
- target_branch = "master"
- mr_title = "merge request title"
+def test_wait_for_rebase_to_finish__fail_connection_error(
+ fake_responses, fake_gitlab_helper_factory
+):
+ get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
- responses.add(
- responses.GET,
- "https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing",
- json={
- "id": project_id,
- # several fields omitted
- },
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
+ body=ConnectionError(),
)
- responses.add(
- responses.POST,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests",
- json={
- "id": 107,
- "iid": merge_request_id,
- "project_id": project_id,
- "title": mr_title,
- "target_branch": target_branch,
- "source_branch": source_branch,
- # several fields omitted
- },
+ gitlab_helper = fake_gitlab_helper_factory()
+
+ with pytest.raises(ConnectionError):
+ gitlab_helper._wait_for_rebase_to_finish(MERGE_REQUEST_ID)
+
+ assert len(fake_responses.calls) == 2
+ assert not gitlab_helper.is_rebase_finished
+
+
+def test_wait_for_rebase_to_finish__fail_server_error(
+ fake_responses, fake_gitlab_helper_factory
+):
+ get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
+
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
+ body="Service Unavailable",
+ status=503,
)
- responses.add(
- responses.PUT,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/rebase",
+ gitlab_helper = fake_gitlab_helper_factory()
+
+ with pytest.raises(GitlabError):
+ gitlab_helper._wait_for_rebase_to_finish(MERGE_REQUEST_ID)
+
+ assert len(fake_responses.calls) == 2
+ assert not gitlab_helper.is_rebase_finished
+
+
+def test_merge_request(fake_responses, fake_gitlab_helper_factory, mocker):
+ fake_responses.add(
+ fake_responses.POST,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests",
+ json=CREATE_MR_RESPONSE_JSON,
+ )
+
+ fake_responses.add(
+ fake_responses.PUT,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/rebase",
json={},
)
- responses.add(
- responses.PUT,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/merge?should_remove_source_branch=True",
+ fake_responses.add(
+ fake_responses.PUT,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/merge?should_remove_source_branch=True",
json={
- "iid": merge_request_id,
+ "iid": MERGE_REQUEST_ID,
"state": "merged",
# several fields omitted
},
)
- # the responses are defined in the same order as they are expected to be called
- expected_http_methods = [r.method for r in responses._matches]
- expected_urls = [r.url for r in responses._matches]
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
gitlab_helper = fake_gitlab_helper_factory()
gitlab_helper._wait_for_rebase_to_finish = mocker.MagicMock()
gitlab_helper._wait_for_rebase_to_finish.return_value = True
- success = gitlab_helper.merge(source_branch, mr_title)
+ success = gitlab_helper.merge(SOURCE_BRANCH, MR_TITLE)
assert success
- assert len(responses.calls) == 4
- assert [c.request.method for c in responses.calls] == expected_http_methods
- assert [c.request.url for c in responses.calls] == expected_urls
+ assert len(fake_responses.calls) == 4
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
-def test_merge_request_fail(responses, fake_gitlab_helper_factory, mocker):
- project_id = 21259233
- merge_request_id = 7
- source_branch = "new_branch"
- target_branch = "master"
- mr_title = "merge request title"
+def test_merge_request_fail(fake_responses, fake_gitlab_helper_factory, mocker):
+ fake_responses.add(
+ fake_responses.POST,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests",
+ json=CREATE_MR_RESPONSE_JSON,
+ )
- responses.add(
- responses.GET,
- "https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing",
+ fake_responses.add(
+ fake_responses.PUT,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/rebase",
+ json={},
+ )
+
+ fake_responses.add(
+ fake_responses.PUT,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/merge?should_remove_source_branch=True",
+ json={"error": "Method not allowed"},
+ status=405,
+ )
+
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
+
+ gitlab_helper = fake_gitlab_helper_factory()
+ gitlab_helper._wait_for_rebase_to_finish = mocker.MagicMock()
+ gitlab_helper._wait_for_rebase_to_finish.return_value = True
+
+ success = gitlab_helper.merge(SOURCE_BRANCH, MR_TITLE)
+
+ assert not success
+ assert len(fake_responses.calls) == 4
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
+
+
+def test_merge_request__success_after_errors(
+ fake_responses, fake_gitlab_helper_factory
+):
+ fake_responses.add(
+ fake_responses.POST,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests",
+ json=CREATE_MR_RESPONSE_JSON,
+ )
+
+ rebase_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/rebase"
+
+ fake_responses.add(
+ fake_responses.PUT,
+ rebase_url,
+ json={"rebase_in_progress": True},
+ )
+
+ get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
+
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
+ body="Service Unavailable",
+ status=503,
+ )
+
+ fake_responses.add(
+ fake_responses.PUT,
+ rebase_url,
+ json={"rebase_in_progress": True},
+ )
+
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
+ body=ConnectionError(),
+ )
+
+ fake_responses.add(
+ fake_responses.PUT,
+ rebase_url,
+ json={"rebase_in_progress": True},
+ )
+
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
json={
- "id": project_id,
- # several fields omitted
+ "rebase_in_progress": True,
+ "merge_error": None,
},
)
- responses.add(
- responses.POST,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests",
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
json={
- "id": 107,
- "iid": merge_request_id,
- "project_id": project_id,
- "title": mr_title,
- "target_branch": target_branch,
- "source_branch": source_branch,
+ "rebase_in_progress": False,
+ "merge_error": None,
+ },
+ )
+
+ fake_responses.add(
+ fake_responses.PUT,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/merge?should_remove_source_branch=True",
+ json={
+ "iid": MERGE_REQUEST_ID,
+ "state": "merged",
# several fields omitted
},
)
- responses.add(
- responses.PUT,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/rebase",
- json={},
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
+
+ gitlab_helper = fake_gitlab_helper_factory()
+
+ success = gitlab_helper.merge(SOURCE_BRANCH, MR_TITLE)
+
+ assert success
+ assert len(fake_responses.calls) == 10
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
+
+
+def test_merge_request__fail_bad_request(fake_responses, fake_gitlab_helper_factory):
+ fake_responses.add(
+ fake_responses.POST,
+ f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests",
+ json=CREATE_MR_RESPONSE_JSON,
)
- responses.add(
- responses.PUT,
- f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/merge?should_remove_source_branch=True",
- json={"error": "Method not allowed"},
- status=405,
+ rebase_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/rebase"
+
+ fake_responses.add(
+ fake_responses.PUT,
+ rebase_url,
+ json={"rebase_in_progress": True},
)
- # the responses are defined in the same order as they are expected to be called
- expected_http_methods = [r.method for r in responses._matches]
- expected_urls = [r.url for r in responses._matches]
+ get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
+
+ fake_responses.add(
+ fake_responses.GET,
+ get_mr_url,
+ body="Bad Request",
+ status=400,
+ )
+
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
gitlab_helper = fake_gitlab_helper_factory()
- gitlab_helper._wait_for_rebase_to_finish = mocker.MagicMock()
- gitlab_helper._wait_for_rebase_to_finish.return_value = True
- success = gitlab_helper.merge(source_branch, mr_title)
+ with pytest.raises(GitlabError):
+ gitlab_helper.merge(SOURCE_BRANCH, MR_TITLE)
- assert not success
- assert len(responses.calls) == 4
- assert [c.request.method for c in responses.calls] == expected_http_methods
- assert [c.request.url for c in responses.calls] == expected_urls
+ assert len(fake_responses.calls) == 4
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
+
+
+def test_create_merge_request__no_retry_5xx_error(
+ fake_responses, fake_gitlab_helper_factory
+):
+ request_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests"
+
+ fake_responses.add(
+ fake_responses.POST,
+ request_url,
+ body="Service Unavailable",
+ status=503,
+ )
+
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
+
+ gitlab_helper = fake_gitlab_helper_factory()
+
+ with pytest.raises(GitlabCreateError):
+ gitlab_helper.project.mergerequests.create(
+ {
+ "source_branch": "branch",
+ "target_branch": gitlab_helper.branch,
+ "title": "MR title",
+ }
+ )
+
+ assert len(fake_responses.calls) == 2
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
+
+
+def test_create_merge_request__retry_5xx_error(
+ fake_responses, fake_gitlab_helper_factory
+):
+ request_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests?retry_transient_errors=True"
+
+ fake_responses.add(
+ fake_responses.POST,
+ request_url,
+ body="Service Unavailable",
+ status=503,
+ )
+
+ fake_responses.add(
+ fake_responses.POST,
+ request_url,
+ body="Service Unavailable",
+ status=503,
+ )
+
+ fake_responses.add(
+ fake_responses.POST,
+ request_url,
+ json=CREATE_MR_RESPONSE_JSON,
+ )
+
+ # the fake_responses are defined in the same order as they are expected to be called
+ expected_http_methods = [r.method for r in fake_responses._matches]
+ expected_urls = [r.url for r in fake_responses._matches]
+
+ gitlab_helper = fake_gitlab_helper_factory()
+
+ gitlab_helper.project.mergerequests.create(
+ {
+ "source_branch": "branch",
+ "target_branch": gitlab_helper.branch,
+ "title": "MR title",
+ },
+ retry_transient_errors=True,
+ )
+
+ assert len(fake_responses.calls) == 4
+ assert [c.request.method for c in fake_responses.calls] == expected_http_methods
+ assert [c.request.url for c in fake_responses.calls] == expected_urls
--
GitLab