From 46c460fd9bb22e50248c90ae8cdd673f2caf11df Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 29 Oct 2018 09:42:26 +0100 Subject: [PATCH] [IMP] runbot_merge: send comments asynchronously via feedback table Hopefully this finally fixes the double commenting issue (e.g. odoo/odoo#28160). This seems (according to reading the logs and also logic) like a failure (concurrency of some sort) leading to a transaction rollback *after* the GH API call, so the cron's DB actions are rollbacked (then performed again on the next run) *but* the GH API interactions are not rollbacked, and are thus performed twice. Since the entire purpose of the feedback table is to send comments, send both "staging failed" and "Linked pull requests" comments via that. --- runbot_merge/models/pull_requests.py | 15 ++++++++++----- runbot_merge/tests/test_basic.py | 4 ++-- runbot_merge/tests/test_multirepo.py | 11 +++++------ runbot_merge/tests/test_utils.py | 6 ++++++ 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index ca8193b0..57f3f20a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -758,14 +758,16 @@ class PullRequests(models.Model): unready = prs - ready for r in ready: - r.repository.github().comment( - r.number, "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': r.repository.id, + 'pull_request': r.number, + 'message': "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( ', '.join(map( '{0.repository.name}#{0.number}'.format, unready )) ) - ) + }) r.link_warned = True if commit: self.env.cr.commit() @@ -992,8 +994,11 @@ class Stagings(models.Model): prs = prs or self.batch_ids.prs prs.write({'state': 'error'}) for pr in prs: - pr.repository.github().comment( - pr.number, "Staging failed: %s" % message) + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'message':"Staging failed: %s" % message + }) self.batch_ids.write({'active': False}) self.write({ diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index cf0a5af9..fbd0d57e 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -10,7 +10,7 @@ from lxml import html import odoo -from test_utils import re_matches +from test_utils import re_matches, run_crons @pytest.fixture def repo(make_repo): @@ -387,7 +387,7 @@ def test_staging_ci_failure_single(env, repo, users): staging_head = repo.commit('heads/staging.master') repo.post_status(staging_head.id, 'success', 'legal/cla') repo.post_status(staging_head.id, 'failure', 'ci/runbot') # stable genius - env['runbot_merge.project']._check_progress() + run_crons(env) assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index fdae3d8a..10e04561 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -9,7 +9,7 @@ import json import pytest -from test_utils import re_matches +from test_utils import re_matches, run_crons @pytest.fixture def repo_a(make_repo): @@ -263,13 +263,12 @@ class TestCompanionsNotReady: assert pr_b.state == 'validated' assert pr_a.label == pr_b.label == '{}:do-a-thing'.format(owner) - env['runbot_merge.project']._check_progress() + run_crons(env) assert not pr_b.staging_id assert not pr_a.staging_id, \ "pr_a should not have been staged as companion is not ready" - env['runbot_merge.pull_requests']._check_linked_prs_statuses() assert p_a.comments == [ (users['reviewer'], 'hansen r+'), (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), @@ -296,7 +295,7 @@ class TestCompanionsNotReady: make_branch(repo_c, 'master', 'initial', {'f': 'c0'}) pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing', reviewer=None) - env['runbot_merge.pull_requests']._check_linked_prs_statuses() + run_crons(env) assert pr_a.comments == [] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), @@ -321,7 +320,7 @@ class TestCompanionsNotReady: make_branch(repo_c, 'master', 'initial', {'f': 'c0'}) pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing') - env['runbot_merge.pull_requests']._check_linked_prs_statuses() + run_crons(env) assert pr_a.comments == [] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), @@ -356,7 +355,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users): repo_a.post_status('heads/staging.master', 'success', 'ci/runbot', target_url="http://example.org/a") repo_b.post_status('heads/staging.master', 'success', 'legal/cla') repo_b.post_status('heads/staging.master', 'failure', 'ci/runbot', target_url="http://example.org/b") - env['runbot_merge.project']._check_progress() + run_crons(env) sth = repo_b.commit('heads/staging.master').id assert not pr.staging_id diff --git a/runbot_merge/tests/test_utils.py b/runbot_merge/tests/test_utils.py index 707dbff7..a0a31121 100644 --- a/runbot_merge/tests/test_utils.py +++ b/runbot_merge/tests/test_utils.py @@ -11,3 +11,9 @@ class re_matches: def __repr__(self): return '~' + self._r.pattern + '~' + +def run_crons(env): + "Helper to run all crons (in a relevant order) except for the fetch PR one" + env['runbot_merge.project']._check_progress() + env['runbot_merge.pull_requests']._check_linked_prs_statuses() + env['runbot_merge.project']._send_feedback()