diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 3255b945..266947f7 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -15,10 +15,9 @@ from dateutil import relativedelta from odoo import api, fields, models from odoo.addons.runbot_merge import git -from odoo.addons.runbot_merge.github import GH # how long a merged PR survives -MERGE_AGE = relativedelta.relativedelta(weeks=2) +MERGE_AGE = relativedelta.relativedelta(weeks=1) FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n' _logger = logging.getLogger(__name__) @@ -379,7 +378,7 @@ class DeleteBranches(models.Model, Queue): _name = 'forwardport.branch_remover' _description = "Removes branches of merged PRs" - pr_id = fields.Many2one('runbot_merge.pull_requests') + pr_id = fields.Many2one('runbot_merge.pull_requests', index=True) @api.model_create_multi def create(self, vals_list): @@ -389,7 +388,11 @@ class DeleteBranches(models.Model, Queue): def _search_domain(self): cutoff = getattr(builtins, 'forwardport_merged_before', None) \ or fields.Datetime.to_string(datetime.now() - MERGE_AGE) - return [('pr_id.merge_date', '<', cutoff)] + return [ + '|', ('pr_id.merge_date', '<', cutoff), + '&', ('pr_id.closed', '=', True), + ('pr_id.write_date', '<', cutoff), + ] def _process_item(self): _deleter.info( @@ -398,8 +401,8 @@ class DeleteBranches(models.Model, Queue): self.pr_id.label ) - if self.pr_id.state != 'merged': - _deleter.info('✘ PR is not "merged" (got %s)', self.pr_id.state) + if self.pr_id.state not in ('merged', 'closed'): + _deleter.info('✘ PR is active (%s)', self.pr_id.state) return repository = self.pr_id.repository @@ -414,34 +417,17 @@ class DeleteBranches(models.Model, Queue): _deleter.info('✘ PR owner != FP target owner (%s)', repo_owner) return # probably don't have access to arbitrary repos - github = GH(token=repository.project_id.fp_github_token, repo=fp_remote) - refurl = 'git/refs/heads/' + branch - ref = github('get', refurl, check=False) - if ref.status_code != 200: - _deleter.info("✘ branch already deleted (%s)", ref.json()) - return - - ref = ref.json() - if isinstance(ref, list): + r = git.get_local(repository).check(False).push( + git.fw_url(repository), + '--delete', branch, + f'--force-with-lease={branch}:{self.pr_id.head}', + ) + if r.returncode: _deleter.info( - "✘ got a fuzzy match (%s), branch probably deleted", - ', '.join(r['ref'] for r in ref) - ) - return - - if ref['object']['sha'] != self.pr_id.head: - _deleter.info( - "✘ branch %s head mismatch, expected %s, got %s", + '✘ failed to delete branch %s of PR %s:\n%s', self.pr_id.label, - self.pr_id.head, - ref['object']['sha'] + self.pr_id.display_name, + r.stderr.decode(), ) - return - - r = github('delete', refurl, check=False) - assert r.status_code == 204, \ - "Tried to delete branch %s of %s, got %s" % ( - branch, self.pr_id.display_name, - r.json() - ) - _deleter.info('✔ deleted branch %s of PR %s', self.pr_id.label, self.pr_id.display_name) + else: + _deleter.info('✔ deleted branch %s of PR %s', self.pr_id.label, self.pr_id.display_name) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 52fc41a0..672fdfa8 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -1105,7 +1105,8 @@ class TestBranchDeletion: # check that the branches still exist assert other.get_ref('heads/abranch') == pr_heads[0] - assert other.get_ref('heads/bbranch') == pr_heads[1] + with pytest.raises(AssertionError, match="Not Found"): + other.get_ref('heads/bbranch') assert other.get_ref('heads/cbranch') == pr_heads[2] assert other.get_ref('heads/dbranch') == pr_heads[3] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 2340f33a..4f59068e 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1499,14 +1499,20 @@ For your own safety I've ignored *everything in your entire comment*. if merge_method not in (False, 'rebase-ff') and pr.message != vals['message']: pr.unstage("merge message updated") + # FIXME: remove condition once mergebot and forwardbot modules are merged + remover = self.env.get('forwardport.branch_remover') match vals.get('closed'): case True if not self.closed: vals['reviewed_by'] = False + if remover is not None: + remover.create([{'pr_id': self.id}]) case False if self.closed and not self.batch_id: vals['batch_id'] = self._get_batch( target=vals.get('target') or self.target.id, label=vals.get('label') or self.label, ) + if remover is not None: + remover.search([('pr_id', '=', self.id)]).unlink() w = super().write(vals) newhead = vals.get('head')