diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 35c4e542..afbe92d6 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -257,7 +257,7 @@ def handle_pr(env, event): pr_obj = env['runbot_merge.pull_requests']._from_gh(pr) return "Tracking PR as {}".format(pr_obj.id) - pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number']) + pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number'], closing=event['action'] == 'closed') if not pr_obj: _logger.info("webhook %s on unknown PR %s#%s, scheduled fetch", event['action'], repo.name, pr['number']) return "Unknown PR {}:{}, scheduling fetch".format(repo.name, pr['number']) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 96581736..1b7a120b 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -96,28 +96,30 @@ All substitutions are tentatively applied sequentially to the input. self._cr, 'runbot_merge_unique_repo', self._table, ['name']) return res - def _load_pr(self, number): + def _load_pr(self, number, *, closing=False): gh = self.github() # fetch PR object and handle as *opened* issue, pr = gh.pr(number) + repo_name = pr['base']['repo']['full_name'] if not self.project_id._has_branch(pr['base']['ref']): - _logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring", - number, self.name, pr['base']['ref']) - self.env.ref('runbot_merge.pr.load.unmanaged')._send( - repository=self, - pull_request=number, - format_args = { - 'pr': pr, - 'repository': self, - }, - ) + _logger.info("Tasked with loading %s PR %s#%d for un-managed branch %s:%s, ignoring", + pr['state'], repo_name, number, self.name, pr['base']['ref']) + if not closing: + self.env.ref('runbot_merge.pr.load.unmanaged')._send( + repository=self, + pull_request=number, + format_args = { + 'pr': pr, + 'repository': self, + }, + ) return # if the PR is already loaded, force sync a few attributes pr_id = self.env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', pr['base']['repo']['full_name']), + ('repository.name', '=', repo_name), ('number', '=', number), ]) if pr_id: @@ -143,6 +145,11 @@ All substitutions are tentatively applied sequentially to the input. 'pull_request': pr, 'sender': {'login': self.project_id.github_prefix} }) + '. ' + if pr_id.state != 'closed' and 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 + pr_id.state = 'closed' self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr_id.repository.id, 'pull_request': number, @@ -150,6 +157,11 @@ All substitutions are tentatively applied sequentially to the input. }) return + # special case for closed PRs, just ignore all events and skip feedback + if closing: + self.env['runbot_merge.pull_requests']._from_gh(pr) + return + sender = {'login': self.project_id.github_prefix} # init the PR to the null commit so we can later synchronise it back # back to the "proper" head while resetting reviews @@ -199,7 +211,7 @@ All substitutions are tentatively applied sequentially to the input. 'sender': sender, }) pr_id = self.env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', pr['base']['repo']['full_name']), + ('repository.name', '=', repo_name), ('number', '=', number), ]) if pr['state'] == 'closed': @@ -520,7 +532,7 @@ class PullRequests(models.Model): for r in self: r.batch_id = r.batch_ids.filtered(lambda b: b.active)[:1] - def _get_or_schedule(self, repo_name, number, target=None): + def _get_or_schedule(self, repo_name, number, *, target=None, closing=False): repo = self.env['runbot_merge.repository'].search([('name', '=', repo_name)]) if not repo: return @@ -546,6 +558,7 @@ class PullRequests(models.Model): Fetch.create({ 'repository': repo.id, 'number': number, + 'closing': closing, }) def _parse_command(self, commandstring): @@ -1823,6 +1836,7 @@ class FetchJob(models.Model): active = fields.Boolean(default=True) repository = fields.Many2one('runbot_merge.repository', required=True) number = fields.Integer(required=True, group_operator=None) + closing = fields.Boolean(default=False) def _check(self, commit=False): """ @@ -1835,7 +1849,7 @@ class FetchJob(models.Model): self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch") try: - f.repository._load_pr(f.number) + f.repository._load_pr(f.number, closing=f.closing) except Exception: self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch") _logger.exception("Failed to load pr %s, skipping it", f.number) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 03dda69c..1244bda2 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -3218,6 +3218,74 @@ class TestUnknownPR: "as I didn't see previous commands."), ] + def test_close_unknown_unmanaged(self, env, repo, users, config): + """If an "unknown PR" is *closed*, it should be saved as closed but not + commented on, because that's unnecessary spam. + """ + 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() + assert pr.comments == [seen(env, pr, users)] + + to_pr(env, pr).unlink() + env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink() + + with repo: + pr.close() + + 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)] + + + def test_close_unknown_disabled(self, env, repo, users, config): + """If an "unknown PR" on an disabled branch is *closed*, it should be + saved as closed but not commented on, because that's unnecessary spam. + """ + 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() + assert pr.comments == [seen(env, pr, users)] + + to_pr(env, pr).unlink() + env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink() + env['runbot_merge.branch'].search([('name', '=', 'master')]).active = False + + with repo: + pr.close() + + 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)] + def test_rplus_unmanaged(self, env, repo, users, config): """ r+ on an unmanaged target should notify about """