From ef52785d82229f685a9c17a9a2dc7de0c803abcc Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 19 Jan 2023 14:23:41 +0100 Subject: [PATCH] [IMP] forwardport: store and display detachment reason Currently, if a PR forward-port PR gets detached the reason for it is not always obvious, and may have to be hunted in the logs or in "sibling" PRs. By writing a forward port reason (hopefully) ever time we detach a PR, and displaying that reason in the form and dashboard, the justification should be a lot more obvious. Fixes #679 --- forwardport/__manifest__.py | 2 +- forwardport/data/views.xml | 7 ++---- .../migrations/15.0.1.2/pre-migration.py | 11 +++++++++ forwardport/models/project.py | 23 +++++++++++++++++-- forwardport/tests/test_simple.py | 5 +++- 5 files changed, 39 insertions(+), 9 deletions(-) create mode 100644 forwardport/migrations/15.0.1.2/pre-migration.py diff --git a/forwardport/__manifest__.py b/forwardport/__manifest__.py index b86c425e..3ece49af 100644 --- a/forwardport/__manifest__.py +++ b/forwardport/__manifest__.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- { 'name': 'forward port bot', - 'version': '1.1', + 'version': '1.2', 'summary': "A port which forward ports successful PRs.", 'depends': ['runbot_merge'], 'data': [ diff --git a/forwardport/data/views.xml b/forwardport/data/views.xml index e0da455e..46a8b48a 100644 --- a/forwardport/data/views.xml +++ b/forwardport/data/views.xml @@ -112,7 +112,7 @@ - DETACHED + DETACHED () @@ -196,10 +196,7 @@ - - Detached from forward porting (either conflicting - or explicitly updated). - + diff --git a/forwardport/migrations/15.0.1.2/pre-migration.py b/forwardport/migrations/15.0.1.2/pre-migration.py new file mode 100644 index 00000000..a632b336 --- /dev/null +++ b/forwardport/migrations/15.0.1.2/pre-migration.py @@ -0,0 +1,11 @@ +def migrate(cr, version): + """ Add a dummy detach reason to detached PRs. + """ + cr.execute( + "ALTER TABLE runbot_merge_pull_requests" + " ADD COLUMN detach_reason varchar" + ) + cr.execute( + "UPDATE runbot_merge_pull_requests" + " SET detach_reason = 'unknown'" + " WHERE source_id IS NOT NULL AND parent_id IS NULL") diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 69d6378e..f59bdbbd 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -235,12 +235,20 @@ class PullRequests(models.Model): reminder_backoff_factor = fields.Integer(default=-4, group_operator=None) merge_date = fields.Datetime() + detach_reason = fields.Char() + fw_policy = fields.Selection([ ('ci', "Normal"), ('skipci', "Skip CI"), # ('skipmerge', "Skip merge"), ], required=True, default="ci") + _sql_constraints = [( + 'fw_constraint', + 'check(source_id is null or num_nonnulls(parent_id, detach_reason) = 1)', + "fw PRs must either be attached or have a reason for being detached", + )] + refname = fields.Char(compute='_compute_refname') @api.depends('label') def _compute_refname(self): @@ -280,6 +288,8 @@ class PullRequests(models.Model): closed_fp = self.filtered(lambda p: p.state == 'closed' and p.source_id) if newhead and not self.env.context.get('ignore_head_update') and newhead != self.head: vals.setdefault('parent_id', False) + if with_parents and vals['parent_id'] is False: + vals['detach_reason'] = f"Head updated from {self.head} to {newhead}" # if any children, this is an FP PR being updated, enqueue # updating children if self.search_count([('parent_id', '=', self.id)]): @@ -333,8 +343,14 @@ class PullRequests(models.Model): def _try_closing(self, by): r = super()._try_closing(by) if r: - self.with_context(forwardport_detach_warn=False).parent_id = False - self.search([('parent_id', '=', self.id)]).parent_id = False + self.with_context(forwardport_detach_warn=False).write({ + 'parent_id': False, + 'detach_reason': f"Closed by {by}", + }) + self.search([('parent_id', '=', self.id)]).write({ + 'parent_id': False, + 'detach_reason': f"{by} closed parent PR {self.display_name}", + }) return r def _parse_commands(self, author, comment, login): @@ -728,6 +744,9 @@ class PullRequests(models.Model): 'source_id': source.id, # only link to previous PR of sequence if cherrypick passed 'parent_id': pr.id if not has_conflicts else False, + 'detach_reason': "conflicts: {}".format( + f'\n{conflicts[pr]}\n{conflicts[pr]}'.strip() + ) if has_conflicts else None, # Copy author & delegates of source as well as delegates of # previous so they can r+ the new forward ports. 'delegates': [(6, False, (source.delegates | pr.delegates).ids)] diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index a4239d4e..ef31c495 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -547,7 +547,10 @@ def test_delegate_fw(env, config, make_repo, users): # ensure pr1 has to be approved to be forward-ported _, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') # detatch from source - pr1_id.parent_id = False + pr1_id.write({ + 'parent_id': False, + 'detach_reason': "Detached for testing.", + }) with prod: prod.post_status(pr1_id.head, 'success', 'legal/cla') prod.post_status(pr1_id.head, 'success', 'ci/runbot')