diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 218df00a..0a431dd9 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -237,7 +237,6 @@ class StatusConfiguration(models.Model): context = fields.Char(required=True) repo_id = fields.Many2one('runbot_merge.repository', required=True, ondelete='cascade') branch_filter = fields.Char(help="branches this status applies to") - # FIXME: migrate branch_ids -> branch_filter = [('id', 'in', branch_ids.ids)] prs = fields.Boolean(string="Applies to pull requests", default=True) stagings = fields.Boolean(string="Applies to stagings", default=True) @@ -484,10 +483,25 @@ class Branch(models.Model): gh.set_ref('tmp.{}'.format(self.name), it['head']) batch_limit = self.project_id.batch_limit + first = True for batch in batched_prs: if len(staged) >= batch_limit: break - staged |= Batch.stage(meta, batch) + try: + staged |= Batch.stage(meta, batch) + except exceptions.MergeError as e: + if first: + pr = e.args[0] + _logger.exception("Failed to merge %s into staging branch", + pr.display_name) + pr.state = 'error' + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'message': "Unable to stage PR (%s)" % e.__context__, + }) + else: + first = False if not staged: return @@ -1925,6 +1939,14 @@ class Batch(models.Model): original_head, new_heads[pr] ) except (exceptions.MergeError, AssertionError) as e: + # reset the head which failed, as rebase() may have partially + # updated it (despite later steps failing) + gh.set_ref(target, original_head) + # then reset every previous update + for to_revert in new_heads.keys(): + it = meta[to_revert.repository] + it['gh'].set_ref('tmp.{}'.format(to_revert.target.name), it['head']) + if isinstance(e, exceptions.Skip): old_head, new_head, to_squash = e.args pr.write({ @@ -1936,31 +1958,18 @@ class Batch(models.Model): "head mismatch on %s: had %s but found %s", pr.display_name, old_head, new_head ) - msg = "We apparently missed an update to this PR and" \ - " tried to stage it in a state which might not have" \ - " been approved. PR has been updated to %s, please" \ - " check and approve or re-approve." % new_head + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'message': "We apparently missed an update to this PR " + "and tried to stage it in a state which " + "might not have been approved. PR has been " + "updated to %s, please check and approve or " + "re-approve." % new_head + }) + return self.env['runbot_merge.batch'] else: - _logger.exception("Failed to merge %s into staging branch (error: %s)", - pr.display_name, e) - pr.state = 'error' - msg = "Unable to stage PR (%s)" % e - - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': msg, - }) - - # reset the head which failed, as rebase() may have partially - # updated it (despite later steps failing) - gh.set_ref(target, original_head) - # then reset every previous update - for to_revert in new_heads.keys(): - it = meta[to_revert.repository] - it['gh'].set_ref('tmp.{}'.format(to_revert.target.name), it['head']) - - return self.env['runbot_merge.batch'] + raise exceptions.MergeError(pr) # update meta to new heads for pr, head in new_heads.items(): diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index c179a4ab..235dc248 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -11,6 +11,8 @@ from lxml import html import odoo from test_utils import re_matches, get_partner, _simple_init +from utils import Commit + @pytest.fixture def repo(env, project, make_repo, users, setreviewers): @@ -322,7 +324,7 @@ class TestWebhookSecurity: ('number', '=', pr0.number), ]) -def test_staging_conflict(env, repo, config): +def test_staging_ongoing(env, repo, config): with repo: # create base branch m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) @@ -411,8 +413,9 @@ def test_staging_concurrent(env, repo, config): ]) assert pr2.staging_id -def test_staging_merge_fail(env, repo, users, config): - """ # of staging failure (no CI) before mark & notify? +def test_staging_confict_first(env, repo, users, config): + """ If the first batch of a staging triggers a conflict, the PR should be + marked as in error """ with repo: m1 = repo.make_commit(None, 'initial', None, tree={'f': 'm1'}) @@ -439,6 +442,52 @@ def test_staging_merge_fail(env, repo, users, config): (users['user'], re_matches('^Unable to stage PR')), ] +def test_staging_conflict_second(env, repo, users, config): + """ If the non-first batch of a staging triggers a conflict, the PR should + just be skipped: it might be a conflict with an other PR which could fail + the staging + """ + with repo: + [m] = repo.make_commits(None, Commit('initial', tree={'a': '1'}), ref='heads/master') + + with repo: + repo.make_commits(m, Commit('first pr', tree={'a': '2'}), ref='heads/pr0') + pr0 = repo.make_pr(target='master', head='pr0') + repo.post_status(pr0.head, 'success', 'ci/runbot') + repo.post_status(pr0.head, 'success', 'legal/cla') + pr0.post_comment('hansen r+', config['role_reviewer']['token']) + + with repo: + repo.make_commits(m, Commit('second pr', tree={'a': '3'}), ref='heads/pr1') + pr1 = repo.make_pr(target='master', head='pr1') + repo.post_status(pr1.head, 'success', 'ci/runbot') + repo.post_status(pr1.head, 'success', 'legal/cla') + pr1.post_comment('hansen r+', config['role_reviewer']['token']) + + env.run_crons() + PRs = env['runbot_merge.pull_requests'] + pr0_id = PRs.search([ + ('repository.name', '=', repo.name), + ('number', '=', pr0.number), + ]) + pr1_id = PRs.search([ + ('repository.name', '=', repo.name), + ('number', '=', pr1.number), + ]) + assert pr0_id.staging_id, "pr0 should have been staged" + assert not pr1_id.staging_id, "pr1 should not have been staged (due to conflict)" + assert pr1_id.state == 'ready', "pr1 should not be in error yet" + + # merge the staging, this should try to stage pr1, fail, and put it in error + # as it now conflicts with the master proper + with repo: + repo.post_status('staging.master', 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + env.run_crons() + + assert pr1_id.state == 'error', "now pr1 should be in error" + + def test_staging_ci_timeout(env, repo, config): """If a staging timeouts (~ delay since staged greater than configured)... requeue?