From 4f237d15b077da08b9ab1b8b174a4b37381f6cb4 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Jun 2023 09:19:48 +0200 Subject: [PATCH 001/192] [FIX] runbot_merge: correctly check request in handle_pr 652b1ff9aea395b2bc24004e0e0741631190ba26 wanted to check if a request was available, however it deref'd the `request` object without checking it which is not correct: a `request` normally has an `httprequest`, but the `request` itself might be missing if the handler is called from e.g. a cron. Fixes #739 --- runbot_merge/controllers/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 87b84606..efb752ac 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -154,7 +154,7 @@ def handle_pr(env, event): if not branch: return "Not set up to care about {}:{}".format(r, b) - headers = request.httprequest.headers if request.httprequest else {} + headers = request.httprequest.headers if request else {} _logger.info( "%s: %s#%s (%s) (by %s, delivery %s by %s)", event['action'], From 485d2d7b55ed3c909eebad0d0dbf0690d60b2acd Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Jun 2023 09:54:06 +0200 Subject: [PATCH 002/192] [IMP] runbot_merge: add sitemap params to http controllers When it's missing, website complains because it's dumb. Fixes #763 --- runbot_merge/controllers/dashboard.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runbot_merge/controllers/dashboard.py b/runbot_merge/controllers/dashboard.py index e80a2f15..f15c52d2 100644 --- a/runbot_merge/controllers/dashboard.py +++ b/runbot_merge/controllers/dashboard.py @@ -11,13 +11,13 @@ from odoo.http import Controller, route, request LIMIT = 20 class MergebotDashboard(Controller): - @route('/runbot_merge', auth="public", type="http", website=True) + @route('/runbot_merge', auth="public", type="http", website=True, sitemap=True) def dashboard(self): return request.render('runbot_merge.dashboard', { 'projects': request.env['runbot_merge.project'].with_context(active_test=False).sudo().search([]), }) - @route('/runbot_merge/', auth='public', type='http', website=True) + @route('/runbot_merge/', auth='public', type='http', website=True, sitemap=False) def stagings(self, branch_id, until=None): branch = request.env['runbot_merge.branch'].browse(branch_id).sudo().exists() if not branch: @@ -49,7 +49,7 @@ class MergebotDashboard(Controller): entries.setdefault(key, []).extend(map(item_converter, items)) return entries - @route('/runbot_merge/changelog', auth='public', type='http', website=True) + @route('/runbot_merge/changelog', auth='public', type='http', website=True, sitemap=True) def changelog(self): md = markdown.Markdown(extensions=['nl2br'], output_format='html5') entries = self.entries(lambda t: markupsafe.Markup(md.convert(t))) @@ -57,7 +57,7 @@ class MergebotDashboard(Controller): 'entries': entries, }) - @route('///pull/', auth='public', type='http', website=True) + @route('///pull/', auth='public', type='http', website=True, sitemap=False) def pr(self, org, repo, pr): pr_id = request.env['runbot_merge.pull_requests'].sudo().search([ ('repository.name', '=', f'{org}/{repo}'), From 611f9150ff1d9e2bcdf8514642f15fecdf9ae464 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Jun 2023 11:14:47 +0200 Subject: [PATCH 003/192] [IMP] runbot_merge: add signed kw support to from_role, use it Closes #774 --- conftest.py | 2 +- runbot_merge/controllers/reviewer_provisioning.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/conftest.py b/conftest.py index c47c07a0..09ca14b7 100644 --- a/conftest.py +++ b/conftest.py @@ -355,7 +355,7 @@ def dummy_addons_path(): 'version': '1.0', }), encoding='utf-8') (mod / 'util.py').write_text("""\ -def from_role(_): +def from_role(*_, **__): return lambda fn: fn """, encoding='utf-8') diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index 1b7a9f2f..6a9b3af4 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/runbot_merge/controllers/reviewer_provisioning.py @@ -6,12 +6,12 @@ from odoo.http import Controller, request, route try: from odoo.addons.saas_worker.util import from_role except ImportError: - def from_role(_): + def from_role(*_, **__): return lambda _: None _logger = logging.getLogger(__name__) class MergebotReviewerProvisioning(Controller): - @from_role('accounts') + @from_role('accounts', signed=True) @route('/runbot_merge/users', type='json', auth='public') def list_users(self): env = request.env(su=True) @@ -23,7 +23,7 @@ class MergebotReviewerProvisioning(Controller): if u.github_login ] - @from_role('accounts') + @from_role('accounts', signed=True) @route('/runbot_merge/provision', type='json', auth='public') def provision_user(self, users): _logger.info('Provisioning %s users: %s.', len(users), ', '.join(map( @@ -107,7 +107,7 @@ class MergebotReviewerProvisioning(Controller): _logger.info("Provisioning: created %d updated %d.", created, updated) return [created, updated] - @from_role('accounts') + @from_role('accounts', signed=True) @route(['/runbot_merge/get_reviewers'], type='json', auth='public') def fetch_reviewers(self, **kwargs): reviewers = request.env['res.partner.review'].sudo().search([ @@ -115,7 +115,7 @@ class MergebotReviewerProvisioning(Controller): ]).mapped('partner_id.github_login') return reviewers - @from_role('accounts') + @from_role('accounts', signed=True) @route(['/runbot_merge/remove_reviewers'], type='json', auth='public', methods=['POST']) def update_reviewers(self, github_logins, **kwargs): partners = request.env['res.partner'].sudo().search([('github_login', 'in', github_logins)]) From 4a4252b4b912158365e3afa56aeb18d9115dd3e3 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Jun 2023 14:42:08 +0200 Subject: [PATCH 004/192] [FIX] runbot_merge: holes in provisioning - github logins are case-insensitive while the db field is CI the dict in which partners are stored for matching is not, And the caller may not preserve casing. Thus it's necessary to check the casefolded database values against casefolded parameters, rather than exactly. - users may get disabled by mistake or when one leaves the project, they may also get switched from internal to portal, therefore it is necessary to re-enable and re-enroll them if they come back. - while at it remove the user's email when they depart, as they likely use an organisational email which they don't have access to anymore Side-note, but remove the limit on the number of users / partners being created at once: because there are almost no modules in the mergebot's instance, creating partner goes quite fast (compared to a full instance), thus the limitation is almost certainly unnecessary (creating ~300 users seems to take ~450ms). Fixes ##776 --- conftest.py | 9 ++ .../controllers/reviewer_provisioning.py | 38 +++++--- runbot_merge/tests/test_provisioning.py | 97 +++++++++++++++++-- 3 files changed, 127 insertions(+), 17 deletions(-) diff --git a/conftest.py b/conftest.py index 09ca14b7..8e7b05f9 100644 --- a/conftest.py +++ b/conftest.py @@ -1078,6 +1078,15 @@ class Environment: def __getitem__(self, name): return Model(self, name) + def ref(self, xid, raise_if_not_found=True): + model, obj_id = self( + 'ir.model.data', 'check_object_reference', + *xid.split('.', 1), + raise_on_access_error=raise_if_not_found + ) + return Model(self, model, [obj_id]) if obj_id else None + + def run_crons(self, *xids, **kw): crons = xids or self._default_crons print('running crons', crons, file=sys.stderr) diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index 6a9b3af4..e0aa0fc7 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/runbot_merge/controllers/reviewer_provisioning.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import logging +from odoo import Command from odoo.http import Controller, request, route try: @@ -34,7 +35,7 @@ class MergebotReviewerProvisioning(Controller): Partners = env['res.partner'] Users = env['res.users'] - existing_partners = Partners.search([ + existing_partners = Partners.with_context(active_test=False).search([ '|', ('email', 'in', [u['email'] for u in users]), ('github_login', 'in', [u['github_login'] for u in users]) ]) @@ -55,20 +56,24 @@ class MergebotReviewerProvisioning(Controller): if p.github_login: # assume there can't be an existing one because github_login is # unique, and should not be able to collide with emails - partners[p.github_login] = p + partners[p.github_login.casefold()] = p + portal = env.ref('base.group_portal') internal = env.ref('base.group_user') odoo_provider = env.ref('auth_oauth.provider_openerp') to_create = [] - created = updated = 0 + updated = 0 + to_activate = Partners for new in users: if 'sub' in new: new['oauth_provider_id'] = odoo_provider.id new['oauth_uid'] = new.pop('sub') # prioritise by github_login as that's the unique-est point of information - current = partners.get(new['github_login']) or partners.get(new['email']) or Partners + current = partners.get(new['github_login'].casefold()) or partners.get(new['email']) or Partners + if not current.active: + to_activate |= current # entry doesn't have user -> create user if not current.user_ids: # skip users without an email (= login) as that @@ -77,7 +82,7 @@ class MergebotReviewerProvisioning(Controller): continue new['login'] = new['email'] - new['groups_id'] = [(4, internal.id)] + new['groups_id'] = [Command.link(internal.id)] # entry has partner -> create user linked to existing partner # (and update partner implicitly) if current: @@ -90,19 +95,29 @@ class MergebotReviewerProvisioning(Controller): if len(user) != 1: _logger.warning("Got %d users for partner %s.", len(user), current.display_name) user = user[:1] + new.setdefault("active", True) update_vals = { k: v for k, v in new.items() - if v not in ('login', 'email') if v != (user[k] if k != 'oauth_provider_id' else user[k].id) } + if user.has_group('base.group_portal'): + update_vals['groups_id'] = [ + Command.unlink(portal.id), + Command.link(internal.id), + ] + if update_vals: user.write(update_vals) updated += 1 + + created = len(to_create) if to_create: # only create 100 users at a time to avoid request timeout - Users.create(to_create[:100]) - created = len(to_create[:100]) + Users.create(to_create) + + if to_activate: + to_activate.active = True _logger.info("Provisioning: created %d updated %d.", created, updated) return [created, updated] @@ -120,12 +135,13 @@ class MergebotReviewerProvisioning(Controller): def update_reviewers(self, github_logins, **kwargs): partners = request.env['res.partner'].sudo().search([('github_login', 'in', github_logins)]) partners.write({ - 'review_rights': [(5, 0, 0)], - 'delegate_reviewer': [(5, 0, 0)], + 'email': False, + 'review_rights': [Command.clear()], + 'delegate_reviewer': [Command.clear()], }) # Assign the linked users as portal users partners.mapped('user_ids').write({ - 'groups_id': [(6, 0, [request.env.ref('base.group_portal').id])] + 'groups_id': [Command.set([request.env.ref('base.group_portal').id])] }) return True diff --git a/runbot_merge/tests/test_provisioning.py b/runbot_merge/tests/test_provisioning.py index 6119bf17..af939305 100644 --- a/runbot_merge/tests/test_provisioning.py +++ b/runbot_merge/tests/test_provisioning.py @@ -15,10 +15,8 @@ def test_basic_provisioning(env, port): assert g.partner_id.name == GEORGE['name'] assert g.partner_id.github_login == GEORGE['github_login'] assert g.oauth_uid == GEORGE['sub'] - (model, g_id) = env['ir.model.data']\ - .check_object_reference('base', 'group_user') - assert model == 'res.groups' - assert g.groups_id.id == g_id, "check that users were provisioned as internal (not portal)" + internal = env.ref('base.group_user') + assert (g.groups_id & internal) == internal, "check that users were provisioned as internal (not portal)" # repeated provisioning should be a no-op r = provision_user(port, [GEORGE]) @@ -79,8 +77,20 @@ def test_upgrade_partner(env, port): 'email': GEORGE['email'], }] - p.user_ids.unlink() - p.unlink() + # deactivate user to see if provisioning re-enables them + p.user_ids.active = False + r = provision_user(port, [GEORGE]) + assert r == [0, 1] + assert len(p.user_ids) == 1, "provisioning should re-enable user" + assert p.user_ids.active + + # re-enable both user and partner to see if both get re-enabled + p.user_ids.active = False + p.active = False + r = provision_user(port, [GEORGE]) + assert r == [0, 1] + assert p.active, "provisioning should re-enable partner" + assert p.user_ids.active def test_no_email(env, port): """ Provisioning system should ignore email-less entries @@ -88,6 +98,81 @@ def test_no_email(env, port): r = provision_user(port, [{**GEORGE, 'email': None}]) assert r == [0, 0] +def test_casing(env, port): + p = env['res.partner'].create({ + 'name': 'Bob', + 'github_login': "Bob", + }) + assert not p.user_ids + assert provision_user(port, [{ + 'name': "Bob Thebuilder", + 'github_login': "bob", + 'email': 'bob@example.org', + 'sub': '5473634', + }]) == [1, 0] + + assert p.user_ids.name == 'Bob Thebuilder' + assert p.user_ids.email == 'bob@example.org' + assert p.user_ids.oauth_uid == '5473634' + # should be written on the partner through the user + assert p.name == 'Bob Thebuilder' + assert p.email == 'bob@example.org' + assert p.github_login == 'bob' + +def test_user_leaves_and_returns(env, port): + internal = env.ref('base.group_user') + portal = env.ref('base.group_portal') + categories = internal | portal | env.ref('base.group_public') + + assert provision_user(port, [{ + "name": "Bamien Douvy", + "github_login": "DouvyB", + "email": "bado@example.org", + "sub": "123456", + }]) == [1, 0] + p = env['res.partner'].search([('github_login', '=', "DouvyB")]) + assert (p.user_ids.groups_id & categories) == internal + + # bye bye 👋 + requests.post(f'http://localhost:{port}/runbot_merge/remove_reviewers', json={ + 'jsonrpc': '2.0', + 'id': None, + 'method': 'call', + 'params': {'github_logins': ['douvyb']}, + }) + assert (p.user_ids.groups_id & categories) == portal + assert p.email is False + + # he's back ❤️ + assert provision_user(port, [{ + "name": "Bamien Douvy", + "github_login": "DouvyB", + "email": "bado@example.org", + "sub": "123456", + }]) == [0, 1] + assert (p.user_ids.groups_id & categories) == internal + assert p.email == 'bado@example.org' + +def test_bulk_ops(env, port): + a, b = env['res.partner'].create([{ + 'name': "Bob", + 'email': "bob@example.org", + 'active': False, + }, { + 'name': "Coc", + 'email': "coc@example.org", + 'active': False, + }]) + assert a.active == b.active == False + + assert provision_user(port, [ + {'email': 'bob@example.org', 'github_login': 'xyz'}, + {'email': 'coc@example.org', 'github_login': 'abc'}, + ]) == [2, 0] + assert a.users_id + assert b.users_id + assert a.active == b.active == True + def provision_user(port, users): r = requests.post(f'http://localhost:{port}/runbot_merge/provision', json={ 'jsonrpc': '2.0', From 2009177adae5b8aec600c0a6d9bf916666cf3504 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 8 Jun 2023 08:19:43 +0200 Subject: [PATCH 005/192] [IMP] *: allow disabling staging on branch, remove fp target flag - currently disabling staging only works globally, allow disabling on a single branch - use a toggle - remove a pair of tests which work specifically with `fp_target`, can't work with `active` (probably) - cleanup search of possible and active stagings, add relevant indexes and use direct search of relevant branches instead of looking up from the project - also use toggle button for `active` on branches - shitty workaround for upgrading DB: apparently mail really wants to have a `user_id` to do some weird thing, so need to re-add it after resetting everything Fixes #727 --- forwardport/data/views.xml | 6 -- forwardport/models/project.py | 15 +--- forwardport/tests/test_conflicts.py | 2 +- forwardport/tests/test_limit.py | 77 +-------------------- forwardport/tests/test_updates.py | 12 ++-- forwardport/tests/test_weird.py | 16 ++--- mergebot_test_utils/utils.py | 6 +- runbot_merge/models/project.py | 31 +++++---- runbot_merge/models/pull_requests.py | 8 ++- runbot_merge/tests/test_staging.py | 41 +++++++++++ runbot_merge/views/res_partner.xml | 1 + runbot_merge/views/runbot_merge_project.xml | 3 +- 12 files changed, 88 insertions(+), 130 deletions(-) create mode 100644 runbot_merge/tests/test_staging.py diff --git a/forwardport/data/views.xml b/forwardport/data/views.xml index 46a8b48a..c25bb311 100644 --- a/forwardport/data/views.xml +++ b/forwardport/data/views.xml @@ -152,12 +152,6 @@ help="Repository where forward port branches will be created" /> - - - - diff --git a/forwardport/models/project.py b/forwardport/models/project.py index f59bdbbd..fcf7c645 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -210,17 +210,6 @@ class Repository(models.Model): _inherit = 'runbot_merge.repository' fp_remote_target = fields.Char(help="where FP branches get pushed") -class Branch(models.Model): - _inherit = 'runbot_merge.branch' - - fp_target = fields.Boolean(default=True) - fp_enabled = fields.Boolean(compute='_compute_fp_enabled') - - @api.depends('active', 'fp_target') - def _compute_fp_enabled(self): - for b in self: - b.fp_enabled = b.active and b.fp_target - class PullRequests(models.Model): _inherit = 'runbot_merge.pull_requests' @@ -438,7 +427,7 @@ class PullRequests(models.Model): ping = False msg = "Forward-port disabled." self.limit_id = limit_id - elif not limit_id.fp_enabled: + elif not limit_id.active: msg = "branch %r is disabled, it can't be used as a forward port target." % limit_id.name else: ping = False @@ -550,7 +539,7 @@ class PullRequests(models.Model): return next(( branch for branch in branches[from_+1:to_+1] - if branch.fp_enabled + if branch.active ), None) def _commits_lazy(self): diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 4e6dd084..3cda8430 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -16,7 +16,7 @@ def test_conflict(env, config, make_repo, users): project = env['runbot_merge.project'].search([]) project.write({ 'branch_ids': [ - (0, 0, {'name': 'd', 'sequence': 40, 'fp_target': True}) + (0, 0, {'name': 'd', 'sequence': 40}) ] }) diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 8310619d..fca04111 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -50,31 +50,6 @@ def test_configure(env, config, make_repo): assert prs[2].number == originals[2] -def test_self_disabled(env, config, make_repo): - """ Allow setting target as limit even if it's disabled - """ - prod, other = make_basic(env, config, make_repo) - bot_name = env['runbot_merge.project'].search([]).fp_github_name - branch_a = env['runbot_merge.branch'].search([('name', '=', 'a')]) - branch_a.fp_target = False - with prod: - [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch') - pr = prod.make_pr(target='a', head='mybranch') - prod.post_status(c, 'success', 'legal/cla') - prod.post_status(c, 'success', 'ci/runbot') - pr.post_comment('hansen r+\n%s up to a' % bot_name, config['role_reviewer']['token']) - env.run_crons() - pr_id = env['runbot_merge.pull_requests'].search([('number', '=', pr.number)]) - assert pr_id.limit_id == branch_a - - with prod: - prod.post_status('staging.a', 'success', 'legal/cla') - prod.post_status('staging.a', 'success', 'ci/runbot') - - assert env['runbot_merge.pull_requests'].search([]) == pr_id,\ - "should not have created a forward port" - - def test_ignore(env, config, make_repo): """ Provide an "ignore" command which is equivalent to setting the limit to target @@ -100,17 +75,12 @@ def test_ignore(env, config, make_repo): "should not have created a forward port" -@pytest.mark.parametrize('enabled', ['active', 'fp_target']) -def test_disable(env, config, make_repo, users, enabled): +def test_disable(env, config, make_repo, users): """ Checks behaviour if the limit target is disabled: * disable target while FP is ongoing -> skip over (and stop there so no FP) * forward-port over a disabled branch * request a disabled target as limit - - Disabling (with respect to forward ports) can be performed by marking the - branch as !active (which also affects mergebot operations), or as - !fp_target (won't be forward-ported to). """ prod, other = make_basic(env, config, make_repo) project = env['runbot_merge.project'].search([]) @@ -133,7 +103,7 @@ def test_disable(env, config, make_repo, users, enabled): prod.post_status('staging.a', 'success', 'legal/cla') prod.post_status('staging.a', 'success', 'ci/runbot') # disable branch b - env['runbot_merge.branch'].search([('name', '=', 'b')]).write({enabled: False}) + env['runbot_merge.branch'].search([('name', '=', 'b')]).active = False env.run_crons() # should have created a single PR (to branch c, for pr 1) @@ -168,49 +138,6 @@ def test_disable(env, config, make_repo, users, enabled): seen(env, pr, users), } - -def test_default_disabled(env, config, make_repo, users): - """ If the default limit is disabled, it should still be the default - limit but the ping message should be set on the actual last FP (to the - last non-deactivated target) - """ - prod, other = make_basic(env, config, make_repo) - branch_c = env['runbot_merge.branch'].search([('name', '=', 'c')]) - branch_c.fp_target = False - - with prod: - [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/branch0') - pr = prod.make_pr(target='a', head='branch0') - 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() - - assert env['runbot_merge.pull_requests'].search([]).limit_id == branch_c - - with prod: - prod.post_status('staging.a', 'success', 'legal/cla') - prod.post_status('staging.a', 'success', 'ci/runbot') - env.run_crons() - - p1, p2 = env['runbot_merge.pull_requests'].search([], order='number') - assert p1.number == pr.number - pr2 = prod.get_pr(p2.number) - - cs = pr2.comments - assert len(cs) == 2 - assert pr2.comments == [ - seen(env, pr2, users), - (users['user'], """\ -@%(user)s @%(reviewer)s this PR targets b and is the last of the forward-port chain. - -To merge the full chain, say -> @%(user)s r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""" % users) - ] - def test_limit_after_merge(env, config, make_repo, users): """ If attempting to set a limit () on a PR which is merged (already forward-ported or not), or is a forward-port PR, fwbot should diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 99471cd7..384df08f 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -151,9 +151,7 @@ def test_update_merged(env, make_repo, config, users): with prod: prod.make_ref('heads/d', prod.commit('c').id) env['runbot_merge.project'].search([]).write({ - 'branch_ids': [(0, 0, { - 'name': 'd', 'sequence': 40, 'fp_target': True, - })] + 'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})] }) with prod: @@ -251,10 +249,10 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'master', 'sequence': 0, 'fp_target': True}), - (0, 0, {'name': 'v3', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'v2', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'v1', 'sequence': 3, 'fp_target': True}), + (0, 0, {'name': 'master', 'sequence': 0}), + (0, 0, {'name': 'v3', 'sequence': 1}), + (0, 0, {'name': 'v2', 'sequence': 2}), + (0, 0, {'name': 'v1', 'sequence': 3}), ], 'repo_ids': [ (0, 0, { diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index ab077c05..4ef56feb 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -26,9 +26,9 @@ def make_basic(env, config, make_repo, *, fp_token, fp_remote): 'github_prefix': 'hansen', 'fp_github_token': fp_token and config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 2}), + (0, 0, {'name': 'b', 'sequence': 1}), + (0, 0, {'name': 'c', 'sequence': 0}), ], }) @@ -263,9 +263,9 @@ class TestNotAllBranches: 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 2}), + (0, 0, {'name': 'b', 'sequence': 1}), + (0, 0, {'name': 'c', 'sequence': 0}), ] }) repo_a = env['runbot_merge.repository'].create({ @@ -514,7 +514,7 @@ def test_new_intermediate_branch(env, config, make_repo): env.run_crons() project.write({ 'branch_ids': [ - (0, False, {'name': 'new', 'sequence': 1, 'fp_target': True}), + (0, False, {'name': 'new', 'sequence': 1}), ] }) env.run_crons() @@ -748,7 +748,7 @@ def test_retarget_after_freeze(env, config, make_repo, users): project.write({ 'branch_ids': [ (1, branch_c.id, {'sequence': 1}), - (0, 0, {'name': 'bprime', 'sequence': 2, 'fp_target': True}), + (0, 0, {'name': 'bprime', 'sequence': 2}), (1, branch_b.id, {'sequence': 3}), (1, branch_a.id, {'sequence': 4}), ] diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 2df8995d..a23d6726 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -74,9 +74,9 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 100, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 80, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 60, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 100}), + (0, 0, {'name': 'b', 'sequence': 80}), + (0, 0, {'name': 'c', 'sequence': 60}), ], }) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index fb46c33f..883c91c6 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -46,10 +46,11 @@ class Project(models.Model): freeze_reminder = fields.Text() def _check_stagings(self, commit=False): - for branch in self.search([]).mapped('branch_ids').filtered('active'): + # check branches with an active staging + for branch in self.env['runbot_merge.branch']\ + .with_context(active_test=False)\ + .search([('active_staging_id', '!=', False)]): staging = branch.active_staging_id - if not staging: - continue try: with self.env.cr.savepoint(): staging.check_status() @@ -61,16 +62,20 @@ class Project(models.Model): self.env.cr.commit() def _create_stagings(self, commit=False): - for branch in self.search([]).mapped('branch_ids').filtered('active'): - if not branch.active_staging_id: - try: - with self.env.cr.savepoint(): - branch.try_staging() - except Exception: - _logger.exception("Failed to create staging for branch %r", branch.name) - else: - if commit: - self.env.cr.commit() + # look up branches which can be staged on and have no active staging + for branch in self.env['runbot_merge.branch'].search([ + ('active_staging_id', '=', False), + ('active', '=', True), + ('staging_enabled', '=', True), + ]): + try: + with self.env.cr.savepoint(): + branch.try_staging() + except Exception: + _logger.exception("Failed to create staging for branch %r", branch.name) + else: + if commit: + self.env.cr.commit() def _find_commands(self, comment): return re.findall( diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8be1070b..47ea4001 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -67,7 +67,7 @@ class Repository(models.Model): sequence = fields.Integer(default=50, group_operator=None) name = fields.Char(required=True) - project_id = fields.Many2one('runbot_merge.project', required=True) + project_id = fields.Many2one('runbot_merge.project', required=True, index=True) status_ids = fields.One2many('runbot_merge.repository.status', 'repo_id', string="Required Statuses") group_id = fields.Many2one('res.groups', default=lambda self: self.env.ref('base.group_user')) @@ -235,10 +235,10 @@ class Branch(models.Model): _order = 'sequence, name' name = fields.Char(required=True) - project_id = fields.Many2one('runbot_merge.project', required=True) + project_id = fields.Many2one('runbot_merge.project', required=True, index=True) active_staging_id = fields.Many2one( - 'runbot_merge.stagings', compute='_compute_active_staging', store=True, + 'runbot_merge.stagings', compute='_compute_active_staging', store=True, index=True, help="Currently running staging for the branch." ) staging_ids = fields.One2many('runbot_merge.stagings', 'target') @@ -252,6 +252,8 @@ class Branch(models.Model): active = fields.Boolean(default=True) sequence = fields.Integer(group_operator=None) + staging_enabled = fields.Boolean(default=True) + def _auto_init(self): res = super(Branch, self)._auto_init() tools.create_unique_index( diff --git a/runbot_merge/tests/test_staging.py b/runbot_merge/tests/test_staging.py new file mode 100644 index 00000000..c594cc76 --- /dev/null +++ b/runbot_merge/tests/test_staging.py @@ -0,0 +1,41 @@ +import pytest + +from utils import Commit, to_pr + + +@pytest.fixture +def repo(env, project, make_repo, users, setreviewers): + r = make_repo('repo') + project.write({'repo_ids': [(0, 0, { + 'name': r.name, + 'group_id': False, + 'required_statuses': 'ci' + })]}) + setreviewers(*project.repo_ids) + return r + +def test_staging_disabled_branch(env, project, repo, config): + """Check that it's possible to disable staging on a specific branch + """ + project.branch_ids = [(0, 0, { + 'name': 'other', + 'staging_enabled': False, + })] + with repo: + [master_commit] = repo.make_commits(None, Commit("master", tree={'a': '1'}), ref="heads/master") + [c1] = repo.make_commits(master_commit, Commit("thing", tree={'a': '2'}), ref='heads/master-thing') + master_pr = repo.make_pr(title="whatever", target="master", head="master-thing") + master_pr.post_comment("hansen r+", config['role_reviewer']['token']) + repo.post_status(c1, 'success', 'ci') + + [other_commit] = repo.make_commits(None, Commit("other", tree={'b': '1'}), ref='heads/other') + [c2] = repo.make_commits(other_commit, Commit("thing", tree={'b': '2'}), ref='heads/other-thing') + other_pr = repo.make_pr(title="whatever", target="other", head="other-thing") + other_pr.post_comment("hansen r+", config['role_reviewer']['token']) + repo.post_status(c2, 'success', 'ci') + env.run_crons() + + assert to_pr(env, master_pr).staging_id, \ + "master is allowed to stage, should be staged" + assert not to_pr(env, other_pr).staging_id, \ + "other is *not* allowed to stage, should not be staged" diff --git a/runbot_merge/views/res_partner.xml b/runbot_merge/views/res_partner.xml index fde5f7e1..507e5a32 100644 --- a/runbot_merge/views/res_partner.xml +++ b/runbot_merge/views/res_partner.xml @@ -25,6 +25,7 @@ + diff --git a/runbot_merge/views/runbot_merge_project.xml b/runbot_merge/views/runbot_merge_project.xml index 2f670ed3..544f9cf9 100644 --- a/runbot_merge/views/runbot_merge_project.xml +++ b/runbot_merge/views/runbot_merge_project.xml @@ -56,7 +56,8 @@ - + + From 048ae0c5ff9774f269d03b783a48b6449f42b097 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 8 Jun 2023 08:26:10 +0200 Subject: [PATCH 006/192] [FIX] forwardport: flag statuses as `recursive` I'd been convinced this was an ORM error because the field is not recursive... in runbot_merge, in forwardbot it is and thus does indeed need to be flagged to avoid the warning. --- forwardport/models/project.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index fcf7c645..3112941b 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -213,6 +213,8 @@ class Repository(models.Model): class PullRequests(models.Model): _inherit = 'runbot_merge.pull_requests' + statuses = fields.Text(recursive=True) + limit_id = fields.Many2one('runbot_merge.branch', help="Up to which branch should this PR be forward-ported") parent_id = fields.Many2one( From e14616b2fb9a7458f4739c8c4212995c349e4b03 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 8 Jun 2023 10:03:26 +0200 Subject: [PATCH 007/192] [IMP] runbot_merge: add support for draft check 1cea247e6ce6b8add5bca06634655e43a53024dd missed the update of the `draft` flag, add support for it. Fixes #753 --- runbot_merge/models/pull_requests.py | 9 ++++++++- runbot_merge/tests/test_basic.py | 7 ++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 47ea4001..76ebb427 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -143,10 +143,17 @@ All substitutions are tentatively applied sequentially to the input. }, 'sender': {'login': self.project_id.github_prefix}, }) + edit2 = '' + if pr_id.draft != pr['draft']: + edit2 = controllers.handle_pr(self.env, { + 'action': 'converted_to_draft' if pr['draft'] else 'ready_for_review', + 'pull_request': pr, + 'sender': {'login': self.project_id.github_prefix} + }) + '. ' feedback({ 'repository': pr_id.repository.id, 'pull_request': number, - 'message': f"{edit}. {sync}.", + 'message': f"{edit}. {edit2}{sync}.", }) return diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ea5dd098..c15925b1 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2520,6 +2520,7 @@ Please check and re-approve. 'state': 'ready', 'message': "Something else", 'target': other.id, + 'draft': True, }) with repo: pr.post_comment('hansen check') @@ -2528,7 +2529,11 @@ Please check and re-approve. assert pr_id.head == c2 assert pr_id.message == 'title\n\nbody' # the commit's message was used for the PR assert pr_id.target.name == 'master' - assert pr.comments[-1] == (users['user'], f"Updated target, squash, message. Updated to {c2}.") + assert not pr_id.draft + assert pr.comments[-1] == ( + users['user'], + f"Updated target, squash, message. Updated {pr_id.display_name} to ready. Updated to {c2}." + ) def test_update_closed(self, env, repo): with repo: From 270dfdd495e5f58ee78f245aa76f1b49a05bda4d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 12 Jun 2023 14:41:42 +0200 Subject: [PATCH 008/192] [REF] *: move most feedback messages to pseudo-templates Current system makes it hard to iterate feedback messages and make them clearer, this should improve things a touch. Use a bespoke model to avoid concerns with qweb rendering complexity (we just want GFM output and should not need logic). Also update fwbot test setup to always configure an fwbot name, in order to avoid ping messages closing the PRs they're talking about, that took a while to debug, and given the old message I assume I'd already hit it and just been too lazy to fix. This requires updating a bunch of tests as fwbot ping are sent *to* `fp_github_name`, but sent *from* the reference user (because that's the key we set). Note: noupdate on CSV files doesn't seem to work anymore, which isn't great. But instead set tracking on the template's templates, it's not quite as good but should be sufficient. Fixes #769 --- forwardport/models/forwardport.py | 61 ++--- forwardport/models/project.py | 213 +++++++-------- forwardport/tests/test_conflicts.py | 6 +- forwardport/tests/test_limit.py | 13 +- forwardport/tests/test_simple.py | 20 +- forwardport/tests/test_updates.py | 5 +- forwardport/tests/test_weird.py | 10 +- mergebot_test_utils/utils.py | 2 + runbot_merge/__manifest__.py | 1 + runbot_merge/controllers/__init__.py | 14 +- ..._merge.pull_requests.feedback.template.csv | 171 ++++++++++++ runbot_merge/models/pull_requests.py | 246 +++++++++--------- runbot_merge/security/ir.model.access.csv | 1 + runbot_merge/tests/test_basic.py | 11 +- runbot_merge/tests/test_disabled_branch.py | 2 +- runbot_merge/views/configuration.xml | 37 ++- 16 files changed, 512 insertions(+), 301 deletions(-) create mode 100644 runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index f94b6db9..78c23a46 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -98,14 +98,6 @@ class ForwardPortTasks(models.Model, Queue): batch.active = False -CONFLICT_TEMPLATE = "{ping}WARNING: the latest change ({previous.head}) triggered " \ - "a conflict when updating the next forward-port " \ - "({next.display_name}), and has been ignored.\n\n" \ - "You will need to update this pull request differently, " \ - "or fix the issue by hand on {next.display_name}." -CHILD_CONFLICT = "{ping}WARNING: the update of {previous.display_name} to " \ - "{previous.head} has caused a conflict in this pull request, " \ - "data may have been lost." 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' @@ -116,7 +108,6 @@ class UpdateQueue(models.Model, Queue): new_root = fields.Many2one('runbot_merge.pull_requests') def _process_item(self): - Feedback = self.env['runbot_merge.pull_requests.feedback'] previous = self.new_root with ExitStack() as s: for child in self.new_root._iter_descendants(): @@ -134,41 +125,35 @@ class UpdateQueue(models.Model, Queue): self.new_root.display_name ) if child.state in ('closed', 'merged'): - Feedback.create({ - 'repository': child.repository.id, - 'pull_request': child.number, - 'message': "%sancestor PR %s has been updated but this PR" - " is %s and can't be updated to match." - "\n\n" - "You may want or need to manually update any" - " followup PR." % ( - child.ping(), - self.new_root.display_name, - child.state, - ) - }) + self.env.ref('runbot_merge.forwardport.updates.closed')._send( + repository=child.repository, + pull_request=child.number, + token_field='fp_github_token', + format_args={'pr': child, 'parent': self.new_root}, + ) return conflicts, working_copy = previous._create_fp_branch( child.target, child.refname, s) if conflicts: _, out, err, _ = conflicts - Feedback.create({ - 'repository': previous.repository.id, - 'pull_request': previous.number, - 'message': CONFLICT_TEMPLATE.format( - ping=previous.ping(), - previous=previous, - next=child - ) - }) - Feedback.create({ - 'repository': child.repository.id, - 'pull_request': child.number, - 'message': CHILD_CONFLICT.format(ping=child.ping(), previous=previous, next=child)\ - + (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else '') - + (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else '') - }) + self.env.ref('runbot_merge.forwardport.updates.conflict.parent')._send( + repository=previous.repository, + pull_request=previous.number, + token_field='fp_github_token', + format_args={'pr': previous, 'next': child}, + ) + self.env.ref('runbot_merge.forwardport.updates.conflict.child')._send( + repository=child.repository, + pull_request=child.number, + token_field='fp_github_token', + format_args={ + 'previous': previous, + 'pr': child, + 'stdout': (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else ''), + 'stderr': (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else ''), + }, + ) new_head = working_copy.stdout().rev_parse(child.refname).stdout.decode().strip() commits_count = int(working_copy.stdout().rev_list( diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 3112941b..720000e7 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -67,8 +67,9 @@ class Project(models.Model): def _compute_git_identity(self): s = requests.Session() for project in self: - if not project.fp_github_token: + if not project.fp_github_token or (self.fp_github_name and self.fp_github_email): continue + r0 = s.get('https://api.github.com/user', headers={ 'Authorization': 'token %s' % project.fp_github_token }) @@ -246,6 +247,25 @@ class PullRequests(models.Model): for pr in self: pr.refname = pr.label.split(':', 1)[-1] + ping = fields.Char(recursive=True) + + @api.depends('source_id.author.github_login', 'source_id.reviewed_by.github_login') + def _compute_ping(self): + """For forward-port PRs (PRs with a source) the author is the PR bot, so + we want to ignore that and use the author & reviewer of the original PR + """ + source = self.source_id + if not source: + return super()._compute_ping() + + for pr in self: + s = ' '.join( + f'@{p.github_login}' + for p in source.author | source.reviewed_by | self.reviewed_by + ) + pr.ping = s and (s + ' ') + + @api.model_create_single def create(self, vals): # PR opened event always creates a new PR, override so we can precreate PRs @@ -297,32 +317,24 @@ class PullRequests(models.Model): if self.env.context.get('forwardport_detach_warn', True): for p in with_parents: if not p.parent_id: - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': p.repository.id, - 'pull_request': p.number, - 'message': "%sthis PR was modified / updated and has become a normal PR. " - "It should be merged the normal way (via @%s)" % ( - p.source_id.ping(), - p.repository.project_id.github_prefix, - ), - 'token_field': 'fp_github_token', - }) + self.env.ref('runbot_merge.forwardport.update.detached')._send( + repository=p.repository, + pull_request=p.number, + token_field='fp_github_token', + format_args={'pr': p}, + ) for p in closed_fp.filtered(lambda p: p.state != 'closed'): - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': p.repository.id, - 'pull_request': p.number, - 'message': "%sthis PR was closed then reopened. " - "It should be merged the normal way (via @%s)" % ( - p.source_id.ping(), - p.repository.project_id.github_prefix, - ), - 'token_field': 'fp_github_token', - }) + self.env.ref('runbot_merge.forwardport.reopen.detached')._send( + repository=p.repository, + pull_request=p.number, + token_field='fp_github_token', + format_args={'pr': p}, + ) if vals.get('state') == 'merged': - for p in self: - self.env['forwardport.branch_remover'].create({ - 'pr_id': p.id, - }) + self.env['forwardport.branch_remover'].create([ + {'pr_id': p.id} + for p in self + ]) # if we change the policy to skip CI, schedule followups on existing FPs if vals.get('fw_policy') == 'skipci' and self.state == 'merged': self.env['runbot_merge.pull_requests'].search([ @@ -455,15 +467,12 @@ class PullRequests(models.Model): if not (self.state == 'opened' and self.parent_id): return - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'token_field': 'fp_github_token', - 'message': '%s%s failed on this forward-port PR' % ( - self.source_id.ping(), - ci, - ) - }) + self.env.ref('runbot_merge.forwardport.ci.failed')._send( + repository=self.repository, + pull_request=self.number, + token_field='fp_github_token', + format_args={'pr': self, 'ci': ci}, + ) def _validate(self, statuses): failed = super()._validate(statuses) @@ -636,16 +645,12 @@ class PullRequests(models.Model): linked, other = different_pr, different_target if t != target: linked, other = ref, target - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'token_field': 'fp_github_token', - 'message': "%sthis pull request can not be forward ported: " - "next branch is %r but linked pull request %s " - "has a next branch %r." % ( - pr.ping(), t.name, linked.display_name, other.name - ) - }) + self.env.ref('runbot_merge.forwardport.failure.discrepancy')._send( + repository=pr.repository, + pull_request=pr.number, + token_field='fp_github_token', + format_args={'pr': pr, 'linked': linked, 'next': t.name, 'other': other.name}, + ) _logger.warning( "Cancelling forward-port of %s: found different next branches (%s)", self, all_targets @@ -743,23 +748,18 @@ class PullRequests(models.Model): 'delegates': [(6, False, (source.delegates | pr.delegates).ids)] }) if has_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'): - message = source.ping() + """\ -the next pull request (%s) is in conflict. You can merge the chain up to here by saying -> @%s r+ -%s""" % (new_pr.display_name, pr.repository.project_id.fp_github_name, footer) - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': message, - 'token_field': 'fp_github_token', - }) + self.env.ref('runbot_merge.forwardport.failure.conflict')._send( + repository=pr.repository, + pull_request=pr.number, + token_field='fp_github_token', + format_args={'source': source, 'pr': pr, 'new': new_pr, 'footer': footer}, + ) # not great but we probably want to avoid the risk of the webhook # creating the PR from under us. There's still a "hole" between # the POST being executed on gh and the commit but... self.env.cr.commit() for pr, new_pr in zip(self, new_batch): - source = pr.source_id or pr (h, out, err, hh) = conflicts.get(pr) or (None, None, None, None) if h: @@ -775,47 +775,47 @@ the next pull request (%s) is in conflict. You can merge the chain up to here by '* %s%s\n' % (sha, ' <- on this commit' if sha == h else '') for sha in hh ) - message = f"""{source.ping()}cherrypicking of pull request {source.display_name} failed. -{lines}{sout}{serr} -Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). - -In the former case, you may want to edit this PR message as well. -""" + template = 'runbot_merge.forwardport.failure' + format_args = { + 'pr': new_pr, + 'commits': lines, + 'stdout': sout, + 'stderr': serr, + 'footer': footer, + } elif has_conflicts: - message = """%s\ -while this was properly forward-ported, at least one co-dependent PR (%s) did \ -not succeed. You will need to fix it before this can be merged. - -Both this PR and the others will need to be approved via `@%s r+` as they are \ -all considered "in conflict". -%s""" % ( - source.ping(), - ', '.join(p.display_name for p in (new_batch - new_pr)), - proj.github_prefix, - footer - ) + template = 'runbot_merge.forwardport.linked' + format_args = { + 'pr': new_pr, + 'siblings': ', '.join(p.display_name for p in (new_batch - new_pr)), + 'footer': footer, + } elif base._find_next_target(new_pr) is None: ancestors = "".join( "* %s\n" % p.display_name for p in pr._iter_ancestors() if p.parent_id ) - message = source.ping() + """\ -this PR targets %s and is the last of the forward-port chain%s -%s -To merge the full chain, say -> @%s r+ -%s""" % (target.name, ' containing:' if ancestors else '.', ancestors, pr.repository.project_id.fp_github_name, footer) + template = 'runbot_merge.forwardport.final' + format_args = { + 'pr': new_pr, + 'containing': ' containing:' if ancestors else '.', + 'ancestors': ancestors, + 'footer': footer, + } else: - message = """\ -This PR targets %s and is part of the forward-port chain. Further PRs will be created up to %s. -%s""" % (target.name, base.limit_id.name, footer) - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': new_pr.repository.id, - 'pull_request': new_pr.number, - 'message': message, - 'token_field': 'fp_github_token', - }) + template = 'runbot_merge.forwardport.intermediate' + format_args = { + 'pr': new_pr, + 'footer': footer, + } + self.env.ref(template)._send( + repository=new_pr.repository, + pull_request=new_pr.number, + token_field='fp_github_token', + format_args=format_args, + ) + labels = ['forwardport'] if has_conflicts: labels.append('conflict') @@ -1150,31 +1150,18 @@ stderr: if source.merge_date > (cutoff_dt - backoff): continue source.reminder_backoff_factor += 1 - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': source.repository.id, - 'pull_request': source.number, - 'message': "%sthis pull request has forward-port PRs awaiting action (not merged or closed):\n%s" % ( - source.ping(), - '\n- '.join(pr.display_name for pr in sorted(prs, key=lambda p: p.number)) - ), - 'token_field': 'fp_github_token', - }) - - def ping(self, author=True, reviewer=True): - source = self.source_id - if not source: - return super().ping(author=author, reviewer=reviewer) - - # use a dict literal to maintain ordering (3.6+) - pingline = ' '.join( - f'@{p.github_login}' - for p in filter(None, { - author and source.author: None, - reviewer and source.reviewed_by: None, - reviewer and self.reviewed_by: None, - }) - ) - return pingline and (pingline + ' ') + self.env.ref('runbot_merge.forwardport.reminder')._send( + repository=source.repository, + pull_request=source.number, + token_field='fp_github_token', + format_args={ + 'pr': source, + 'outstanding': ''.join( + f'\n- {pr.display_name}' + for pr in sorted(prs, key=lambda p: p.number) + ), + } + ) class Stagings(models.Model): _inherit = 'runbot_merge.stagings' diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 3cda8430..d5bb3486 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -316,11 +316,11 @@ def test_multiple_commits_different_authorship(env, config, make_repo, users, ro c = prod.commit(pr2_id.head) assert len(c.parents) == 1 get = itemgetter('name', 'email') - rm = rolemap['user'] - assert get(c.author) == (rm['login'], ''), \ + bot = pr_id.repository.project_id.fp_github_name + assert get(c.author) == (bot, ''), \ "In a multi-author PR, the squashed conflict commit should have the " \ "author set to the bot but an empty email" - assert get(c.committer) == (rm['login'], '') + assert get(c.committer) == (bot, '') assert re.match(r'''<<<\x3c<<< HEAD b diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index fca04111..db13f535 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -174,7 +174,7 @@ def test_limit_after_merge(env, config, make_repo, users): (users['reviewer'], "hansen r+"), seen(env, pr1, users), (users['reviewer'], bot_name + ' up to b'), - (bot_name, "@%s forward-port limit can only be set before the PR is merged." % users['reviewer']), + (users['user'], "@%s forward-port limit can only be set before the PR is merged." % users['reviewer']), ] assert pr2.comments == [ seen(env, pr2, users), @@ -184,7 +184,7 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port """), (users['reviewer'], bot_name + ' up to b'), - (bot_name, "@%s forward-port limit can only be set on an origin PR" + (users['user'], "@%s forward-port limit can only be set on an origin PR" " (%s here) before it's merged and forward-ported." % ( users['reviewer'], p1.display_name, @@ -208,13 +208,14 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port env.run_crons() assert pr2.comments[4:] == [ - (bot_name, "@%s @%s this PR was modified / updated and has become a normal PR. " + (users['user'], "@%s @%s this PR was modified / updated and has become a normal PR. " "It should be merged the normal way (via @%s)" % ( users['user'], users['reviewer'], p2.repository.project_id.github_prefix )), (users['reviewer'], bot_name + ' up to b'), - (bot_name, f"@{users['reviewer']} forward-port limit can only be set on an origin PR " - f"({p1.display_name} here) before it's merged and forward-ported." - ), + (users['user'], f"@{users['reviewer']} forward-port limit can only be " + f"set on an origin PR ({p1.display_name} here) before " + f"it's merged and forward-ported." + ), ] diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index ef31c495..5f5e21b8 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -109,7 +109,7 @@ def test_straightforward_flow(env, config, make_repo, users): assert c.author['name'] == other_user['user'], "author should still be original's probably" assert c.committer['name'] == other_user['user'], "committer should also still be the original's, really" - assert pr1.ping() == "@%s @%s " % ( + assert pr1.ping == "@%s @%s " % ( config['role_other']['user'], config['role_reviewer']['user'], ), "ping of forward-port PR should include author and reviewer of source" @@ -132,7 +132,7 @@ def test_straightforward_flow(env, config, make_repo, users): (users['reviewer'], 'hansen r+ rebase-ff'), seen(env, pr, users), (users['user'], 'Merge method set to rebase and fast-forward.'), - (users['user'], '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % ( + (users['user'], '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n- %s' % ( users['other'], users['reviewer'], '\n- '.join((pr1 | pr2).mapped('display_name')) )), @@ -160,7 +160,7 @@ def test_straightforward_flow(env, config, make_repo, users): @%s @%s this PR targets c and is the last of the forward-port chain containing: * %s -To merge the full chain, say +To merge the full chain, use > @%s r+ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port @@ -315,7 +315,11 @@ def test_empty(env, config, make_repo, users): assert env['runbot_merge.pull_requests'].search([], order='number') == prs # change FP token to see if the feedback comes from the proper user project = env['runbot_merge.project'].search([]) - project.fp_github_token = config['role_other']['token'] + project.write({ + 'fp_github_name': False, + 'fp_github_email': False, + 'fp_github_token': config['role_other']['token'], + }) assert project.fp_github_name == users['other'] # check reminder @@ -324,7 +328,7 @@ def test_empty(env, config, make_repo, users): awaiting = ( users['other'], - '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % ( + '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n- %s' % ( users['user'], users['reviewer'], fail_id.display_name ) @@ -582,11 +586,11 @@ def test_delegate_fw(env, config, make_repo, users): seen(env, pr2, users), (users['user'], '''@{self_reviewer} @{reviewer} this PR targets c and is the last of the forward-port chain. -To merge the full chain, say -> @{user} r+ +To merge the full chain, use +> @{bot} r+ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -'''.format_map(users)), +'''.format(bot=pr1_id.repository.project_id.fp_github_name, **users)), (users['other'], 'hansen r+') ] diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 384df08f..1635fc5e 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -3,9 +3,6 @@ Test cases for updating PRs during after the forward-porting process after the initial merge has succeeded (and forward-porting has started) """ import re -import sys - -import pytest from utils import seen, re_matches, Commit, make_basic, to_pr @@ -248,6 +245,8 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): 'github_token': config['github']['token'], 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], + 'fp_github_name': 'herbert', + 'fp_github_email': 'hb@example.com', 'branch_ids': [ (0, 0, {'name': 'master', 'sequence': 0}), (0, 0, {'name': 'v3', 'sequence': 1}), diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 4ef56feb..32bcf0db 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -25,6 +25,8 @@ def make_basic(env, config, make_repo, *, fp_token, fp_remote): 'github_token': config['github']['token'], 'github_prefix': 'hansen', 'fp_github_token': fp_token and config['github']['token'], + 'fp_github_name': 'herbert', + 'fp_github_email': 'hb@example.com', 'branch_ids': [ (0, 0, {'name': 'a', 'sequence': 2}), (0, 0, {'name': 'b', 'sequence': 1}), @@ -262,6 +264,8 @@ class TestNotAllBranches: 'github_token': config['github']['token'], 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], + 'fp_github_name': 'herbert', + 'fp_github_email': 'hb@example.com', 'branch_ids': [ (0, 0, {'name': 'a', 'sequence': 2}), (0, 0, {'name': 'b', 'sequence': 1}), @@ -401,7 +405,7 @@ class TestNotAllBranches: assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_a, users), - (users['user'], "@%s @%s this pull request can not be forward ported:" + (users['user'], "@%s @%s this pull request can not be forward-ported:" " next branch is 'b' but linked pull request %s " "has a next branch 'c'." % ( users['user'], users['reviewer'], pr_b_id.display_name, @@ -410,7 +414,7 @@ class TestNotAllBranches: assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_b, users), - (users['user'], "@%s @%s this pull request can not be forward ported:" + (users['user'], "@%s @%s this pull request can not be forward-ported:" " next branch is 'c' but linked pull request %s " "has a next branch 'b'." % ( users['user'], users['reviewer'], pr_a_id.display_name, @@ -472,6 +476,7 @@ def test_new_intermediate_branch(env, config, make_repo): with prod: validate(prod, pr0_fp_id.head) env.run_crons() + assert pr0_fp_id.state == 'validated' original0 = PRs.search([('parent_id', '=', pr0_fp_id.id)]) assert original0, "Could not find FP of PR0 to C" assert original0.target.name == 'c' @@ -519,6 +524,7 @@ def test_new_intermediate_branch(env, config, make_repo): }) env.run_crons() + assert pr0_fp_id.state == 'validated' # created an intermediate PR for 0 and x desc0 = PRs.search([('source_id', '=', pr0_id.id)]) new0 = desc0 - pr0_fp_id - original0 diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index a23d6726..b87a2345 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -73,6 +73,8 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje 'github_token': config['github']['token'], 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], + 'fp_github_name': 'herbert', + 'fp_github_email': 'hb@example.com', 'branch_ids': [ (0, 0, {'name': 'a', 'sequence': 100}), (0, 0, {'name': 'b', 'sequence': 80}), diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index 82978d76..3640d249 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -7,6 +7,7 @@ 'security/ir.model.access.csv', 'data/merge_cron.xml', + 'data/runbot_merge.pull_requests.feedback.template.csv', 'views/res_partner.xml', 'views/runbot_merge_project.xml', 'views/mergebot.xml', diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index efb752ac..51d8c71d 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -143,11 +143,19 @@ def handle_pr(env, event): message = None if not branch: - message = f"This PR targets the un-managed branch {r}:{b}, it needs to be retargeted before it can be merged." + message = env.ref('runbot_merge.handle.branch.unmanaged')._format( + repository=r, + branch=b, + event=event, + ) _logger.info("Ignoring event %s on PR %s#%d for un-managed branch %s", event['action'], r, pr['number'], b) elif not branch.active: - message = f"This PR targets the disabled branch {r}:{b}, it needs to be retargeted before it can be merged." + message = env.ref('runbot_merge.handle.branch.inactive')._format( + repository=r, + branch=b, + event=event, + ) if message and event['action'] not in ('synchronize', 'closed'): feedback(message=message) @@ -240,7 +248,7 @@ def handle_pr(env, event): if pr_obj.state == 'merged': feedback( close=True, - message="@%s ya silly goose you can't reopen a merged PR." % event['sender']['login'] + message=env.ref('runbot_merge.handle.pr.merged')._format(event=event), ) if pr_obj.state == 'closed': diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv new file mode 100644 index 00000000..70ed7397 --- /dev/null +++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv @@ -0,0 +1,171 @@ +id,template,help +runbot_merge.handle.branch.unmanaged,"This PR targets the un-managed branch {repository}:{branch}, it needs to be retargeted before it can be merged.","Notifies of event on PR whose branch is not managed by the mergebot. + +repository: repository name +branch: branch (ref) name +event: complete pr event" +runbot_merge.handle.branch.inactive,"This PR targets the disabled branch {repository}:{branch}, it needs to be retargeted before it can be merged.","Notifies of event on PR whose branch is deactivated. + +repository: repository name +branch: branch (ref) name +event: complete pr event" +runbot_merge.handle.pr.merged,@{event[sender][login]} ya silly goose you can't reopen a merged PR.,"Notifies that a user tried to reopen a merged PR. + +Event: complete PR event" +runbot_merge.pr.load.unmanaged,"Branch `{pr[base][ref]}` is not within my remit, imma just ignore it.","Notifies that a user tried to load a PR targeting a non-handled branch. + +pr: pull request (github object) +Repository: repository object (???)" +runbot_merge.pr.load.fetched,"{pr.ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction). + +Pr: pr object we just created" +runbot_merge.pr.branch.disabled,"{pr.ping}the target branch {pr.target.name!r} has been disabled, you may want to close this PR.","Notifies that the target branch for this PR was deactivated. + +pr: pull request in question" +runbot_merge.pr.merge.failed,{pr.ping}unable to stage: {reason},"Notifies that the PR could not be merged into the staging branch. + +pr: pr object we tried to merge +reason: error message +exc: exception object" +runbot_merge.pr.fetch.unmanaged,I'm sorry. Branch `{branch}` is not within my remit.,"Responds to a request to fetch a PR to an unmanaged branch. + +repository: pr repository +branch: target branch +number: pr number" +runbot_merge.command.access.no,"I'm sorry, @{user}. I'm afraid I can't do that.","Responds to command by a user who has no rights at all. + +user: github login of comment sender +pr: pr object to which the command was sent" +runbot_merge.command.approve.failure,@{user} you may want to rebuild or fix this PR as it has failed CI.,"Responds to r+ of PR with failed CI. + +user: github login of comment sender +pr: pr object to which the command was sent" +runbot_merge.command.unapprove.p0,"PR priority reset to 1, as pull requests with priority 0 ignore review state.","Responds to r- of pr in p=0. + +user: github login of comment sender +pr: pr object to which the command was sent" +runbot_merge.command.method,Merge method set to {new_method}.,"Responds to the setting of the merge method. + +new_method: ... +pr: pr object to which the command was sent +user: github login of the comment sender" +runbot_merge.failure.approved,{pr.ping}{status!r} failed on this reviewed PR.,"Notification of failed status on a reviewed PR. + +pr: pull request in question +status: failed status" +runbot_merge.pr.created,[Pull request status dashboard]({pr.url}).,"Initial comment on PR creation. + +pr: created pr" +runbot_merge.pr.linked.not_ready,{pr.ping}linked pull request(s) {siblings} not ready. Linked PRs are not staged until all of them are ready.,"Comment when a PR is ready (approved & validated) but it is linked to other PRs which are not. + +pr: pr we're looking at +siblings: its siblings, as a single comma-separated list of PR links" +runbot_merge.pr.merge_method,"{pr.ping}because this PR has multiple commits, I need to know how to merge it: + +{methods}","Comment when a PR is ready but doesn't have a merge method set + +pr: the pr we can't stage +methods: a markdown-formatted list of valid merge methods" +runbot_merge.pr.staging.mismatch,"{pr.ping}we apparently missed updates to this PR and tried to stage it in a state which might not have been approved. + +The properties {mismatch} were not correctly synchronized and have been updated. + +
differences + +```diff +{diff}``` +
+ +Note that we are unable to check the properties {unchecked}. + +Please check and re-approve. +","Comment when staging was attempted but a sanity check revealed the github state and the mergebot state differ. + +pr: the pr we tried to stage +mismatch: comma separated list of mismatched property names +diff: patch-style view of the differing properties +unchecked: comma-separated list of properties which can't be checked" +runbot_merge.pr.staging.fail,{pr.ping}staging failed: {message},"Comment when a PR caused a staging to fail (normally only sent if the staging has a single batch, may be sent on multiple PRs depending whether the heuristic to guess the problematic PR of a batch succeeded) + +pr: the pr +message: staging failure information (error message, build link, etc...)" +runbot_merge.forwardport.updates.closed,"{pr.ping}ancestor PR {parent.display_name} has been updated but this PR is {pr.state} and can't be updated to match. + +You may want or need to manually update any followup PR.","Comment when a PR is updated and on of its followups is already merged or closed. Sent to the followup. + +pr: the closed or merged PR +parent: the modified ancestor PR" +runbot_merge.forwardport.updates.conflict.parent,"{pr.ping}WARNING: the latest change ({pr.head}) triggered a conflict when updating the next forward-port ({next.display_name}), and has been ignored. + +You will need to update this pull request differently, or fix the issue by hand on {next.display_name}.","Comment when a PR update triggers a conflict in a child. + +pr: updated parent PR +next: child PR in conflict" +runbot_merge.forwardport.updates.conflict.child,"{pr.ping}WARNING: the update of {previous.display_name} to {previous.head} has caused a conflict in this pull request, data may have been lost.{stdout}{stderr}","Comment when a PR update followup is in conflict. + +pr: PR where update followup conflict happened +previous: parent PR which triggered the followup +stdout: markdown-formatted stdout of git, if any +stderr: markdown-formatted stderr of git, if any" +runbot_merge.forwardport.update.detached,{pr.ping}this PR was modified / updated and has become a normal PR. It should be merged the normal way (via @{pr.repository.project_id.github_prefix}),"Comment when a forwardport PR gets updated, documents that the PR now needs to be merged the “normal” way. + +pr: the pr in question " +runbot_merge.forwardport.reopen.detached,{pr.ping}this PR was closed then reopened. It should be merged the normal way (via @{pr.repository.project_id.github_prefix}),"Comment when a forwardport PR gets closed then reopened, documents that the PR is now in a detached state. + +pr: the pr in question" +runbot_merge.forwardport.ci.failed,{pr.ping}{ci} failed on this forward-port PR,"Comment when CI fails on a forward-port PR (which thus won't port any further, for now). + +pr: the pr in question +ci: the failed status" +runbot_merge.forwardport.failure.discrepancy,{pr.ping}this pull request can not be forward-ported: next branch is {next!r} but linked pull request {linked.display_name} has a next branch {other!r}.,"Comment when we tried to forward port a PR batch, but the PRs have different next targets (unlikely to happen really). + +pr: the pr we tried to forward port +linked: the linked PR with a different next target +next: next target for the current pr +other: next target for the other pr" +runbot_merge.forwardport.failure.conflict,"{pr.ping}the next pull request ({new.display_name}) is in conflict. You can merge the chain up to here by saying +> @{pr.repository.project_id.fp_github_name} r+ +{footer}","Comment when a forward port was created but is in conflict, warns of that & gives instructions for current PR. + +pr: the pr which was just forward ported +new: the new forward-port +footer: some footer text" +runbot_merge.forwardport.reminder,{pr.ping}this pull request has forward-port PRs awaiting action (not merged or closed):{outstanding},"Comment when a forward port has outstanding (not merged or closed) descendants + +pr: the original (source) PR +outstanding: markdown-formatted list of outstanding PRs" +runbot_merge.forwardport.failure,"{pr.ping}cherrypicking of pull request {pr.source_id.display_name} failed. +{commits}{stdout}{stderr} +Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). + +In the former case, you may want to edit this PR message as well. +{footer}","Comment when a forward-port failed. + +pr: the new pr (in failure) +commits: markdown-formatted list of source commits, indicating which failed +stdout: git's stdout +stderr: git's stderr +footer: some footer text" +runbot_merge.forwardport.linked,"{pr.ping}while this was properly forward-ported, at least one co-dependent PR ({siblings}) did not succeed. You will need to fix it before this can be merged. + +Both this PR and the others will need to be approved via `@{pr.repository.project_id.github_prefix} r+` as they are all considered “in conflict”. +{footer} ","Comment when a forward port succeeded but at least one sibling failed. + +pr: the current pr (new) +siblings: comma-separated list of sibling links +footer: some footer text" +runbot_merge.forwardport.final,"{pr.ping}this PR targets {pr.target.name} and is the last of the forward-port chain{containing} +{ancestors} +To merge the full chain, use +> @{pr.repository.project_id.fp_github_name} r+ +{footer}","Comment when a forward port was created and is the last of a sequence (target the limit branch). + +pr: the new forward port +containing: label changing depending whether there are ancestors to merge +ancestors: markdown formatted list of parent PRs which can be approved as part of the chain +footer: a footer" +runbot_merge.forwardport.intermediate,"This PR targets {pr.target.name} and is part of the forward-port chain. Further PRs will be created up to {pr.limit_id.name}. +{footer}","Comment when a forward port was succcessfully created but is not the last of the line. + +pr: the new forward port +footer: a footer" diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 76ebb427..8e7cc1f5 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -16,6 +16,7 @@ import time from difflib import Differ from itertools import takewhile +from typing import Optional import requests import werkzeug @@ -111,15 +112,17 @@ All substitutions are tentatively applied sequentially to the input. # fetch PR object and handle as *opened* issue, pr = gh.pr(number) - feedback = self.env['runbot_merge.pull_requests.feedback'].create 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']) - feedback({ - 'repository': self.id, - 'pull_request': number, - 'message': "Branch `{}` is not within my remit, imma just ignore it.".format(pr['base']['ref']), - }) + 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 @@ -150,20 +153,13 @@ All substitutions are tentatively applied sequentially to the input. 'pull_request': pr, 'sender': {'login': self.project_id.github_prefix} }) + '. ' - feedback({ + self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr_id.repository.id, 'pull_request': number, 'message': f"{edit}. {edit2}{sync}.", }) return - feedback({ - 'repository': self.id, - 'pull_request': number, - 'message': "%sI didn't know about this PR and had to retrieve " - "its information, you may have to re-approve it as " - "I didn't see previous commands." % pr_id.ping() - }) 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 @@ -212,14 +208,21 @@ All substitutions are tentatively applied sequentially to the input. 'pull_request': pr, 'sender': sender, }) + pr_id = self.env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', pr['base']['repo']['full_name']), + ('number', '=', number), + ]) 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' + pr_id.state = 'closed' + + self.env.ref('runbot_merge.pr.load.fetched')._send( + repository=self, + pull_request=number, + format_args={'pr': pr_id}, + ) def having_branch(self, branch): branches = self.env['runbot_merge.branch'].search @@ -281,10 +284,11 @@ class Branch(models.Model): "Target branch deactivated by %r.", self.env.user.login, ) + tmpl = self.env.ref('runbot_merge.pr.branch.disabled') self.env['runbot_merge.pull_requests.feedback'].create([{ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message': f'Hey {pr.ping()}the target branch {pr.target.name!r} has been disabled, you may want to close this PR.', + 'message': tmpl._format(pr=pr), } for pr in self.prs]) return True @@ -392,11 +396,11 @@ class Branch(models.Model): reason = json.loads(str(reason))['message'].lower() pr.state = 'error' - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': f'{pr.ping()}unable to stage: {reason}', - }) + self.env.ref('runbot_merge.pr.merge.failed')._send( + repository=pr.repository, + pull_request=pr.number, + format_args= {'pr': pr, 'reason': reason, 'exc': e}, + ) else: first = False @@ -584,16 +588,16 @@ class PullRequests(models.Model): repo_name = fields.Char(related='repository.name') message_title = fields.Char(compute='_compute_message_title') - def ping(self, author=True, reviewer=True): - P = self.env['res.partner'] - s = ' '.join( - f'@{p.github_login}' - for p in (self.author if author else P) | (self.reviewed_by if reviewer else P) - if p - ) - if s: - s += ' ' - return s + ping = fields.Char(compute='_compute_ping') + + @api.depends('author.github_login', 'reviewed_by.github_login') + def _compute_ping(self): + for pr in self: + s = ' '.join( + f'@{p.github_login}' + for p in (pr.author | pr.reviewed_by ) + ) + pr.ping = s and (s + ' ') @api.depends('repository.name', 'number') def _compute_url(self): @@ -739,11 +743,11 @@ class PullRequests(models.Model): return if target and not repo.project_id._has_branch(target): - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': repo.id, - 'pull_request': number, - 'message': "I'm sorry. Branch `{}` is not within my remit.".format(target), - }) + self.env.ref('runbot_merge.pr.fetch.unmanaged')._send( + repository=repo, + pull_request=number, + format_args={'repository': repo, 'branch': target, 'number': number} + ) return pr = self.search([ @@ -825,16 +829,15 @@ class PullRequests(models.Model): ) return 'ok' - Feedback = self.env['runbot_merge.pull_requests.feedback'] if not (is_author or any(cmd == 'override' for cmd, _ in commands)): # no point even parsing commands _logger.info("ignoring comment of %s (%s): no ACL to %s", login, name, self.display_name) - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "I'm sorry, @{}. I'm afraid I can't do that.".format(login) - }) + self.env.ref('runbot_merge.command.access.no')._send( + repository=self.repository, + pull_request=self.number, + format_args={'user': login, 'pr': self} + ) return 'ignored' applied, ignored = [], [] @@ -890,11 +893,11 @@ class PullRequests(models.Model): if self.status == 'failure': # the normal infrastructure is for failure and # prefixes messages with "I'm sorry" - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "@{} you may want to rebuild or fix this PR as it has failed CI.".format(login), - }) + self.env.ref("runbot_merge.command.approve.failure")._send( + repository=self.repository, + pull_request=self.number, + format_args={'user': login, 'pr': self}, + ) elif not param and is_author: newstate = RMINUS.get(self.state) if self.priority == 0 or newstate: @@ -902,11 +905,11 @@ class PullRequests(models.Model): self.state = newstate if self.priority == 0: self.priority = 1 - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "PR priority reset to 1, as pull requests with priority 0 ignore review state.", - }) + self.env.ref("runbot_merge.command.unapprove.p0")._send( + repository=self.repository, + pull_request=self.number, + format_args={'user': login, 'pr': self}, + ) self.unstage("unreviewed (r-) by %s", login) ok = True else: @@ -938,11 +941,11 @@ class PullRequests(models.Model): self.merge_method = param ok = True explanation = next(label for value, label in type(self).merge_method.selection if value == param) - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message':"Merge method set to %s." % explanation - }) + self.env.ref("runbot_merge.command.method")._send( + repository=self.repository, + pull_request=self.number, + format_args={'new_method': explanation, 'pr': self, 'user': login}, + ) elif command == 'override': overridable = author.override_rights\ .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\ @@ -983,7 +986,7 @@ class PullRequests(models.Model): if msgs: joiner = ' ' if len(msgs) == 1 else '\n- ' msgs.insert(0, "I'm sorry, @{}:".format(login)) - Feedback.create({ + self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': self.repository.id, 'pull_request': self.number, 'message': joiner.join(msgs), @@ -1078,11 +1081,11 @@ class PullRequests(models.Model): def _notify_ci_failed(self, ci): # only report an issue of the PR is already approved (r+'d) if self.state == 'approved': - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "%s%r failed on this reviewed PR." % (self.ping(), ci), - }) + self.env.ref("runbot_merge.failure.approved")._send( + repository=self.repository, + pull_request=self.number, + format_args={'pr': self, 'status': ci} + ) def _auto_init(self): super(PullRequests, self)._auto_init() @@ -1108,11 +1111,11 @@ class PullRequests(models.Model): pr._validate(json.loads(c.statuses or '{}')) if pr.state not in ('closed', 'merged'): - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': f"[Pull request status dashboard]({pr.url}).", - }) + self.env.ref('runbot_merge.pr.created')._send( + repository=pr.repository, + pull_request=pr.number, + format_args={'pr': pr}, + ) return pr def _from_gh(self, description, author=None, branch=None, repo=None): @@ -1211,14 +1214,14 @@ class PullRequests(models.Model): unready = (prs - ready).sorted(key=lambda p: (p.repository.name, p.number)) for r in ready: - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': r.repository.id, - 'pull_request': r.number, - 'message': "{}linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( - r.ping(), - ', '.join(map('{0.display_name}'.format, unready)) - ) - }) + self.env.ref('runbot_merge.pr.linked.not_ready')._send( + repository=r.repository, + pull_request=r.number, + format_args={ + 'pr': r, + 'siblings': ', '.join(map('{0.display_name}'.format, unready)) + }, + ) r.link_warned = True if commit: self.env.cr.commit() @@ -1236,14 +1239,11 @@ class PullRequests(models.Model): ('merge_method', '=', False), ('method_warned', '=', False), ]): - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': r.repository.id, - 'pull_request': r.number, - 'message': "%sbecause this PR has multiple commits, I need to know how to merge it:\n\n%s" % ( - r.ping(), - methods, - ) - }) + self.env.ref('runbot_merge.pr.merge_method')._send( + repository=r.repository, + pull_request=r.number, + format_args={'pr': r, 'methods':methods}, + ) r.method_warned = True if commit: self.env.cr.commit() @@ -1663,6 +1663,33 @@ class Feedback(models.Model): to_remove.append(f.id) self.browse(to_remove).unlink() +class FeedbackTemplate(models.Model): + _name = 'runbot_merge.pull_requests.feedback.template' + _description = "str.format templates for feedback messages, no integration," \ + "but that's their purpose" + _inherit = ['mail.thread'] + + template = fields.Text(tracking=True) + help = fields.Text(readonly=True) + + def _format(self, **args): + return self.template.format_map(args) + + def _send(self, *, repository: Repository, pull_request: int, format_args: dict, token_field: Optional[str] = None) -> Optional[Feedback]: + try: + feedback = { + 'repository': repository.id, + 'pull_request': pull_request, + 'message': self.template.format_map(format_args), + } + if token_field: + feedback['token_field'] = token_field + return self.env['runbot_merge.pull_requests.feedback'].create(feedback) + except Exception: + _logger.exception("Failed to render template %s", self.get_external_id()) + raise + return None + class Commit(models.Model): """Represents a commit onto which statuses might be posted, independent of everything else as commits can be created by @@ -1900,11 +1927,11 @@ class Stagings(models.Model): prs = prs or self.batch_ids.prs prs.write({'state': 'error'}) for pr in prs: - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': "%sstaging failed: %s" % (pr.ping(), message), - }) + self.env.ref('runbot_merge.pr.staging.fail')._send( + repository=pr.repository, + pull_request=pr.number, + format_args={'pr': pr, 'message': message}, + ) self.batch_ids.write({'active': False}) self.write({ @@ -2213,31 +2240,16 @@ class Batch(models.Model): "data mismatch on %s:\n%s", pr.display_name, diff ) - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': """\ -{ping}we apparently missed updates to this PR and tried to stage it in a state \ -which might not have been approved. - -The properties {mismatch} were not correctly synchronized and have been updated. - -
differences - -```diff -{diff}``` -
- -Note that we are unable to check the properties {unchecked}. - -Please check and re-approve. -""".format( - ping=pr.ping(), - mismatch=', '.join(pr_fields[f].string for f in e.args[0]), - diff=diff, - unchecked=', '.join(pr_fields[f].string for f in UNCHECKABLE), -) - }) + self.env.ref('runbot_merge.pr.staging.mismatch')._send( + repository=pr.repository, + pull_request=pr.number, + format_args={ + 'pr': pr, + 'mismatch': ', '.join(pr_fields[f].string for f in e.args[0]), + 'diff': diff, + 'unchecked': ', '.join(pr_fields[f].string for f in UNCHECKABLE) + } + ) return self.env['runbot_merge.batch'] # update meta to new heads diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index a6cb79ba..006f59de 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -25,3 +25,4 @@ access_runbot_merge_pull_requests,User access to PR,model_runbot_merge_pull_requ access_runbot_merge_pull_requests_feedback,Users have no reason to access feedback,model_runbot_merge_pull_requests_feedback,,0,0,0,0 access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_review,base.group_user,1,0,0,0 access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0 +runbot_merge.access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index c15925b1..305a79e2 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -3212,10 +3212,10 @@ class TestUnknownPR: seen(env, prx, users), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), - (users['user'], "I didn't know about this PR and had to " + seen(env, prx, users), + (users['user'], f"@{users['user']} @{users['reviewer']} I didn't know about this PR and had to " "retrieve its information, you may have to " "re-approve it as I didn't see previous commands."), - seen(env, prx, users), ] pr = env['runbot_merge.pull_requests'].search([ @@ -3265,10 +3265,13 @@ class TestUnknownPR: assert pr.comments == [ seen(env, pr, users), (users['reviewer'], 'hansen r+'), - (users['user'], "I didn't know about this PR and had to retrieve " + seen(env, pr, users), + # reviewer is set because fetch replays all the comments (thus + # setting r+ and reviewer) but then syncs the head commit thus + # unsetting r+ but leaving the reviewer + (users['user'], f"@{users['user']} @{users['reviewer']} I didn't know about this PR and had to retrieve " "its information, you may have to re-approve it " "as I didn't see previous commands."), - seen(env, pr, users), ] def test_rplus_unmanaged(self, env, repo, users, config): diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index f56ea125..2cff73ce 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -63,7 +63,7 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf assert pr.comments == [ (users['reviewer'], "hansen r+"), seen(env, pr, users), - (users['user'], "Hey @%(user)s @%(reviewer)s the target branch 'other' has been disabled, you may want to close this PR." % users), + (users['user'], "@%(user)s @%(reviewer)s the target branch 'other' has been disabled, you may want to close this PR." % users), ] with repo: diff --git a/runbot_merge/views/configuration.xml b/runbot_merge/views/configuration.xml index 70e8d710..32d3f3e7 100644 --- a/runbot_merge/views/configuration.xml +++ b/runbot_merge/views/configuration.xml @@ -33,11 +33,42 @@
- + + Feedback Templates tree + runbot_merge.pull_requests.feedback.template + + + Feedback Templates + runbot_merge.pull_requests.feedback.template + + + + + + + + + Feedback Templates form + runbot_merge.pull_requests.feedback.template + +
+ + + + +
+ +
+
+
+
+ + + + From cd4ded899b56ded376824a117bfaa444b39f8c92 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 13 Jun 2023 14:22:40 +0200 Subject: [PATCH 009/192] [IMP] runbot_merge: error reporting Largely informed by sentry, - Fix an inconsistency in staging ref verification, `set_ref` internally waits for the observed head to match the requested head, but then `try_staging` would re-check that and immediately fail if it didn't. Because github is *eventually* consistent (hopefully) this second check can fail (and is also an extra API call), breaking staging unnecessarily, especially as we're still going to wait for the update to be visible to git. Remove this redundant check entirely, as github provides no way to ensure we have a consistent view of anything, it doesn't have much value and can do much harm. - Add github request id to one of the sanity check warnings as that could be a useful thing to send upstream, missing github request ids in the future should be noted and added. - Reworked the GH object's calls to be clearer and more coherent: consistently log the same thing on all GH errors (if `check`), rather than just on the one without a `check` entry. Also remove `raise_for_status` and raise `HTTPError` by hand every time we hit a status >= 400, so we always forward the response body no matter what its type is. - Try again to log the request body (in full as it should be pretty small), also remove stripping since we specifically wanted to add a newline at the start, I've no idea what I was thinking. Fixes #735, #764, #544 --- runbot_merge/github.py | 60 ++++++++++++++-------------- runbot_merge/models/pull_requests.py | 7 ---- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 27cf6305..4f2b6fc8 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -1,12 +1,11 @@ import collections.abc import itertools -import json as json_ +import json import logging import logging.handlers import os import pathlib import pprint -import textwrap import unicodedata import requests @@ -47,7 +46,7 @@ def _init_gh_logger(): if odoo.netsvc._logger_init: _init_gh_logger() -GH_LOG_PATTERN = """=> {method} /{self._repo}/{path}{qs}{body} +GH_LOG_PATTERN = """=> {method} /{path}{qs}{body} <= {r.status_code} {r.reason} {headers} @@ -62,7 +61,7 @@ class GH(object): session.headers['Authorization'] = 'token {}'.format(token) session.headers['Accept'] = 'application/vnd.github.symmetra-preview+json' - def _log_gh(self, logger, method, path, params, json, response, level=logging.INFO): + def _log_gh(self, logger: logging.Logger, response: requests.Response, level: int = logging.INFO, extra=None): """ Logs a pair of request / response to github, to the specified logger, at the specified level. @@ -70,11 +69,14 @@ class GH(object): bodies, at least in part) so we have as much information as possible for post-mortems. """ - body = body2 = '' + req = response.request + url = werkzeug.urls.url_parse(req.url) + if url.netloc is not 'api.github.com': + return - if json: - body = '\n' + textwrap.indent('\t', pprint.pformat(json, indent=4)) + body = '' if not req.body else ('\n' + pprint.pformat(json.loads(req.body.decode()), indent=4)) + body2 = '' if response.content: if _is_json(response): body2 = pprint.pformat(response.json(), depth=4) @@ -87,19 +89,15 @@ class GH(object): ) logger.log(level, GH_LOG_PATTERN.format( - self=self, # requests data - method=method, path=path, - qs='' if not params else ('?' + werkzeug.urls.url_encode(params)), - body=utils.shorten(body.strip(), 400), + method=req.method, path=url.path, qs=url.query, body=body, # response data r=response, headers='\n'.join( '\t%s: %s' % (h, v) for h, v in response.headers.items() ), body2=utils.shorten(body2.strip(), 400) - )) - return body2 + ), extra=extra) def __call__(self, method, path, params=None, json=None, check=True): """ @@ -107,21 +105,21 @@ class GH(object): """ path = f'/repos/{self._repo}/{path}' r = self._session.request(method, self._url + path, params=params, json=json) - self._log_gh(_gh, method, path, params, json, r) + self._log_gh(_gh, r) if check: - if isinstance(check, collections.abc.Mapping): - exc = check.get(r.status_code) - if exc: - raise exc(r.text) - if r.status_code >= 400: - body = self._log_gh( - _logger, method, path, params, json, r, level=logging.ERROR) - if not isinstance(body, (bytes, str)): - raise requests.HTTPError( - json_.dumps(body, indent=4), - response=r - ) - r.raise_for_status() + try: + if isinstance(check, collections.abc.Mapping): + exc = check.get(r.status_code) + if exc: + raise exc(r.text) + if r.status_code >= 400: + raise requests.HTTPError(r.text, response=r) + except Exception: + self._log_gh(_logger, r, level=logging.ERROR, extra={ + 'github-request-id': r.headers.get('x-github-request-id'), + }) + raise + return r def user(self, username): @@ -180,13 +178,17 @@ class GH(object): if r.status_code == 200: head = r.json()['object']['sha'] else: - head = '' % (r.status_code, r.json() if _is_json(r) else r.text) + head = '' % (r.status_code, r.text) if head == to: _logger.debug("Sanity check ref update of %s to %s: ok", branch, to) return - _logger.warning("Sanity check ref update of %s, expected %s got %s", branch, to, head) + _logger.warning( + "Sanity check ref update of %s, expected %s got %s (response-id %s)", + branch, to, head, + r.headers.get('x-github-request-id') + ) return head def fast_forward(self, branch, sha): diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8e7cc1f5..a25b3cd2 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -463,13 +463,6 @@ For-Commit-Id: %s ) refname = 'staging.{}'.format(self.name) it['gh'].set_ref(refname, staging_head) - # asserts that the new head is visible through the api - head = it['gh'].head(refname) - assert head == staging_head,\ - "[api] updated %s:%s to %s but found %s" % ( - r.name, refname, - staging_head, head, - ) i = itertools.count() @utils.backoff(delays=WAIT_FOR_VISIBILITY, exc=TimeoutError) From 06a3a1bab57ad4cf72409d4823776d4e43da2195 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 14 Jun 2023 13:34:22 +0200 Subject: [PATCH 010/192] [IMP] runbot_merge: add sentry filtering, rework some error messages - move sentry configuration and add exception-based filtering - clarify and reclassify (e.g. from warning to info) a few messages - convert assertions in rebase to MergeError so they can be correctly logged & reported, and ignored by sentry, also clarify them (especially the consistency one) Related to #544 --- runbot_merge/__init__.py | 38 +------------ runbot_merge/controllers/__init__.py | 16 +++--- runbot_merge/github.py | 22 +++++--- runbot_merge/models/pull_requests.py | 13 ++--- runbot_merge/sentry.py | 80 ++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 59 deletions(-) create mode 100644 runbot_merge/sentry.py diff --git a/runbot_merge/__init__.py b/runbot_merge/__init__.py index f4821304..31b308ad 100644 --- a/runbot_merge/__init__.py +++ b/runbot_merge/__init__.py @@ -1,41 +1,5 @@ -import logging -from os import environ - -import sentry_sdk -from sentry_sdk.integrations.logging import LoggingIntegration -from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware - -from odoo import http from . import models, controllers - -def delegate(self, attr): - return getattr(self.app, attr) -SentryWsgiMiddleware.__getattr__ = delegate - -def enable_sentry(): - logger = logging.getLogger('runbot_merge') - - dsn = environ.get('SENTRY_DSN') - if not dsn: - logger.info("No DSN found, skipping sentry...") - return - - try: - sentry_sdk.init( - dsn, - integrations=[ - # note: if the colorformatter is enabled, sentry gets lost - # and classifies everything as errors because it fails to - # properly classify levels as the colorformatter injects - # the ANSI color codes right into LogRecord.levelname - LoggingIntegration(level=logging.INFO, event_level=logging.WARNING), - ] - ) - http.root = SentryWsgiMiddleware(http.root) - except Exception: - logger.exception("DSN found, failed to enable sentry...") - else: - logger.info("DSN found, sentry enabled...") +from .sentry import enable_sentry def _check_citext(cr): cr.execute("select 1 from pg_extension where extname = 'citext'") diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 51d8c71d..4781207d 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -235,14 +235,14 @@ def handle_pr(env, event): oldstate, ) return 'Closed {}'.format(pr_obj.display_name) - 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)' + + _logger.info( + '%s tried to close %s (state=%s) but locking failed', + event['sender']['login'], + pr_obj.display_name, + oldstate, + ) + return 'Ignored: could not lock rows (probably being merged)' if event['action'] == 'reopened' : if pr_obj.state == 'merged': diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 4f2b6fc8..e5e5fa9e 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -275,7 +275,7 @@ class GH(object): try: r = r.json() except Exception: - raise MergeError("Got non-JSON reponse from github: %s %s (%s)" % (r.status_code, r.reason, r.text)) + raise MergeError("got non-JSON reponse from github: %s %s (%s)" % (r.status_code, r.reason, r.text)) _logger.debug( "merge(%s, %s (%s), %s) -> %s", self._repo, dest, r['parents'][0]['sha'], @@ -297,10 +297,17 @@ class GH(object): logger.debug("rebasing %s, %s on %s (reset=%s, commits=%s)", self._repo, pr, dest, reset, len(commits)) - assert commits, "can't rebase a PR with no commits" + if not commits: + raise MergeError("PR has no commits") prev = original_head for original in commits: - assert len(original['parents']) == 1, "can't rebase commits with more than one parent" + if len(original['parents']) != 1: + raise MergeError( + "commits with multiple parents ({sha}) can not be rebased, " + "either fix the branch to remove merges or merge without " + "rebasing".format_map( + original + )) tmp_msg = 'temp rebasing PR %s (%s)' % (pr, original['sha']) merged = self.merge(original['sha'], dest, tmp_msg) @@ -309,9 +316,12 @@ class GH(object): # expect (either original_head or the previously merged commit) [base_commit] = (parent['sha'] for parent in merged['parents'] if parent['sha'] != original['sha']) - assert prev == base_commit,\ - "Inconsistent view of %s between head (%s) and merge (%s)" % ( - dest, prev, base_commit, + if prev != base_commit: + raise MergeError( + f"Inconsistent view of branch {dest} while rebasing " + f"PR {pr} expected commit {prev} but the other parent of " + f"merge commit {merged['sha']} is {base_commit}.\n\n" + f"The branch may be getting concurrently modified." ) prev = merged['sha'] original['new_tree'] = merged['tree']['sha'] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index a25b3cd2..c2ccb47a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -388,7 +388,7 @@ class Branch(models.Model): if len(e.args) > 1 and e.args[1]: reason = e.args[1] else: - reason = e.__context__ + reason = e.__cause__ or e.__context__ # if the reason is a json document, assume it's a github # error and try to extract the error message to give it to # the user @@ -1588,7 +1588,7 @@ class Tagging(models.Model): try: gh.change_tags(pr, tags_remove, tags_add) except Exception: - _logger.exception( + _logger.info( "Error while trying to change the tags of %s#%s from %s to %s", repo.name, pr, remove, add, ) @@ -2211,8 +2211,8 @@ class Batch(models.Model): it = meta[to_revert.repository] it['gh'].set_ref('tmp.{}'.format(to_revert.target.name), it['head']) raise - except github.MergeError: - raise exceptions.MergeError(pr) + except github.MergeError as e: + raise exceptions.MergeError(pr) from e except exceptions.Mismatch as e: def format_items(items): """ Bit of a pain in the ass because difflib really wants @@ -2229,10 +2229,7 @@ class Batch(models.Model): old = list(format_items((n, str(v)) for n, v, _ in e.args[1])) new = list(format_items((n, str(v)) for n, _, v in e.args[1])) diff = ''.join(Differ().compare(old, new)) - _logger.warning( - "data mismatch on %s:\n%s", - pr.display_name, diff - ) + _logger.info("data mismatch on %s:\n%s", pr.display_name, diff) self.env.ref('runbot_merge.pr.staging.mismatch')._send( repository=pr.repository, pull_request=pr.number, diff --git a/runbot_merge/sentry.py b/runbot_merge/sentry.py new file mode 100644 index 00000000..55f3214c --- /dev/null +++ b/runbot_merge/sentry.py @@ -0,0 +1,80 @@ +import logging +from os import environ + +import sentry_sdk +from sentry_sdk.integrations.logging import LoggingIntegration +from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware + +from odoo import http +from runbot_merge.exceptions import FastForwardError, Mismatch, MergeError, Unmergeable + + +def delegate(self, attr): + return getattr(self.app, attr) +SentryWsgiMiddleware.__getattr__ = delegate + +def enable_sentry(): + logger = logging.getLogger('runbot_merge') + + dsn = environ.get('SENTRY_DSN') + if not dsn: + logger.info("No DSN found, skipping sentry...") + return + + try: + setup_sentry(dsn) + except Exception: + logger.exception("DSN found, failed to enable sentry...") + else: + logger.info("DSN found, sentry enabled...") + + +def setup_sentry(dsn): + sentry_sdk.init( + dsn, + traces_sample_rate=1.0, + integrations=[ + # note: if the colorformatter is enabled, sentry gets lost + # and classifies everything as errors because it fails to + # properly classify levels as the colorformatter injects + # the ANSI color codes right into LogRecord.levelname + LoggingIntegration(level=logging.INFO, event_level=logging.WARNING), + ], + before_send=event_filter, + ) + http.root = SentryWsgiMiddleware(http.root) + +dummy_record = logging.LogRecord(name="", level=logging.NOTSET, pathname='', lineno=0, msg='', args=(), exc_info=None) +# mapping of exception types to predicates, if the predicate returns `True` the +# exception event should be suppressed +SUPPRESS_EXCEPTION = { + # Someone else deciding to push directly to the branch (which is generally + # what leads to this error) is not really actionable. + # + # Other possibilities are more structural and thus we probably want to know: + # - other 422 Unprocessable github errors (likely config issues): + # - reference does not exist + # - object does not exist + # - object is not a commit + # - branch protection issue + # - timeout on ref update (github probably dying) + # - other HTTP error (also github probably dying) + # + # might be worth using richer exceptions to make this clearer, and easier to classify + FastForwardError: lambda e: 'not a fast forward' in str(e.__cause__), + # Git conflict when merging (or non-json response which is weird), + # notified on PR + MergeError: lambda _: True, + # Failed preconditions on merging, notified on PR + Unmergeable: lambda _: True, +} +def event_filter(event, hint): + # event['level'], event['logger'], event['logentry'], event['exception'] + # known hints: log_record: LogRecord, exc_info: (type, BaseExeption, Traceback) | None + exc_info = hint.get('exc_info') or hint.get('log_record', dummy_record).exc_info + if exc_info: + etype, exc, _ = exc_info + if SUPPRESS_EXCEPTION.get(etype, lambda _: False)(exc): + return None + + From ed0fd88854be57826fe8463335338de5946a59a9 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 15 Jun 2023 08:22:38 +0200 Subject: [PATCH 011/192] [ADD] runbot_merge: sentry instrumentation Currently sentry is only hooked from the outside, which doesn't necessarily provide sufficiently actionable information. Add some a few hooks to (try and) report odoo / mergebot metadata: - add the user to WSGI transactions - add a transaction (with users) around crons - add the webhook event info to webhook requests - add a few spans to the long-running crons, when they cover multiple units per iteration (e.g. a span per branch being staged) Closes #544 --- forwardport/models/forwardport.py | 8 +++++- runbot_merge/controllers/__init__.py | 9 +++++++ runbot_merge/models/project.py | 6 ++++- runbot_merge/models/pull_requests.py | 6 ++++- runbot_merge/sentry.py | 38 +++++++++++++++++++++++++++- 5 files changed, 63 insertions(+), 4 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 78c23a46..6e3e0c24 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -1,6 +1,9 @@ # -*- coding: utf-8 -*- import logging import pathlib + +import sentry_sdk + import resource import subprocess import uuid @@ -28,7 +31,8 @@ class Queue: def _process(self): for b in self.search(self._search_domain(), order='create_date, id', limit=self.limit): try: - b._process_item() + with sentry_sdk.start_span(description=self._name): + b._process_item() b.unlink() self.env.cr.commit() except Exception: @@ -68,6 +72,7 @@ class ForwardPortTasks(models.Model, Queue): def _process_item(self): batch = self.batch_id + sentry_sdk.set_tag('forward-porting', batch.prs.mapped('display_name')) newbatch = batch.prs._port_forward() if newbatch: @@ -109,6 +114,7 @@ class UpdateQueue(models.Model, Queue): def _process_item(self): previous = self.new_root + sentry_sdk.set_tag("update-root", self.new_root.display_name) with ExitStack() as s: for child in self.new_root._iter_descendants(): self.env.cr.execute(""" diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 4781207d..2f9fbc20 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -3,6 +3,7 @@ import hmac import logging import json +import sentry_sdk import werkzeug.exceptions from odoo.http import Controller, request, route @@ -18,6 +19,13 @@ class MergebotController(Controller): def index(self): req = request.httprequest event = req.headers['X-Github-Event'] + with sentry_sdk.configure_scope() as scope: + if scope.transaction: + # only in 1.8.0 (or at least 1.7.2 + if hasattr(scope, 'set_transaction_name'): + scope.set_transaction_name(f"webhook {event}") + else: # but our servers use 1.4.3 + scope.transaction = f"webhook {event}" github._gh.info(self._format(req)) @@ -39,6 +47,7 @@ class MergebotController(Controller): req.headers.get('X-Hub-Signature')) return werkzeug.exceptions.Forbidden() + sentry_sdk.set_context('webhook', request.jsonrequest) return c(env, request.jsonrequest) def _format(self, request): diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index 883c91c6..8578aeb5 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -1,6 +1,8 @@ import logging import re +import sentry_sdk + from odoo import models, fields _logger = logging.getLogger(__name__) @@ -69,7 +71,9 @@ class Project(models.Model): ('staging_enabled', '=', True), ]): try: - with self.env.cr.savepoint(): + with self.env.cr.savepoint(), \ + sentry_sdk.start_span(description=f'create staging {branch.name}') as span: + span.set_tag('branch', branch.name) branch.try_staging() except Exception: _logger.exception("Failed to create staging for branch %r", branch.name) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index c2ccb47a..19c63aef 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -19,6 +19,7 @@ from itertools import takewhile from typing import Optional import requests +import sentry_sdk import werkzeug from werkzeug.datastructures import Headers @@ -2031,7 +2032,10 @@ class Stagings(models.Model): FOR UPDATE ''', [tuple(self.mapped('batch_ids.prs.id'))]) try: - self._safety_dance(gh, staging_heads) + with sentry_sdk.start_span(description="merge staging") as span: + span.set_tag("staging", self.id) + span.set_tag("branch", self.target.name) + self._safety_dance(gh, staging_heads) except exceptions.FastForwardError as e: logger.warning( "Could not fast-forward successful staging on %s:%s", diff --git a/runbot_merge/sentry.py b/runbot_merge/sentry.py index 55f3214c..9c249e9e 100644 --- a/runbot_merge/sentry.py +++ b/runbot_merge/sentry.py @@ -6,7 +6,10 @@ from sentry_sdk.integrations.logging import LoggingIntegration from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware from odoo import http -from runbot_merge.exceptions import FastForwardError, Mismatch, MergeError, Unmergeable +from odoo.addons.base.models.ir_cron import ir_cron +from odoo.http import WebRequest + +from .exceptions import FastForwardError, Mismatch, MergeError, Unmergeable def delegate(self, attr): @@ -41,8 +44,41 @@ def setup_sentry(dsn): LoggingIntegration(level=logging.INFO, event_level=logging.WARNING), ], before_send=event_filter, + # apparently not in my version of the sdk + # functions_to_trace = [] ) http.root = SentryWsgiMiddleware(http.root) + instrument_odoo() + +def instrument_odoo(): + """Monkeypatches odoo core to copy odoo metadata into sentry for more + informative events + """ + # add user to wsgi request context + old_call_function = WebRequest._call_function + def _call_function(self, *args, **kwargs): + if self.uid: + sentry_sdk.set_user({ + 'id': self.uid, + 'email': self.env.user.email, + 'username': self.env.user.login, + }) + else: + sentry_sdk.set_user({'username': ''}) + return old_call_function(self, *args, **kwargs) + WebRequest._call_function = _call_function + + # create transaction for tracking crons, add user to that + old_callback = ir_cron._callback + def _callback(self, cron_name, server_action_id, job_id): + sentry_sdk.start_transaction(name=f"cron {cron_name}") + sentry_sdk.set_user({ + 'id': self.env.user.id, + 'email': self.env.user.email, + 'username': self.env.user.login, + }) + return old_callback(self, cron_name, server_action_id, job_id) + ir_cron._callback = _callback dummy_record = logging.LogRecord(name="", level=logging.NOTSET, pathname='', lineno=0, msg='', args=(), exc_info=None) # mapping of exception types to predicates, if the predicate returns `True` the From 9260384284cb58514594dd2c15babf53149a6858 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 15 Jun 2023 15:25:17 +0200 Subject: [PATCH 012/192] [FIX] runbot_merge: concurrency error in freeze wizard (hopefully) During the 16.3 freeze an issue was noticed with the concurrency safety of the freeze wizard (because it blew up, which caused a few issues): it is possible for the cancelling of an active staging to the master branch to fail, which causes the mergebot side of the freeze to fail, but the github state is completed, which puts the entire thing in a less than ideal state. Especially with the additional issue that the branch inserter has its own concurrency issue (which maybe I should fix): if there are branches *being* forward-ported across the new branch, it's unable to see them, and thus can not create the now-missing PRs. Try to make the freeze wizard more resilient: 1. Take a lock on the master staging (if any) early on, this means if we can acquire it we should be able to cancel it, and it won't suffer a concurrency error. 2. Add the `process_updated_commits` cron to the set of locked crons, trying to read the log timeline it looks like the issue was commits being impacted on that staging while the action had started: REPEATABLE READ meant the freeze's transaction was unable to see the update from the commit statuses, therefore creating a diverging update when it cancelled the staging, which postgres then reported as a serialization error. I'd like to relax the locking of the cron (to just FOR SHARE), but I think it would work, per postgres: > SELECT FOR UPDATE, and SELECT FOR SHARE commands behave the same as > SELECT in terms of searching for target rows: they will only find > target rows that were committed as of the transaction start > time. However, such a target row might have already been updated (or > deleted or locked) by another concurrent transaction by the time it > is found. In this case, the repeatable read transaction will wait > for the first updating transaction to commit or roll back (if it is > still in progress). If the first updater rolls back, then its > effects are negated and the repeatable read transaction can proceed > with updating the originally found row. But if the first updater > commits (and actually updated or deleted the row, not just locked > it) then the repeatable read transaction will be rolled back with > the message This means it would be possible to lock the cron, and then get a transaction error because the cron modified one of the records we're going to hit while it was running: as far as the above is concerned the cron's worker had "just locked" the row so it's fine to continue. However this makes it more and more likely an error will be hit when trying to freeze (to no issue, but still). We'll have to see how that ends up. Fixes #766 maybe --- runbot_merge/models/project_freeze/__init__.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index c6bc4330..b2c35d9e 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -177,7 +177,9 @@ class FreezeWizard(models.Model): if self.errors: return self.action_open() - conflict_crons = self.env.ref('runbot_merge.merge_cron') | self.env.ref('runbot_merge.staging_cron') + conflict_crons = self.env.ref('runbot_merge.merge_cron')\ + | self.env.ref('runbot_merge.staging_cron')\ + | self.env.ref('runbot_merge.process_updated_commits') # we don't want to run concurrently to the crons above, though we # don't need to prevent read access to them self.env.cr.execute( @@ -190,6 +192,12 @@ class FreezeWizard(models.Model): # everything so the new branch is the second one, just after the branch # it "forks" master, rest = project_id.branch_ids[0], project_id.branch_ids[1:] + if self.bump_pr_ids and master.active_staging_id: + self.env.cr.execute( + 'SELECT * FROM runbot_merge_stagings WHERE id = %s FOR UPDATE NOWAIT', + [master.active_staging_id] + ) + seq = itertools.count(start=1) # start reseq at 1 commands = [ (1, master.id, {'sequence': next(seq)}), From 765281a66500a84f0bbf2e78fe79175b623c5d8f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 21 Jun 2023 14:24:58 +0200 Subject: [PATCH 013/192] [FIX] runbot_merge: make provisioning more resilient A few cases of conflict were missing from the provisioning handler. They can't really be auto-fixed, so just output a warning and ignore the entry, that way the rest of the provisioning succeeds. --- .../controllers/reviewer_provisioning.py | 32 +++++++++++-- runbot_merge/tests/test_provisioning.py | 48 ++++++++++++------- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index e0aa0fc7..35f07d24 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/runbot_merge/controllers/reviewer_provisioning.py @@ -35,6 +35,11 @@ class MergebotReviewerProvisioning(Controller): Partners = env['res.partner'] Users = env['res.users'] + existing_logins = set() + existing_oauth = set() + for u in Users.with_context(active_test=False).search([]): + existing_logins.add(u.login) + existing_oauth .add((u.oauth_provider_id.id, u.oauth_uid)) existing_partners = Partners.with_context(active_test=False).search([ '|', ('email', 'in', [u['email'] for u in users]), ('github_login', 'in', [u['github_login'] for u in users]) @@ -76,9 +81,30 @@ class MergebotReviewerProvisioning(Controller): to_activate |= current # entry doesn't have user -> create user if not current.user_ids: - # skip users without an email (= login) as that - # fails if not new['email']: + _logger.info( + "Unable to create user for %s: no email in provisioning data", + current.display_name + ) + continue + if 'oauth_uid' in new: + if (new['oauth_provider_id'], new['oauth_uid']) in existing_oauth: + _logger.warning( + "Attempted to create user with duplicate oauth uid " + "%s with provider %r for provisioning entry %r. " + "There is likely a duplicate partner (one version " + "with email, one with github login)", + new['oauth_uid'], odoo_provider.display_name, new, + ) + continue + if new['email'] in existing_logins: + _logger.warning( + "Attempted to create user with duplicate login %s for " + "provisioning entry %r. There is likely a duplicate " + "partner (one version with email, one with github " + "login)", + new['email'], new, + ) continue new['login'] = new['email'] @@ -93,7 +119,7 @@ class MergebotReviewerProvisioning(Controller): # otherwise update user (if there is anything to update) user = current.user_ids if len(user) != 1: - _logger.warning("Got %d users for partner %s.", len(user), current.display_name) + _logger.warning("Got %d users for partner %s, updating first.", len(user), current.display_name) user = user[:1] new.setdefault("active", True) update_vals = { diff --git a/runbot_merge/tests/test_provisioning.py b/runbot_merge/tests/test_provisioning.py index af939305..db26e5e7 100644 --- a/runbot_merge/tests/test_provisioning.py +++ b/runbot_merge/tests/test_provisioning.py @@ -1,4 +1,3 @@ -import pytest import requests GEORGE = { @@ -30,24 +29,13 @@ def test_basic_provisioning(env, port): r = provision_user(port, [dict(GEORGE, name="x", github_login="y", sub="42")]) assert r == [0, 1] - # can't fail anymore because github_login now used to look up the existing - # user - # with pytest.raises(Exception): - # provision_user(port, [{ - # 'name': "other@example.org", - # 'email': "x", - # 'github_login': "y", - # 'sub': "42" - # }]) - r = provision_user(port, [dict(GEORGE, active=False)]) assert r == [0, 1] assert not env['res.users'].search([('login', '=', GEORGE['email'])]) assert env['res.partner'].search([('email', '=', GEORGE['email'])]) def test_upgrade_partner(env, port): - # If a partner exists for a github login (and / or email?) it can be - # upgraded by creating a user for it + # matching partner with an email but no github login p = env['res.partner'].create({ 'name': GEORGE['name'], 'email': GEORGE['email'], @@ -64,6 +52,7 @@ def test_upgrade_partner(env, port): p.user_ids.unlink() p.unlink() + # matching partner with a github login but no email p = env['res.partner'].create({ 'name': GEORGE['name'], 'github_login': GEORGE['github_login'], @@ -77,14 +66,14 @@ def test_upgrade_partner(env, port): 'email': GEORGE['email'], }] - # deactivate user to see if provisioning re-enables them + # matching partner with a deactivated user p.user_ids.active = False r = provision_user(port, [GEORGE]) assert r == [0, 1] assert len(p.user_ids) == 1, "provisioning should re-enable user" assert p.user_ids.active - # re-enable both user and partner to see if both get re-enabled + # matching deactivated partner (with a deactivated user) p.user_ids.active = False p.active = False r = provision_user(port, [GEORGE]) @@ -92,6 +81,33 @@ def test_upgrade_partner(env, port): assert p.active, "provisioning should re-enable partner" assert p.user_ids.active +def test_duplicates(env, port): + """In case of duplicate data, the handler should probably not blow up, but + instead log a warning (so the data gets fixed eventually) and skip + """ + # dupe 1: old oauth signup account & github interaction account, provisioning + # prioritises the github account & tries to create a user for it, which + # fails because the signup account has the same oauth uid (probably) + env['res.partner'].create({'name': 'foo', 'github_login': 'foo'}) + env['res.users'].create({'login': 'foo@example.com', 'name': 'foo', 'email': 'foo@example.com', 'oauth_provider_id': 1, 'oauth_uid': '42'}) + assert provision_user(port, [{ + 'name': "foo", + 'email': 'foo@example.com', + 'github_login': 'foo', + 'sub': '42' + }]) == [0, 0] + + # dupe 2: old non-oauth signup account & github interaction account, same + # as previous except it breaks on the login instead of the oauth_uid + env['res.partner'].create({'name': 'bar', 'github_login': 'bar'}) + env['res.users'].create({'login': 'bar@example.com', 'name': 'bar', 'email': 'bar@example.com'}) + assert provision_user(port, [{ + 'name': "bar", + 'email': 'bar@example.com', + 'github_login': 'bar', + 'sub': '43' + }]) == [0, 0] + def test_no_email(env, port): """ Provisioning system should ignore email-less entries """ @@ -182,6 +198,6 @@ def provision_user(port, users): }) r.raise_for_status() json = r.json() - assert 'error' not in json + assert 'error' not in json, json['error']['data']['debug'] return json['result'] From fc41e09f44f33ddec91ab8ef9f75a13988943cc9 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Wed, 21 Jun 2023 10:08:14 +0200 Subject: [PATCH 014/192] [IMP] runbot: add useful indexes remaining from upgrade --- runbot/models/batch.py | 2 +- runbot/models/branch.py | 4 ++-- runbot/models/build.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/runbot/models/batch.py b/runbot/models/batch.py index 6c5f4d01..eaea5f39 100644 --- a/runbot/models/batch.py +++ b/runbot/models/batch.py @@ -22,7 +22,7 @@ class Batch(models.Model): state = fields.Selection([('preparing', 'Preparing'), ('ready', 'Ready'), ('done', 'Done'), ('skipped', 'Skipped')]) hidden = fields.Boolean('Hidden', default=False) age = fields.Integer(compute='_compute_age', string='Build age') - category_id = fields.Many2one('runbot.category', default=lambda self: self.env.ref('runbot.default_category', raise_if_not_found=False)) + category_id = fields.Many2one('runbot.category', index=True, default=lambda self: self.env.ref('runbot.default_category', raise_if_not_found=False)) log_ids = fields.One2many('runbot.batch.log', 'batch_id') has_warning = fields.Boolean("Has warning") base_reference_batch_id = fields.Many2one('runbot.batch') diff --git a/runbot/models/branch.py b/runbot/models/branch.py index daa7f64c..80079bd5 100644 --- a/runbot/models/branch.py +++ b/runbot/models/branch.py @@ -17,7 +17,7 @@ class Branch(models.Model): _sql_constraints = [('branch_repo_uniq', 'unique (name,remote_id)', 'The branch must be unique per repository !')] name = fields.Char('Name', required=True) - remote_id = fields.Many2one('runbot.remote', 'Remote', required=True, ondelete='cascade') + remote_id = fields.Many2one('runbot.remote', 'Remote', required=True, ondelete='cascade', index=True) head = fields.Many2one('runbot.commit', 'Head Commit', index=True) head_name = fields.Char('Head name', related='head.name', store=True) @@ -29,7 +29,7 @@ class Branch(models.Model): pr_title = fields.Char('Pr Title') pr_body = fields.Char('Pr Body') pr_author = fields.Char('Pr Author') - + pull_head_name = fields.Char(compute='_compute_branch_infos', string='PR HEAD name', readonly=1, store=True) pull_head_remote_id = fields.Many2one('runbot.remote', 'Pull head repository', compute='_compute_branch_infos', store=True, index=True) target_branch_name = fields.Char(compute='_compute_branch_infos', string='PR target branch', store=True) diff --git a/runbot/models/build.py b/runbot/models/build.py index 468ea9e1..0e26c4c3 100644 --- a/runbot/models/build.py +++ b/runbot/models/build.py @@ -51,7 +51,7 @@ class BuildParameters(models.Model): version_id = fields.Many2one('runbot.version', required=True, index=True) project_id = fields.Many2one('runbot.project', required=True, index=True) # for access rights trigger_id = fields.Many2one('runbot.trigger', index=True) # for access rights - create_batch_id = fields.Many2one('runbot.batch') + create_batch_id = fields.Many2one('runbot.batch', index=True) category = fields.Char('Category', index=True) # normal vs nightly vs weekly, ... dockerfile_id = fields.Many2one('runbot.dockerfile', index=True, default=lambda self: self.env.ref('runbot.docker_default', raise_if_not_found=False)) skip_requirements = fields.Boolean('Skip requirements.txt auto install') From 72281b0c632c1d7c09a066227c2c98cbb3f31eb0 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 22 Jun 2023 14:37:49 +0200 Subject: [PATCH 015/192] [IMP] runbot_merge: allow running test suite without an explicit addons path --- conftest.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/conftest.py b/conftest.py index 8e7b05f9..0af3c1c5 100644 --- a/conftest.py +++ b/conftest.py @@ -277,7 +277,7 @@ class DbDict(dict): with tempfile.TemporaryDirectory() as d: subprocess.run([ 'odoo', '--no-http', - '--addons-path', self._adpath, + *(['--addons-path', self._adpath] if self._adpath else []), '-d', db, '-i', module + ',auth_oauth', '--max-cron-threads', '0', '--stop-after-init', @@ -296,7 +296,7 @@ def dbcache(request): dbs = DbDict(request.config.getoption('--addons-path')) yield dbs for db in dbs.values(): - subprocess.run(['dropdb', db], check=True) + subprocess.run(['dropdb', '--if-exists', db], check=True) @pytest.fixture def db(request, module, dbcache): @@ -369,10 +369,10 @@ def server(request, db, port, module, dummy_addons_path, tmpdir): if not request.config.getoption('--log-github'): log_handlers.append('github_requests:WARNING') - addons_path = ','.join(map(str, [ + addons_path = ','.join(map(str, filter(None, [ request.config.getoption('--addons-path'), dummy_addons_path, - ])) + ]))) cov = [] if request.config.getoption('--coverage'): From b72fdff01d3d923b7ba37c64b8df9e13b3a42f35 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Thu, 22 Jun 2023 15:57:19 +0200 Subject: [PATCH 016/192] [DEL] runbot: 15.0 eol From f22ae357e343cfd09735fd35a1d097589a99d674 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Thu, 22 Jun 2023 15:57:19 +0200 Subject: [PATCH 017/192] [DEL] runbot: 15.0 eol From d748d4b215a72cf1b73d023f224b61ac908a4926 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 27 Jun 2023 12:51:23 +0200 Subject: [PATCH 018/192] [IMP] *: create a single template db per module to test Before this, when testing in parallel (using xdist) each worker would create its own template database (per module, so 2) then would copy the database for each test. This is pretty inefficient as the init of a db is quite expensive in CPU, and when increasing the number of workers (as the test suite is rather IO bound) this would trigger a stampede as every worker would try to create a template at the start of the test suite, leading to extremely high loads and degraded host performances (e.g. 16 workers would cause a load of 20 on a 4 cores 8 thread machine, which makes its use difficult). Instead we can have a lockfile at a known location of the filesystem, the first worker to need a template for a module install locks it, creates the templates, then writes the template's name to the lockfile. Every other worker can then lock the lockfile and read the name out, using the db for duplication. Note: needs to use `os.open` because the modes of `open` apparently can't express "open at offset 0 for reading or create for writing", `r+` refuses to create the file, `w+` still truncates, and `a+` is undocumented and might not allow seeking back to the start on all systems so better avoid it. The implementation would be simplified by using `lockfile` but that's an additional dependency plus it's deprecated. It recommends `fasteners` but that seems to suck (not clear if storing stuff in the lockfile is supported, it opens the lockfile in append mode). Here the lockfiles are sufficient to do the entire thing. Conveniently, this turns out to improve *both* walltime CPU time compared to the original version, likely because while workers now have to wait on whoever is creating the template they're not competing for resources with it. --- conftest.py | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/conftest.py b/conftest.py index 0af3c1c5..329b3524 100644 --- a/conftest.py +++ b/conftest.py @@ -46,6 +46,7 @@ import collections import configparser import contextlib import copy +import fcntl import functools import http.client import itertools @@ -88,11 +89,20 @@ def pytest_addoption(parser): "blow through the former); localtunnel has no rate-limiting but " "the servers are way less reliable") +def is_manager(config): + return not hasattr(config, 'workerinput') # noinspection PyUnusedLocal def pytest_configure(config): sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'mergebot_test_utils')) +def pytest_unconfigure(config): + if not is_manager(config): + return + + for c in config._tmp_path_factory.getbasetemp().iterdir(): + if c.is_file() and c.name.startswith('template-'): + subprocess.run(['dropdb', '--if-exists', c.read_text(encoding='utf-8')]) @pytest.fixture(scope='session', autouse=True) def _set_socket_timeout(): @@ -269,12 +279,26 @@ def tunnel(pytestconfig, port): raise ValueError("Unsupported %s tunnel method" % tunnel) class DbDict(dict): - def __init__(self, adpath): + def __init__(self, adpath, shared_dir): super().__init__() self._adpath = adpath + self._shared_dir = shared_dir def __missing__(self, module): - self[module] = db = 'template_%s' % uuid.uuid4() - with tempfile.TemporaryDirectory() as d: + with contextlib.ExitStack() as atexit: + f = atexit.enter_context(os.fdopen(os.open( + self._shared_dir / f'template-{module}', + os.O_CREAT | os.O_RDWR + ), mode="r+", encoding='utf-8')) + fcntl.lockf(f, fcntl.LOCK_EX) + atexit.callback(fcntl.lockf, f, fcntl.LOCK_UN) + + db = f.read() + if db: + self[module] = db + return db + + d = atexit.enter_context(tempfile.TemporaryDirectory()) + self[module] = db = 'template_%s' % uuid.uuid4() subprocess.run([ 'odoo', '--no-http', *(['--addons-path', self._adpath] if self._adpath else []), @@ -286,17 +310,25 @@ class DbDict(dict): check=True, env={**os.environ, 'XDG_DATA_HOME': d} ) + f.write(db) + f.flush() + os.fsync(f.fileno()) + return db @pytest.fixture(scope='session') -def dbcache(request): +def dbcache(request, tmp_path_factory): """ Creates template DB once per run, then just duplicates it before starting odoo and running the testcase """ - dbs = DbDict(request.config.getoption('--addons-path')) + shared_dir = tmp_path_factory.getbasetemp() + if not is_manager(request.config): + # xdist workers get a subdir as their basetemp, so we need to go one + # level up to deref it + shared_dir = shared_dir.parent + + dbs = DbDict(request.config.getoption('--addons-path'), shared_dir) yield dbs - for db in dbs.values(): - subprocess.run(['dropdb', '--if-exists', db], check=True) @pytest.fixture def db(request, module, dbcache): From aefbdaf974777a1462d234cd2842097ab0fc61c0 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 4 Jul 2023 13:08:40 +0200 Subject: [PATCH 019/192] [IMP] *: client side sorted implementation Allow sorting by a callback. Sort remains client-side. --- conftest.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/conftest.py b/conftest.py index 329b3524..a244f2af 100644 --- a/conftest.py +++ b/conftest.py @@ -1185,9 +1185,13 @@ class Model: # because sorted is not xmlrpc-compatible (it doesn't downgrade properly) def sorted(self, field): - rs = self.read([field]) - rs.sort(key=lambda r: r[field]) - return Model(self._env, self._model, [r['id'] for r in rs]) + fn = field if callable(field) else lambda r: r[field] + + return Model(self._env, self._model, ( + id + for record in sorted(self, key=fn) + for id in record.ids + )) def __getitem__(self, index): if isinstance(index, str): From 81ce4ea02b76af4dcc859739985a41ff8c073b6e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 5 Jul 2023 15:11:40 +0200 Subject: [PATCH 020/192] [IMP] rewrite /forwardport/outstanding - add support for authorship (not just approval) - make display counts directly - fix `state` filter: postgres can't do negative index lookups - add indexes for author and reviewed_by as we look them up - ensure we handle the entire source filtering via a single subquery Closes #778 --- forwardport/controllers.py | 79 +++++++++++ forwardport/data/views.xml | 152 +++++++++++++-------- forwardport/models/project.py | 25 ---- runbot_merge/models/pull_requests.py | 4 +- runbot_merge/static/scss/runbot_merge.scss | 11 ++ 5 files changed, 185 insertions(+), 86 deletions(-) diff --git a/forwardport/controllers.py b/forwardport/controllers.py index af464dcb..620eab96 100644 --- a/forwardport/controllers.py +++ b/forwardport/controllers.py @@ -1,7 +1,14 @@ +import collections +import datetime import pathlib +import werkzeug.urls + +from odoo.http import route, request +from odoo.osv import expression from odoo.addons.runbot_merge.controllers.dashboard import MergebotDashboard +DEFAULT_DELTA = datetime.timedelta(days=7) class Dashboard(MergebotDashboard): def _entries(self): changelog = pathlib.Path(__file__).parent / 'changelog' @@ -13,3 +20,75 @@ class Dashboard(MergebotDashboard): for d in changelog.iterdir() ] + + @route('/forwardport/outstanding', type='http', methods=['GET'], auth="user", website=True, sitemap=False) + def outstanding(self, partner=0, authors=True, reviewers=True, group=0): + Partners = request.env['res.partner'] + PullRequests = request.env['runbot_merge.pull_requests'] + partner = Partners.browse(int(partner)) + group = Partners.browse(int(group)) + authors = int(authors) + reviewers = int(reviewers) + link = lambda **kw: '?' + werkzeug.urls.url_encode({'partner': partner.id or 0, 'authors': authors, 'reviewers': reviewers, **kw, }) + groups = Partners.search([('is_company', '=', True), ('child_ids', '!=', False)]) + if not (authors or reviewers): + return request.render('forwardport.outstanding', { + 'authors': 0, + 'reviewers': 0, + 'single': partner, + 'culprits': partner, + 'groups': groups, + 'current_group': group, + 'outstanding': [], + 'outstanding_per_author': {partner: 0}, + 'outstanding_per_reviewer': {partner: 0}, + 'link': link, + }) + + source_filter = [('merge_date', '<', datetime.datetime.now() - DEFAULT_DELTA)] + partner_filter = [] + if partner or group: + if partner: + suffix = '' + arg = partner.id + else: + suffix = '.commercial_partner_id' + arg = group.id + + if authors: + partner_filter.append([(f'author{suffix}', '=', arg)]) + if reviewers: + partner_filter.append([(f'reviewed_by{suffix}', '=', arg)]) + + source_filter.extend(expression.OR(partner_filter)) + + outstanding = PullRequests.search([ + ('state', 'in', ['opened', 'validated', 'approved', 'ready', 'error']), + ('source_id', 'in', PullRequests._search(source_filter)), + ]) + outstanding_per_author = collections.Counter() + outstanding_per_reviewer = collections.Counter() + outstandings = [] + for source in outstanding.mapped('source_id').sorted('merge_date'): + outstandings.append({ + 'source': source, + 'prs': source.forwardport_ids.filtered(lambda p: p.state not in ['merged', 'closed']), + }) + if authors: + outstanding_per_author[source.author] += 1 + if reviewers and source: + outstanding_per_reviewer[source.reviewed_by] += 1 + + culprits = Partners.browse(p.id for p, _ in (outstanding_per_reviewer + outstanding_per_author).most_common()) + return request.render('forwardport.outstanding', { + 'authors': authors, + 'reviewers': reviewers, + 'single': partner, + 'culprits': culprits, + 'groups': groups, + 'current_group': group, + 'outstanding_per_author': outstanding_per_author, + 'outstanding_per_reviewer': outstanding_per_reviewer, + 'outstanding': outstandings, + 'link': link, + }) diff --git a/forwardport/data/views.xml b/forwardport/data/views.xml index c25bb311..1b6069c2 100644 --- a/forwardport/data/views.xml +++ b/forwardport/data/views.xml @@ -30,67 +30,101 @@ bg-warning - - Outstanding forward ports - qweb - /forwardport/outstanding - - True - forwardport.outstanding_fp - - - - -
- -

List of pull requests with outstanding forward ports

- -
-

- merged by -

-
- - -
- - by - merged - - by - -
-
- Outstanding forward-ports: -
    -
  • - - () - targeting -
  • -
-
+