From db9e86f7608ed82e03cd7b3543f3bb072f147493 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 20 May 2020 12:42:45 +0200 Subject: [PATCH] [IMP] forwardport: reliability of PR reminders The exponential backoff offsets from the write_date of the children PRs, however it doesn't reset, so the offsetting gets bumped up way more than originally expected or designed if the child PRs are under active development for some reason. Fix this by adding a field to specifically record the date of merge of a PR, and check that feature against the backoff offset. This should provide more regular and reliable backoff. Fixes #369 --- forwardport/__manifest__.py | 1 + .../migrations/13.0.1.1/post-reminder-date.py | 10 ++++++++++ forwardport/migrations/13.0.1.1/pre-tagging.py | 2 ++ forwardport/models/forwardport.py | 2 +- forwardport/models/project.py | 13 +++++++++---- forwardport/tests/test_simple.py | 14 ++++++++++++-- 6 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 forwardport/migrations/13.0.1.1/post-reminder-date.py create mode 100644 forwardport/migrations/13.0.1.1/pre-tagging.py diff --git a/forwardport/__manifest__.py b/forwardport/__manifest__.py index c91fe427..f308a55e 100644 --- a/forwardport/__manifest__.py +++ b/forwardport/__manifest__.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- { 'name': 'forward port bot', + 'version': '1.1', 'summary': "A port which forward ports successful PRs.", 'depends': ['runbot_merge'], 'data': [ diff --git a/forwardport/migrations/13.0.1.1/post-reminder-date.py b/forwardport/migrations/13.0.1.1/post-reminder-date.py new file mode 100644 index 00000000..36d131a8 --- /dev/null +++ b/forwardport/migrations/13.0.1.1/post-reminder-date.py @@ -0,0 +1,10 @@ +def migrate(cr, version): + """ Set the merge_date field to the current write_date, and reset + the backoff to its default so we reprocess old PRs properly. + """ + cr.execute(""" + UPDATE runbot_merge_pull_requests + SET merge_date = write_date, + reminder_backoff_factor = -4 + WHERE state = 'merged' + """) diff --git a/forwardport/migrations/13.0.1.1/pre-tagging.py b/forwardport/migrations/13.0.1.1/pre-tagging.py new file mode 100644 index 00000000..ef2ad801 --- /dev/null +++ b/forwardport/migrations/13.0.1.1/pre-tagging.py @@ -0,0 +1,2 @@ +def migrate(cr, version): + cr.execute("delete from ir_model where model = 'forwardport.tagging'") diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 3a0daab8..e58091b8 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -141,7 +141,7 @@ class DeleteBranches(models.Model, Queue): def _search_domain(self): cutoff = self.env.context.get('forwardport_merged_before') \ or fields.Datetime.to_string(datetime.now() - MERGE_AGE) - return [('pr_id.write_date', '<', cutoff)] + return [('pr_id.merge_date', '<', cutoff)] def _process_item(self): _deleter.info( diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 417787c8..a1454d3c 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -25,7 +25,7 @@ import re import subprocess import tempfile -import dateutil +import dateutil.relativedelta import requests from odoo import _, models, fields, api @@ -188,6 +188,7 @@ class PullRequests(models.Model): ) source_id = fields.Many2one('runbot_merge.pull_requests', index=True, help="the original source of this FP even if parents were detached along the way") reminder_backoff_factor = fields.Integer(default=-4) + merge_date = fields.Datetime() fw_policy = fields.Selection([ ('ci', "Normal"), @@ -218,6 +219,8 @@ class PullRequests(models.Model): )[-1].id if vals.get('parent_id') and 'source_id' not in vals: vals['source_id'] = self.browse(vals['parent_id'])._get_root().id + if vals.get('state') == 'merged': + vals['merge_date'] = fields.Datetime.now() return super().create(vals) def write(self, vals): @@ -240,6 +243,8 @@ class PullRequests(models.Model): if vals.get('parent_id') and 'source_id' not in vals: vals['source_id'] = self.browse(vals['parent_id'])._get_root().id + if vals.get('state') == 'merged': + vals['merge_date'] = fields.Datetime.now() r = super().write(vals) if self.env.context.get('forwardport_detach_warn', True): for p in with_parents: @@ -985,12 +990,12 @@ stderr: ('source_id', '!=', False), # active ('state', 'not in', ['merged', 'closed']), - # last updated more than ago - ('write_date', '<', cutoff), + # original merged more than ago + ('source_id.merge_date', '<', cutoff), ], order='source_id, id'), lambda p: p.source_id): backoff = dateutil.relativedelta.relativedelta(days=2**source.reminder_backoff_factor) prs = list(prs) - if all(p.write_date > (cutoff_dt - backoff) for p in prs): + if source.merge_date > (cutoff_dt - backoff): continue source.reminder_backoff_factor += 1 self.env['runbot_merge.pull_requests.feedback'].create({ diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index a211d639..56d1e0b2 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- import collections -import sys import time from datetime import datetime, timedelta from operator import itemgetter @@ -9,7 +8,8 @@ import pytest from utils import * -FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime('%Y-%m-%d %H:%M:%S') +FMT = '%Y-%m-%d %H:%M:%S' +FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT) # need: # * an odoo server @@ -56,6 +56,12 @@ def test_straightforward_flow(env, config, make_repo, users): # use rebase-ff (instead of rebase-merge) so we don't have to dig in # parents of the merge commit to find the cherrypicks pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', prod.name), + ('number', '=', pr.number), + ]) + assert not pr_id.merge_date,\ + "PR obviously shouldn't have a merge date before being merged" env.run_crons() with prod: @@ -65,6 +71,10 @@ def test_straightforward_flow(env, config, make_repo, users): # should merge the staging then create the FP PR env.run_crons() + assert datetime.now() - datetime.strptime(pr_id.merge_date, FMT) <= timedelta(minutes=1),\ + "check if merge date was set about now (within a minute as crons and " \ + "RPC calls yield various delays before we're back)" + p_1_merged = prod.commit('a') assert p_1_merged.id != p_1