From 747174f610bc4f6eda9f5ae332187b1297e43c2e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 30 Jul 2021 09:20:57 +0200 Subject: [PATCH] [FIX] runbot_merge: when fetching a PR, sync closed state If a PR is closed on github and unknown by the mergebot, when fetched it should be properly sync'd as "closed" in the backend, otherwise the PR can get in a weird state and cause issues. Also move the "I fetched the thing" comment before the actual creation of the PR for workflow clarity, otherwise the reader has the impression that the 'bot knew about the PR then fetched it anyway. And improve savepoint management around the fetching: savepoints should be released in all cases. Closes #488. --- runbot_merge/controllers/__init__.py | 20 +++++++----- runbot_merge/models/pull_requests.py | 31 ++++++++++++++----- runbot_merge/tests/test_basic.py | 46 ++++++++++++++++++++++++++-- runbot_merge/tests/test_multirepo.py | 1 + 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 0067de0c..70765449 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -193,16 +193,22 @@ def handle_pr(env, event): # don't marked merged PRs as closed (!!!) if event['action'] == 'closed' and pr_obj.state != 'merged': - # FIXME: store some sort of "try to close it later" if the merge fails? - _logger.info( - '%s closing %s (state=%s)', - event['sender']['login'], - pr_obj.display_name, - pr_obj.state, - ) + oldstate = pr_obj.state if pr_obj._try_closing(event['sender']['login']): + _logger.info( + '%s closed %s (state=%s)', + event['sender']['login'], + pr_obj.display_name, + oldstate, + ) return 'Closed {}'.format(pr_obj.id) else: + _logger.warning( + '%s tried to close %s (state=%s)', + event['sender']['login'], + pr_obj.display_name, + oldstate, + ) return 'Ignored: could not lock rows (probably being merged)' if event['action'] == 'reopened' : diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 28eae653..0245563e 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -210,7 +210,8 @@ class Project(models.Model): except Exception: self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch") _logger.exception("Failed to load pr %s, skipping it", f.number) - self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch") + finally: + self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch") # commit after each fetched PR f.active = False @@ -329,6 +330,7 @@ All substitutions are tentatively applied sequentially to the input. r = controllers.handle_pr(self.env, { 'action': 'synchronize', 'pull_request': pr, + 'sender': {'login': self.project_id.github_prefix} }) feedback({ 'repository': pr_id.repository.id, @@ -337,11 +339,21 @@ All substitutions are tentatively applied sequentially to the input. }) return + 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." + }) # 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, 'head': {**pr['head'], 'sha': '0'*40}}, + 'pull_request': { + **pr, + 'head': {**pr['head'], 'sha': '0'*40}, + 'state': 'open', + }, }) # fetch & set up actual head for st in gh.statuses(pr['head']['sha']): @@ -378,12 +390,14 @@ All substitutions are tentatively applied sequentially to the input. '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." - }) + if pr['state'] == 'closed': + # don't go through controller because try_closing does weird things + # for safety / race condition reasons which ends up committing + # and breaks everything + self.env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr['base']['repo']['full_name']), + ('number', '=', number), + ]).state = 'closed' def having_branch(self, branch): branches = self.env['runbot_merge.branch'].search @@ -1214,6 +1228,7 @@ class PullRequests(models.Model): if body: message += '\n\n' + body return self.env['runbot_merge.pull_requests'].create({ + 'state': 'opened' if description['state'] == 'open' else 'closed', 'number': description['number'], 'label': repo._remap_label(description['head']['label']), 'author': author.id, diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 3c3f8bec..1f0b08a2 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2894,7 +2894,6 @@ class TestUnknownPR: 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} @@ -2903,10 +2902,10 @@ class TestUnknownPR: seen(env, prx, users), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), - seen(env, prx, users), (users['user'], "Sorry, I didn't know about this PR and had to " "retrieve its information, you may have to " "re-approve it."), + seen(env, prx, users), ] pr = env['runbot_merge.pull_requests'].search([ @@ -2919,6 +2918,49 @@ class TestUnknownPR: prx.post_comment('hansen r+', config['role_reviewer']['token']) assert pr.state == 'ready' + def test_fetch_closed(self, env, repo, users, config): + """ If an "unknown PR" is fetched while closed, it should be saved as + closed + """ + with repo: + m, _ = repo.make_commits( + None, + Commit('initial', tree={'m': 'm'}), + Commit('second', tree={'m2': 'm2'}), + ref='heads/master') + + [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'})) + pr = repo.make_pr(title='title', body='body', target='master', head=c1) + env.run_crons() + with repo: + pr.close() + + # assume an unknown but ready PR: we don't know the PR or its head commit + to_pr(env, pr).unlink() + env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink() + + # reviewer reviewers + with repo: + pr.post_comment('hansen r+', config['role_reviewer']['token']) + + Fetch = env['runbot_merge.fetch_job'] + fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)]) + assert len(fetches) == 1, f"expected one fetch for {pr.number}, found {len(fetches)}" + + env.run_crons('runbot_merge.fetch_prs_cron') + env.run_crons() + assert not Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)]) + + assert to_pr(env, pr).state == 'closed' + assert pr.comments == [ + seen(env, pr, users), + (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."), + seen(env, pr, users), + ] + def test_rplus_unmanaged(self, env, repo, users, config): """ r+ on an unmanaged target should notify about """ diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 5d4f64a0..99d3f48b 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -896,6 +896,7 @@ class TestSubstitutions: 'full_name': r.name, }, 'pull_request': { + 'state': 'open', 'user': {'login': 'bob'}, 'base': { 'repo': {'full_name': r.name},