diff --git a/conftest.py b/conftest.py
index ae717c5d..8bf8a31a 100644
--- a/conftest.py
+++ b/conftest.py
@@ -385,6 +385,10 @@ class Repo:
# unwatch repo
self.unsubscribe()
+ @property
+ def owner(self):
+ return self.name.split('/')[0]
+
def unsubscribe(self, token=None):
self._get_session(token).put('https://api.github.com/repos/{}/subscription'.format(self.name), json={
'subscribed': False,
@@ -417,7 +421,27 @@ class Repo:
assert 200 <= r.status_code < 300, r.json()
def get_ref(self, ref):
- return self.commit(ref).id
+ # differs from .commit(ref).id for the sake of assertion error messages
+ # apparently commits/{ref} returns 422 or some other fool thing when the
+ # ref' does not exist which sucks for asserting "the ref' has been
+ # deleted"
+ # FIXME: avoid calling get_ref on a hash & remove this code
+ if re.match(r'[0-9a-f]{40}', ref):
+ # just check that the commit exists
+ r = self._session.get('https://api.github.com/repos/{}/git/commits/{}'.format(self.name, ref))
+ assert 200 <= r.status_code < 300, r.reason
+ return r.json()['sha']
+
+ if ref.startswith('refs/'):
+ ref = ref[5:]
+ if not ref.startswith('heads'):
+ ref = 'heads/' + ref
+
+ r = self._session.get('https://api.github.com/repos/{}/git/ref/{}'.format(self.name, ref))
+ assert 200 <= r.status_code < 300, r.reason
+ res = r.json()
+ assert res['object']['type'] == 'commit'
+ return res['object']['sha']
def commit(self, ref):
if not re.match(r'[0-9a-f]{40}', ref):
@@ -739,7 +763,7 @@ class PR:
@property
def ref(self):
- return 'heads/' + self.branch[1]
+ return 'heads/' + self.branch.branch
def post_comment(self, body, token=None):
assert self.repo.hook
diff --git a/forwardport/data/crons.xml b/forwardport/data/crons.xml
index 39ce9bb8..02d2be1e 100644
--- a/forwardport/data/crons.xml
+++ b/forwardport/data/crons.xml
@@ -31,4 +31,15 @@
-1
+
+
+ Remove branches of merged PRs
+
+ code
+ model._process()
+ 1
+ hours
+ -1
+
+
diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py
index ced89b7c..50629877 100644
--- a/forwardport/models/forwardport.py
+++ b/forwardport/models/forwardport.py
@@ -1,18 +1,26 @@
# -*- coding: utf-8 -*-
import logging
from contextlib import ExitStack
+from datetime import datetime
+
+from dateutil import relativedelta
from odoo import fields, models
+from odoo.addons.runbot_merge.github import GH
+# how long a merged PR survives
+MERGE_AGE = relativedelta.relativedelta(weeks=2)
_logger = logging.getLogger(__name__)
class Queue:
+ limit = 100
+
def _process_item(self):
raise NotImplementedError
def _process(self):
- for b in self.search([]):
+ for b in self.search(self._search_domain(), order='create_date, id', limit=self.limit):
try:
b._process_item()
b.unlink()
@@ -22,10 +30,15 @@ class Queue:
self.env.cr.rollback()
self.clear_caches()
+ def _search_domain(self):
+ return []
+
class BatchQueue(models.Model, Queue):
_name = 'forwardport.batches'
_description = 'batches which got merged and are candidates for forward-porting'
+ limit = 10
+
batch_id = fields.Many2one('runbot_merge.batch', required=True)
source = fields.Selection([
('merge', 'Merge'),
@@ -56,6 +69,8 @@ class UpdateQueue(models.Model, Queue):
_name = 'forwardport.updates'
_description = 'if a forward-port PR gets updated & has followups (cherrypick succeeded) the followups need to be updated as well'
+ limit = 10
+
original_root = fields.Many2one('runbot_merge.pull_requests')
new_root = fields.Many2one('runbot_merge.pull_requests')
@@ -81,3 +96,63 @@ class UpdateQueue(models.Model, Queue):
self.env.cr.commit()
previous = child
+
+_deleter = _logger.getChild('deleter')
+class DeleteBranches(models.Model, Queue):
+ _name = 'forwardport.branch_remover'
+ _description = "Removes branches of merged PRs"
+
+ pr_id = fields.Many2one('runbot_merge.pull_requests')
+
+ def _search_domain(self):
+ cutoff = self.env.context.get('forwardport_merged_before') \
+ or fields.Datetime.to_string(datetime.now() - MERGE_AGE)
+ return [('pr_id.write_date', '<', cutoff)]
+
+ def _process_item(self):
+ _deleter.info(
+ "PR %s: checking deletion of linked branch %s",
+ self.pr_id.display_name,
+ self.pr_id.label
+ )
+
+ if self.pr_id.state != 'merged':
+ _deleter.info('✘ PR is not "merged" (got %s)', self.pr_id.state)
+ return
+
+ repository = self.pr_id.repository
+ fp_remote = repository.fp_remote_target
+ if not fp_remote:
+ _deleter.info('✘ no forward-port target')
+ return
+
+ repo_owner, repo_name = fp_remote.split('/')
+ owner, branch = self.pr_id.label.split(':')
+ if repo_owner != owner:
+ _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)
+ if ref.status_code != 200:
+ _deleter.info("✘ branch already deleted (%s)", ref.json())
+ return
+
+ ref = ref.json()
+ if ref['object']['sha'] != self.pr_id.head:
+ _deleter.info(
+ "✘ branch %s head mismatch, expected %s, got %s",
+ self.pr_id.label,
+ self.pr_id.head,
+ ref['object']['sha']
+ )
+ return
+
+ r = github('delete', refurl)
+ 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)
diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 7e9c5769..203c7fd0 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -202,6 +202,11 @@ class PullRequests(models.Model):
"It should be merged the normal way (via @%s)" % p.repository.project_id.github_prefix,
'token_field': 'fp_github_token',
})
+ if vals.get('state') == 'merged':
+ for p in self:
+ self.env['forwardport.branch_remover'].create({
+ 'pr_id': p.id,
+ })
return r
def _try_closing(self, by):
diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index d04b0358..2013db93 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
import collections
-from datetime import datetime, timedelta
import time
+from datetime import datetime, timedelta
from operator import itemgetter
import pytest
@@ -203,6 +203,22 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
# check that we didn't just smash the original trees
assert prod.read_tree(prod.commit('a')) != b_tree != c_tree
+ prs = env['forwardport.branch_remover'].search([]).mapped('pr_id')
+ assert prs == pr0 | pr1 | pr2, "pr1 and pr2 should be slated for branch deletion"
+ env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
+
+ # should not have deleted the base branch (wrong repo)
+ assert other_user_repo.get_ref(pr.ref) == p_1
+
+ # should have deleted all PR branches
+ pr1_ref = prod.get_pr(pr1.number).ref
+ with pytest.raises(AssertionError, match='Not Found'):
+ other.get_ref(pr1_ref)
+
+ pr2_ref = prod.get_pr(pr2.number).ref
+ with pytest.raises(AssertionError, match="Not Found"):
+ other.get_ref(pr2_ref)
+
def test_update_pr(env, config, make_repo, users):
""" Even for successful cherrypicks, it's possible that e.g. CI doesn't
pass or the reviewer finds out they need to update the code.
@@ -964,6 +980,86 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
assert not pr1_1.parent_id
assert pr2_1.state == 'opened'
+class TestBranchDeletion:
+ def test_delete_normal(self, env, config, make_repo):
+ """ Regular PRs should get their branch deleted as long as they're
+ created in the fp repository
+ """
+ prod, other = make_basic(env, config, make_repo)
+ with prod, other:
+ [c] = other.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/abranch')
+ pr = prod.make_pr(
+ target='a', head='%s:abranch' % other.owner,
+ title="a pr",
+ )
+ prod.post_status(c, 'success', 'legal/cla')
+ prod.post_status(c, 'success', 'ci/runbot')
+ pr.post_comment('hansen r+', config['role_reviewer']['token'])
+ env.run_crons()
+
+ with prod:
+ prod.post_status('staging.a', 'success', 'legal/cla')
+ prod.post_status('staging.a', 'success', 'ci/runbot')
+ env.run_crons()
+
+ pr_id = env['runbot_merge.pull_requests'].search([
+ ('repository.name', '=', prod.name),
+ ('number', '=', pr.number)
+ ])
+ assert pr_id.state == 'merged'
+ removers = env['forwardport.branch_remover'].search([])
+ to_delete_branch = removers.mapped('pr_id')
+ assert to_delete_branch == pr_id
+
+ env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
+ with pytest.raises(AssertionError, match="Not Found"):
+ other.get_ref('heads/abranch')
+
+ def test_not_merged(self, env, config, make_repo):
+ """ The branches of PRs which are still open or have been closed (rather
+ than merged) should not get deleted
+ """
+ prod, other = make_basic(env, config, make_repo)
+ with prod, other:
+ [c] = other.make_commits('a', Commit('c1', tree={'1': '0'}), ref='heads/abranch')
+ pr1 = prod.make_pr(target='a', head='%s:abranch' % other.owner, title='a')
+ prod.post_status(c, 'success', 'legal/cla')
+ prod.post_status(c, 'success', 'ci/runbot')
+ pr1.post_comment('hansen r+', config['role_reviewer']['token'])
+
+ [c] = other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch')
+ pr2 = prod.make_pr(target='a', head='%s:bbranch' % other.owner, title='b')
+ pr2.close()
+
+ [c] = other.make_commits('a', Commit('c3', tree={'3': '0'}), ref='heads/cbranch')
+ pr3 = prod.make_pr(target='a', head='%s:cbranch' % other.owner, title='c')
+ prod.post_status(c, 'success', 'legal/cla')
+ prod.post_status(c, 'success', 'ci/runbot')
+
+ other.make_commits('a', Commit('c3', tree={'4': '0'}), ref='heads/dbranch')
+ pr4 = prod.make_pr(target='a', head='%s:dbranch' % other.owner, title='d')
+ pr4.post_comment('hansen r+', config['role_reviewer']['token'])
+ env.run_crons()
+
+ PR = env['runbot_merge.pull_requests']
+ # check PRs are in states we expect
+ pr_heads = []
+ for p, st in [(pr1, 'ready'), (pr2, 'closed'), (pr3, 'validated'), (pr4, 'approved')]:
+ p_id = PR.search([
+ ('repository.name', '=', prod.name),
+ ('number', '=', p.number),
+ ])
+ assert p_id.state == st
+ pr_heads.append(p_id.head)
+
+ env.run_crons('forwardport.remover', context={'forwardport_merged_before': FAKE_PREV_WEEK})
+
+ # check that the branches still exist
+ assert other.get_ref('heads/abranch') == pr_heads[0]
+ assert other.get_ref('heads/bbranch') == pr_heads[1]
+ assert other.get_ref('heads/cbranch') == pr_heads[2]
+ assert other.get_ref('heads/dbranch') == pr_heads[3]
+
def sPeNgBaB(s):
return ''.join(
l if i % 2 == 0 else l.upper()