From 8abf1be2789f27b87278f5276abeb385f07f0274 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 10 Nov 2020 16:13:08 +0100 Subject: [PATCH] [FIX] runbot_merge: cancel approval (r+) when fetching PRs When retrieving unknown PRs, the process would apply all comments, thereby applying eventual r+ without taking in account their relationship to a force push. This means it was possible for a mergebot-unknown PR to be r+'d, updated, retargeted, and the mergetbot would consider it good to go. The possible damage would be somewhat limited but still, not great. Sadly Github simply doesn't provide access to the entire event stream of the PR, so there is no way to even know whether the PR was updated, let alone when in relation to comments. Therefore just resync the PR after having applied comments: we still want to apply the merge method & al, we just want to reset back to un-approved. An other minor fix (for something we never actually hit but could): reviews are treated more or less as comments, but separate at github's level. The job would apply all comments then all reviews, so the relative order of comments and reviews would be wrong. Combine and order comments and reviews so they are applied in (hopefully) the correct order of their creation / submission. Closes #416 --- runbot_merge/models/pull_requests.py | 69 +++++++++++++++++++--------- runbot_merge/tests/test_basic.py | 28 +++++++---- 2 files changed, 67 insertions(+), 30 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 38c71db5..37890cf3 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -304,10 +304,11 @@ All substitutions are tentatively applied sequentially to the input. # fetch PR object and handle as *opened* issue, pr = gh.pr(number) + feedback = self.env['runbot_merge.pull_requests.feedback'].create if not self.project_id._has_branch(pr['base']['ref']): _logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring", - pr['number'], self.name, pr['base']['ref']) - self.env['runbot_merge.pull_requests.feedback'].create({ + number, self.name, pr['base']['ref']) + feedback({ 'repository': self.id, 'pull_request': number, 'message': "I'm sorry. Branch `{}` is not within my remit.".format(pr['base']['ref']), @@ -317,7 +318,7 @@ All substitutions are tentatively applied sequentially to the input. # 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']), + ('number', '=', number), ]) if pr_id: # TODO: edited, maybe (requires crafting a 'changes' object) @@ -325,36 +326,60 @@ All substitutions are tentatively applied sequentially to the input. 'action': 'synchronize', 'pull_request': pr, }) - self.env['runbot_merge.pull_requests.feedback'].create({ + feedback({ 'repository': pr_id.repository.id, 'pull_request': number, 'message': r, }) return + # init the PR to the null commit so we can later synchronise it back + # back to the "proper" head while resetting reviews controllers.handle_pr(self.env, { 'action': 'opened', - 'pull_request': pr, + 'pull_request': {**pr, 'head': {**pr['head'], 'sha': '0'*40}}, }) + # fetch & set up actual head for st in gh.statuses(pr['head']['sha']): controllers.handle_status(self.env, st) - # get and handle all comments - for comment in gh.comments(number): - controllers.handle_comment(self.env, { - 'action': 'created', - 'issue': issue, - 'sender': comment['user'], - 'comment': comment, - 'repository': {'full_name': self.name}, - }) - # get and handle all reviews - for review in gh.reviews(number): - controllers.handle_review(self.env, { - 'action': 'submitted', - 'review': review, - 'pull_request': pr, - 'repository': {'full_name': self.name}, - }) + # fetch and apply comments + counter = itertools.count() + items = [ # use counter so `comment` and `review` don't get hit during sort + (comment['created_at'], next(counter), False, comment) + for comment in gh.comments(number) + ] + [ + (review['submitted_at'], next(counter), True, review) + for review in gh.reviews(number) + ] + items.sort() + for _, _, is_review, item in items: + if is_review: + controllers.handle_review(self.env, { + 'action': 'submitted', + 'review': item, + 'pull_request': pr, + 'repository': {'full_name': self.name}, + }) + else: + controllers.handle_comment(self.env, { + 'action': 'created', + 'issue': issue, + 'sender': item['user'], + 'comment': item, + 'repository': {'full_name': self.name}, + }) + # sync to real head + controllers.handle_pr(self.env, { + 'action': 'synchronize', + 'pull_request': pr, + 'sender': {'login': self.project_id.github_prefix} + }) + feedback({ + 'repository': self.id, + 'pull_request': number, + 'message': "Sorry, I didn't know about this PR and had to retrieve " + "its information, you may have to re-approve it." + }) def having_branch(self, branch): branches = self.env['runbot_merge.branch'].search diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index b096641b..f4a15c08 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2725,11 +2725,8 @@ class TestUnknownPR: => instead, create PRs on the fly when getting notifications related to valid but unknown PRs - - * get statuses if head commit unknown (additional cron?) - * handle any comment & review (existing PRs may enter the system on a review/r+) """ - def test_rplus_unknown(self, repo, env, config): + def test_rplus_unknown(self, repo, env, config, users): with repo: m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'}) @@ -2751,27 +2748,42 @@ class TestUnknownPR: # reviewer reviewers with repo: prx.post_comment('hansen r+', config['role_reviewer']['token']) + with repo: + prx.post_review('REQUEST_CHANGES', 'hansen r-', config['role_reviewer']['token']) + with repo: + prx.post_comment('hansen r+', config['role_reviewer']['token']) Fetch = env['runbot_merge.fetch_job'] - assert Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)]) + fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)]) + assert len(fetches) == 1, f"expected one fetch for {prx.number}, found {len(fetches)}" + env.run_crons('runbot_merge.fetch_prs_cron') env.run_crons() assert not Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)]) c = env['runbot_merge.commit'].search([('sha', '=', prx.head)]) + print(prx.head, c, c.statuses, flush=True) assert json.loads(c.statuses) == { 'legal/cla': {'state': 'success', 'target_url': None, 'description': None}, 'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None} } + assert prx.comments == [ + (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen r+'), + (users['user'], "Sorry, I didn't know about this PR and had to " + "retrieve its information, you may have to " + "re-approve it."), + ] pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number) ]) - assert pr.state == 'ready' + assert pr.state == 'validated' - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') - assert pr.staging_id + with repo: + prx.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr.state == 'ready' def test_rplus_unmanaged(self, env, repo, users, config): """ r+ on an unmanaged target should notify about