From 1b5a05e40c77c35ff0710351f644b872fd3a6ac3 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 20 Nov 2019 14:57:40 +0100 Subject: [PATCH] [FIX] mergebot: improve handling of having missed PR updates 1. if we try to stage a PR and realize we'd stored / checked the wrong head, cancel the staging and notify the PR 2. provide a command to forcefully update pr heads (or at least check that a PR's head is up to date) Closes #241 --- runbot_merge/exceptions.py | 2 + runbot_merge/models/pull_requests.py | 60 +++++++++++++++++++++++--- runbot_merge/tests/conftest.py | 6 +-- runbot_merge/tests/test_basic.py | 64 ++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 11 deletions(-) diff --git a/runbot_merge/exceptions.py b/runbot_merge/exceptions.py index 81d48533..4ee61061 100644 --- a/runbot_merge/exceptions.py +++ b/runbot_merge/exceptions.py @@ -2,3 +2,5 @@ class MergeError(Exception): pass class FastForwardError(Exception): pass +class Skip(MergeError): + pass diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 7205093d..58a7bdcd 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -239,6 +239,24 @@ class Repository(models.Model): }) return + # if the PR is already loaded, check... if the heads match? + pr_id = self.env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr['base']['repo']['full_name']), + ('number', '=', pr['number']), + ]) + if pr_id: + # TODO: edited, maybe (requires crafting a 'changes' object) + r = controllers.handle_pr(self.env, { + 'action': 'synchronize', + 'pull_request': pr, + }) + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr_id.repository.id, + 'pull_request': self.number, + 'message': r, + }) + return + controllers.handle_pr(self.env, { 'action': 'opened', 'pull_request': pr, @@ -641,8 +659,8 @@ class PullRequests(models.Model): commandstring, ): name, flag, param = m.groups() - if name == 'retry': - yield ('retry', None) + if name in ('retry', 'check'): + yield (name, None) elif name in ('r', 'review'): if flag == '+': yield ('review', True) @@ -736,6 +754,12 @@ class PullRequests(models.Model): self.state = 'ready' else: msg = "Retry makes no sense when the PR is not in error." + elif command == 'check': + if is_author: + self.env['runbot_merge.fetch_job'].create({ + 'repository': self.repository.id, + 'number': self.number, + }) elif command == 'review': if param and is_reviewer: newstate = RPLUS.get(self.state) @@ -1074,6 +1098,9 @@ class PullRequests(models.Model): "rebasing a PR of more than 50 commits is a tad excessive" assert commits < 250, "merging PRs of 250+ commits is not supported (https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)" pr_commits = gh.commits(self.number) + pr_head = pr_commits[-1]['sha'] + if pr_head != self.head: + raise exceptions.Skip(self.head, pr_head, commits == 1) if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login: # XXX: find other trigger(s) to sync github name? @@ -1716,12 +1743,31 @@ class Batch(models.Model): original_head, new_heads[pr] ) except (exceptions.MergeError, AssertionError) as e: - _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e) - pr.state = 'error' + if isinstance(e, exceptions.Skip): + old_head, new_head, to_squash = e.args + pr.write({ + 'state': 'opened', + 'squash': to_squash, + 'head': new_head, + }) + _logger.warning( + "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 + 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': "Unable to stage PR (%s)" % e, + 'message': msg, }) # reset the head which failed, as rebase() may have partially @@ -1746,8 +1792,8 @@ class FetchJob(models.Model): _name = 'runbot_merge.fetch_job' active = fields.Boolean(default=True) - repository = fields.Many2one('runbot_merge.repository', index=True, required=True) - number = fields.Integer(index=True, required=True) + repository = fields.Many2one('runbot_merge.repository', required=True) + number = fields.Integer(required=True) # The commit (and PR) statuses was originally a map of ``{context:state}`` # however it turns out to clarify error messages it'd be useful to have diff --git a/runbot_merge/tests/conftest.py b/runbot_merge/tests/conftest.py index 2a139056..c1bab557 100644 --- a/runbot_merge/tests/conftest.py +++ b/runbot_merge/tests/conftest.py @@ -14,13 +14,11 @@ def page(port): return r.content return get -# env['runbot_merge.project']._check_fetch() -# runbot_merge.fetch_prs_cron @pytest.fixture def default_crons(): return [ # env['runbot_merge.project']._check_fetch() - # 'runbot_merge.fetch_prs_cron', + 'runbot_merge.fetch_prs_cron', # env['runbot_merge.commit']._notify() 'runbot_merge.process_updated_commits', # env['runbot_merge.project']._check_progress() @@ -39,4 +37,4 @@ def project(env, config): 'github_prefix': 'hansen', 'branch_ids': [(0, 0, {'name': 'master'})], 'required_statuses': 'legal/cla,ci/runbot', - }) \ No newline at end of file + }) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 41556fd0..48bef128 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1959,6 +1959,70 @@ class TestPRUpdate(object): assert pr.head == c2 assert pr.state == 'validated' + def test_update_missed(self, env, repo, config): + """ Sometimes github's webhooks don't trigger properly, a branch's HEAD + does not get updated and we might e.g. attempt to merge a PR despite it + now being unreviewed or failing CI or somesuch. + + This is not a super frequent occurrence, and possibly not the most + problematic issue ever (e.g. if the branch doesn't CI it's not going to + pass staging, though we might still be staging a branch which had been + unreviewed). + + So during the staging process, the heads should be checked, and the PR + will not be staged if the heads don't match (though it'll be reset to + open, rather than put in an error state as technically there's no + failure, we just want to notify users that something went odd with the + mergebot). + + TODO: other cases / situations where we want to update the head? + """ + with repo: + repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master') + + [c] = repo.make_commits( + 'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch') + pr = repo.make_pr(target='master', head='abranch') + repo.post_status(pr.head, 'success', 'legal/cla') + repo.post_status(pr.head, 'success', 'ci/runbot') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr.number), + ]) + env.run_crons('runbot_merge.process_updated_commits') + assert pr_id.state == 'ready' + + # TODO: find way to somehow skip / ignore the update_ref? + with repo: + # can't push a second commit because then the staging crashes due + # to the PR *actually* having more than 1 commit and thus needing + # a configuration + [c2] = repo.make_commits('heads/master', repo.Commit('c2', tree={'a': '2'})) + repo.post_status(c2, 'success', 'legal/cla') + repo.post_status(c2, 'success', 'ci/runbot') + repo.update_ref(pr.ref, c2, force=True) + + # we missed the update notification so the db should still be at c and + # in a "ready" state + pr_id.write({ + 'head': c, + 'state': 'ready', + }) + + env.run_crons() + + # the PR should not get merged, and should be updated + assert pr_id.state == 'validated' + assert pr_id.head == c2 + + pr_id.write({'head': c, 'state': 'ready'}) + with repo: + pr.post_comment('hansen check') + env.run_crons() + assert pr_id.state == 'validated' + assert pr_id.head == c2 + class TestBatching(object): def _pr(self, repo, prefix, trees, *, target='master', user, reviewer, statuses=(('ci/runbot', 'success'), ('legal/cla', 'success'))