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},