From e175609950e633c02c0ffe13e666f2a437bc519e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 13 Jan 2021 08:18:17 +0100 Subject: [PATCH] [IMP] forwardport: unmodified fw automatically inherit overrides Before this change, a CI override would have to be replicated on most / all forward-ports of the base PR. This was intentional to see how it would shake out, the answer being that it's rather annoying. Also add a `statuses_full` computed field on PRs for the aggregate status: the existing `statuses` field is just a copy of the commit statuses which I didn't remember I kept free of the overrides so the commit statuses could be displayed "as-is" in the backend (the overrides are displayed separately). And while at it fix the PR dashboard to use that new field: that was basically the intention but then I went on to use the "wrong" field hence #433. Mebbe the UI part should be displayed using a computed M2M (?) as a table or as tags instead? This m2m could indicate whether the status is an override or an "intrinsic" status. Also removed some dead code: * leftover from the removed tagging feature (removed the tag manipulation but forgot some of the setup / computations) * unused local variables * an empty skipped test case Fixes #439. Fixes #433. --- forwardport/models/project.py | 10 +++ forwardport/tests/test_overrides.py | 116 ++++++++++++++++++++++++++ forwardport/tests/test_simple.py | 2 +- forwardport/tests/test_weird.py | 1 - runbot_merge/controllers/dashboard.py | 3 +- runbot_merge/models/pull_requests.py | 29 ++++--- runbot_merge/tests/test_basic.py | 9 +- 7 files changed, 147 insertions(+), 23 deletions(-) create mode 100644 forwardport/tests/test_overrides.py diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 315e33e3..5c917917 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -513,6 +513,16 @@ class PullRequests(models.Model): else: break + @api.depends('parent_id.statuses') + def _compute_statuses(self): + super()._compute_statuses() + + def _get_overrides(self): + # NB: assumes _get_overrides always returns an "owned" dict which we can modify + p = self.parent_id._get_overrides() if self.parent_id else {} + p.update(super()._get_overrides()) + return p + def _iter_ancestors(self): while self: yield self diff --git a/forwardport/tests/test_overrides.py b/forwardport/tests/test_overrides.py new file mode 100644 index 00000000..e8a18d35 --- /dev/null +++ b/forwardport/tests/test_overrides.py @@ -0,0 +1,116 @@ +import json + +from utils import Commit, make_basic + +def statuses(pr): + return { + k: v['state'] + for k, v in json.loads(pr.statuses_full).items() + } +def test_override_inherited(env, config, make_repo, users): + """ A forwardport should inherit its parents' overrides, until it's edited. + """ + repo, other = make_basic(env, config, make_repo) + project = env['runbot_merge.project'].search([]) + env['res.partner'].search([('github_login', '=', users['reviewer'])])\ + .write({'override_rights': [(0, 0, { + 'repository_id': project.repo_ids.id, + 'context': 'ci/runbot', + })]}) + + with repo: + repo.make_commits('a', Commit('C', tree={'a': '0'}), ref='heads/change') + pr = repo.make_pr(target='a', head='change') + repo.post_status('change', 'success', 'legal/cla') + pr.post_comment('hansen r+ override=ci/runbot', config['role_reviewer']['token']) + env.run_crons() + + original = env['runbot_merge.pull_requests'].search([('repository.name', '=', repo.name), ('number', '=', pr.number)]) + assert original.state == 'ready' + + with repo: + repo.post_status('staging.a', 'success', 'legal/cla') + repo.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') + assert pr0_id == original + assert pr1_id.parent_id, pr0_id + + with repo: + repo.post_status(pr1_id.head, 'success', 'legal/cla') + env.run_crons() + assert pr1_id.state == 'validated' + assert statuses(pr1_id) == {'ci/runbot': 'success', 'legal/cla': 'success'} + + # now we edit the child PR + pr_repo, pr_ref = repo.get_pr(pr1_id.number).branch + with pr_repo: + pr_repo.make_commits( + pr1_id.target.name, + Commit('wop wop', tree={'a': '1'}), + ref=f'heads/{pr_ref}', + make=False + ) + env.run_crons() + assert pr1_id.state == 'opened' + assert not pr1_id.parent_id + assert statuses(pr1_id) == {}, "should not have any status left" + +def test_override_combination(env, config, make_repo, users): + """ A forwardport should inherit its parents' overrides, until it's edited. + """ + repo, other = make_basic(env, config, make_repo) + project = env['runbot_merge.project'].search([]) + env['res.partner'].search([('github_login', '=', users['reviewer'])]) \ + .write({'override_rights': [ + (0, 0, { + 'repository_id': project.repo_ids.id, + 'context': 'ci/runbot', + }), + (0, 0, { + 'repository_id': project.repo_ids.id, + 'context': 'legal/cla', + }) + ]}) + + with repo: + repo.make_commits('a', Commit('C', tree={'a': '0'}), ref='heads/change') + pr = repo.make_pr(target='a', head='change') + repo.post_status('change', 'success', 'legal/cla') + pr.post_comment('hansen r+ override=ci/runbot', config['role_reviewer']['token']) + env.run_crons() + + pr0_id = env['runbot_merge.pull_requests'].search([('repository.name', '=', repo.name), ('number', '=', pr.number)]) + assert pr0_id.state == 'ready' + assert statuses(pr0_id) == {'ci/runbot': 'success', 'legal/cla': 'success'} + + with repo: + repo.post_status('staging.a', 'success', 'legal/cla') + repo.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + # check for combination: ci/runbot is overridden through parent, if we + # override legal/cla then the PR should be validated + pr1_id = env['runbot_merge.pull_requests'].search([('parent_id', '=', pr0_id.id)]) + assert pr1_id.state == 'opened' + assert statuses(pr1_id) == {'ci/runbot': 'success'} + with repo: + repo.get_pr(pr1_id.number).post_comment('hansen override=legal/cla', config['role_reviewer']['token']) + env.run_crons() + assert pr1_id.state == 'validated' + + # editing the child should devalidate + pr_repo, pr_ref = repo.get_pr(pr1_id.number).branch + with pr_repo: + pr_repo.make_commits( + pr1_id.target.name, + Commit('wop wop', tree={'a': '1'}), + ref=f'heads/{pr_ref}', + make=False + ) + env.run_crons() + assert pr1_id.state == 'opened' + assert not pr1_id.parent_id + assert statuses(pr1_id) == {'legal/cla': 'success'}, \ + "should only have its own status left" diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 2919938b..0b015f74 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -1220,7 +1220,7 @@ class TestBranchDeletion: prod.post_status(c, 'success', 'ci/runbot') pr1.post_comment('hansen r+', config['role_reviewer']['token']) - [c] = other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch') + other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch') pr2 = prod.make_pr(target='a', head='%s:bbranch' % other.owner, title='b') pr2.close() diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 9866e1cc..9cfa3ff5 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -173,7 +173,6 @@ def test_failed_staging(env, config, make_repo): env.run_crons() pr1_id, pr2_id, pr3_id = env['runbot_merge.pull_requests'].search([], order='number') - pr2 = prod.get_pr(pr2_id.number) pr3 = prod.get_pr(pr3_id.number) with prod: prod.post_status(pr3_id.head, 'success', 'legal/cla') diff --git a/runbot_merge/controllers/dashboard.py b/runbot_merge/controllers/dashboard.py index 6ba51b4e..33919a82 100644 --- a/runbot_merge/controllers/dashboard.py +++ b/runbot_merge/controllers/dashboard.py @@ -1,5 +1,4 @@ # -*- coding: utf-8 -*- -import ast import json import werkzeug.exceptions @@ -43,7 +42,7 @@ class MergebotDashboard(Controller): # normalise `statuses` to map to a dict st = { k: {'state': v} if isinstance(v, str) else v - for k, v in ast.literal_eval(pr_id.statuses).items() + for k, v in json.loads(pr_id.statuses_full).items() } return request.render('runbot_merge.view_pull_request', { 'pr': pr_id, diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 9449b028..1e73bfb7 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -673,7 +673,14 @@ class PullRequests(models.Model): priority = fields.Integer(default=2, index=True) overrides = fields.Char(required=True, default='{}') - statuses = fields.Text(compute='_compute_statuses') + statuses = fields.Text( + compute='_compute_statuses', + help="Copy of the statuses from the HEAD commit, as a Python literal" + ) + statuses_full = fields.Text( + compute='_compute_statuses', + help="Compilation of the full status of the PR (commit statuses + overrides), as JSON" + ) status = fields.Char(compute='_compute_statuses') previous_failure = fields.Char(default='{}') @@ -773,13 +780,19 @@ class PullRequests(models.Model): pr.blocked = 'linked pr %s is not ready' % unready.display_name continue - @api.depends('head', 'repository.status_ids') + def _get_overrides(self): + if self: + return json.loads(self.overrides) + return {} + + @api.depends('head', 'repository.status_ids', 'overrides') def _compute_statuses(self): Commits = self.env['runbot_merge.commit'] for pr in self: c = Commits.search([('sha', '=', pr.head)]) st = json.loads(c.statuses or '{}') - statuses = {**st, **json.loads(pr.overrides)} + statuses = {**st, **pr._get_overrides()} + pr.statuses_full = json.dumps(statuses) if not statuses: pr.status = pr.statuses = False continue @@ -1079,19 +1092,14 @@ class PullRequests(models.Model): failed = self.browse(()) for pr in self: required = pr.repository.status_ids._for_pr(pr).mapped('context') - sts = {**statuses, **json.loads(pr.overrides)} + sts = {**statuses, **pr._get_overrides()} - a, r = [], [] success = True for ci in required: st = state_(sts, ci) or 'pending' if st == 'success': - r.append(f'\N{Ballot Box} {ci}') - a.append(f'\N{Ballot Box with Check} {ci}') continue - a.append(f'\N{Ballot Box} {ci}') - r.append(f'\N{Ballot Box with Check} {ci}') success = False if st in ('error', 'failure'): failed |= pr @@ -1192,8 +1200,6 @@ class PullRequests(models.Model): }) def write(self, vals): - oldstate = { pr: pr._tagstate for pr in self } - if vals.get('squash'): vals['merge_method'] = False @@ -1472,7 +1478,6 @@ class Tagging(models.Model): tags_add = fields.Char(required=True, default='[]') def create(self, values): - before = str(values) if values.pop('state_from', None): values['tags_remove'] = ALL_TAGS if 'state_to' in values: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index f53be763..bba5b20b 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -659,7 +659,7 @@ def test_ff_failure_batch(env, repo, users, config): # block FF with repo: - m2 = repo.make_commit('heads/master', 'NO!', None, tree={'m': 'm2'}) + repo.make_commit('heads/master', 'NO!', None, tree={'m': 'm2'}) old_staging = repo.commit('heads/staging.master') # confirm staging @@ -801,11 +801,6 @@ class TestPREdition: with repo: prx.base = 'master' -@pytest.mark.skip(reason="What do?") -def test_edit_staged(env, repo): - """ - What should happen when editing the PR/metadata (not pushing) of a staged PR - """ def test_close_staged(env, repo, config, page): """ When closing a staged PR, cancel the staging @@ -915,7 +910,7 @@ def test_rebase_failure(env, repo, users, config): return original(*args) env['runbot_merge.commit']._notify() - with mock.patch.object(GH, 'set_ref', autospec=True, side_effect=wrapper) as m: + with mock.patch.object(GH, 'set_ref', autospec=True, side_effect=wrapper): env['runbot_merge.project']._check_progress() env['runbot_merge.project']._send_feedback()