Skip to content
Snippets Groups Projects
Commit 0ecfe340 authored by Martin's avatar Martin Committed by Bastien Abadie
Browse files

retry all api calls in the same try-except to handle them in the same way

parent e19556b6
No related branches found
No related tags found
1 merge request!49support chunks in git export; rebase before merging to avoid merging manually
Pipeline #78192 passed
Stage: test
Stage: build
Hide whitespace changes
Inline Side-by-side
arkindex_worker/git.py
+ 38
− 14
View file @ 0ecfe340
...@@ -6,6 +6,7 @@ from datetime import datetime ...@@ -6,6 +6,7 @@ from datetime import datetime
from pathlib import Path from pathlib import Path
import gitlab import gitlab
import requests
import sh import sh
from arkindex_worker import logger from arkindex_worker import logger
...@@ -57,23 +58,20 @@ class GitlabHelper: ...@@ -57,23 +58,20 @@ class GitlabHelper:
:param title: title of the merge request :param title: title of the merge request
:return: was the branch successfully merged? :return: was the branch successfully merged?
""" """
logger.info(f"Creating a merge request for {branch_name}") mr = None
mr = self.project.mergerequests.create(
{
"source_branch": branch_name,
"target_branch": self.branch,
"title": title,
}
)
# always rebase first, because other workers might have merged already # always rebase first, because other workers might have merged already
for i in range(self.max_rebase_tries): for i in range(self.max_rebase_tries):
logger.info(f"Trying to merge, try nr: {i}") 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: 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) mr.merge(should_remove_source_branch=self.delete_source_branch)
logger.info("Merge successful") logger.info("Merge successful")
return True return True
...@@ -84,8 +82,30 @@ class GitlabHelper: ...@@ -84,8 +82,30 @@ class GitlabHelper:
else: else:
logger.error(f"Merge was not successful: {e}") logger.error(f"Merge was not successful: {e}")
return False 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 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): def _get_merge_request(self, merge_request_id, include_rebase_in_progress=True):
return self.project.mergerequests.get( return self.project.mergerequests.get(
merge_request_id, include_rebase_in_progress=include_rebase_in_progress merge_request_id, include_rebase_in_progress=include_rebase_in_progress
...@@ -315,7 +335,11 @@ class GitHelper: ...@@ -315,7 +335,11 @@ class GitHelper:
self._git.push("-u", "origin", "HEAD") self._git.push("-u", "origin", "HEAD")
if self.gitlab_helper: 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: else:
logger.info( logger.info(
"No gitlab_helper defined, not trying to merge the pushed branch" "No gitlab_helper defined, not trying to merge the pushed branch"
......
tests/test_git.py
+ 314
− 100
View file @ 0ecfe340
...@@ -2,9 +2,39 @@ ...@@ -2,9 +2,39 @@
from pathlib import Path from pathlib import Path
import pytest import pytest
from gitlab import GitlabCreateError, GitlabError
from requests import ConnectionError
from arkindex_worker.git import GitlabHelper 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): def test_clone_done(fake_git_helper):
assert not fake_git_helper.is_clone_finished assert not fake_git_helper.is_clone_finished
...@@ -91,23 +121,11 @@ def test_merge__rebase_failed(mocker): ...@@ -91,23 +121,11 @@ def test_merge__rebase_failed(mocker):
assert merqe_request.merge.call_count == 0 assert merqe_request.merge.call_count == 0
def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory): def test_wait_for_rebase_to_finish(fake_responses, fake_gitlab_helper_factory):
project_id = 21259233 get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
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
},
)
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,
responses.add(
responses.GET,
get_mr_url, get_mr_url,
json={ json={
"rebase_in_progress": True, "rebase_in_progress": True,
...@@ -115,8 +133,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory): ...@@ -115,8 +133,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
}, },
) )
responses.add( fake_responses.add(
responses.GET, fake_responses.GET,
get_mr_url, get_mr_url,
json={ json={
"rebase_in_progress": True, "rebase_in_progress": True,
...@@ -124,8 +142,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory): ...@@ -124,8 +142,8 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
}, },
) )
responses.add( fake_responses.add(
responses.GET, fake_responses.GET,
get_mr_url, get_mr_url,
json={ json={
"rebase_in_progress": False, "rebase_in_progress": False,
...@@ -135,129 +153,325 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory): ...@@ -135,129 +153,325 @@ def test_wait_for_rebase_to_finish(responses, fake_gitlab_helper_factory):
gitlab_helper = 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 success
assert len(responses.calls) == 4 assert len(fake_responses.calls) == 4
assert gitlab_helper.is_rebase_finished assert gitlab_helper.is_rebase_finished
def test_merge_request(responses, fake_gitlab_helper_factory, mocker): def test_wait_for_rebase_to_finish__fail_connection_error(
project_id = 21259233 fake_responses, fake_gitlab_helper_factory
merge_request_id = 7 ):
source_branch = "new_branch" get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
target_branch = "master"
mr_title = "merge request title"
responses.add( fake_responses.add(
responses.GET, fake_responses.GET,
"https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing", get_mr_url,
json={ body=ConnectionError(),
"id": project_id,
# several fields omitted
},
) )
responses.add( gitlab_helper = fake_gitlab_helper_factory()
responses.POST,
f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests", with pytest.raises(ConnectionError):
json={ gitlab_helper._wait_for_rebase_to_finish(MERGE_REQUEST_ID)
"id": 107,
"iid": merge_request_id, assert len(fake_responses.calls) == 2
"project_id": project_id, assert not gitlab_helper.is_rebase_finished
"title": mr_title,
"target_branch": target_branch,
"source_branch": source_branch, def test_wait_for_rebase_to_finish__fail_server_error(
# several fields omitted 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( gitlab_helper = fake_gitlab_helper_factory()
responses.PUT,
f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/rebase", 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={}, json={},
) )
responses.add( fake_responses.add(
responses.PUT, fake_responses.PUT,
f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/merge?should_remove_source_branch=True", f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/merge?should_remove_source_branch=True",
json={ json={
"iid": merge_request_id, "iid": MERGE_REQUEST_ID,
"state": "merged", "state": "merged",
# several fields omitted # several fields omitted
}, },
) )
# the responses are defined in the same order as they are expected to be called # the fake_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_http_methods = [r.method for r in fake_responses._matches]
expected_urls = [r.url for r in responses._matches] expected_urls = [r.url for r in fake_responses._matches]
gitlab_helper = fake_gitlab_helper_factory() gitlab_helper = fake_gitlab_helper_factory()
gitlab_helper._wait_for_rebase_to_finish = mocker.MagicMock() gitlab_helper._wait_for_rebase_to_finish = mocker.MagicMock()
gitlab_helper._wait_for_rebase_to_finish.return_value = True 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 success
assert len(responses.calls) == 4 assert len(fake_responses.calls) == 4
assert [c.request.method for c in responses.calls] == expected_http_methods assert [c.request.method for c in fake_responses.calls] == expected_http_methods
assert [c.request.url for c in responses.calls] == expected_urls assert [c.request.url for c in fake_responses.calls] == expected_urls
def test_merge_request_fail(responses, fake_gitlab_helper_factory, mocker): def test_merge_request_fail(fake_responses, fake_gitlab_helper_factory, mocker):
project_id = 21259233 fake_responses.add(
merge_request_id = 7 fake_responses.POST,
source_branch = "new_branch" f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests",
target_branch = "master" json=CREATE_MR_RESPONSE_JSON,
mr_title = "merge request title" )
responses.add( fake_responses.add(
responses.GET, fake_responses.PUT,
"https://gitlab.com/api/v4/projects/balsac_exporter%2Fbalsac-exported-xmls-testing", 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={ json={
"id": project_id, "rebase_in_progress": True,
# several fields omitted "merge_error": None,
}, },
) )
responses.add( fake_responses.add(
responses.POST, fake_responses.GET,
f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests", get_mr_url,
json={ json={
"id": 107, "rebase_in_progress": False,
"iid": merge_request_id, "merge_error": None,
"project_id": project_id, },
"title": mr_title, )
"target_branch": target_branch,
"source_branch": source_branch, 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 # several fields omitted
}, },
) )
responses.add( # the fake_responses are defined in the same order as they are expected to be called
responses.PUT, expected_http_methods = [r.method for r in fake_responses._matches]
f"https://gitlab.com/api/v4/projects/{project_id}/merge_requests/{merge_request_id}/rebase", expected_urls = [r.url for r in fake_responses._matches]
json={},
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( rebase_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}/rebase"
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(
json={"error": "Method not allowed"}, fake_responses.PUT,
status=405, rebase_url,
json={"rebase_in_progress": True},
) )
# the responses are defined in the same order as they are expected to be called get_mr_url = f"https://gitlab.com/api/v4/projects/{PROJECT_ID}/merge_requests/{MERGE_REQUEST_ID}?include_rebase_in_progress=True"
expected_http_methods = [r.method for r in responses._matches]
expected_urls = [r.url for r in responses._matches] 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 = 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(fake_responses.calls) == 4
assert len(responses.calls) == 4 assert [c.request.method for c in fake_responses.calls] == expected_http_methods
assert [c.request.method for c in responses.calls] == expected_http_methods assert [c.request.url for c in fake_responses.calls] == expected_urls
assert [c.request.url for c in 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
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment