From 4da0f5df69c86ac7dfca3f9b283091b52d5172ed Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 12 Nov 2021 16:04:34 +0100 Subject: [PATCH] [ADD] runbot_merge: ~~tree~~ freeze wizard Provides a less manual interface for creating the freeze: * takes the name of the branch to create * takes any number of PRs which must be part of the freeze * takes PRs representing the HEADs of the new branches Then essentially takes care of the test. Implementation of the actual wizard is not trivial but fairly straightforward and linear, biggest issue is not being able to `project_id.branch_ids[1]` to get the new branch, not sure why but it seems to ignore the ordering, clearing the cache doens't fix it. When creating the branches, add a sleep after each one for secondary rate limiting purposes. Same when deleting branches. Also the forwardbot has been updated to disable the forwardport cron while a freeze is ongoing, this simplifies the freezing process. Note: after recommendation of @aab-odoo, tried using `_applyChanges` in `_checkState` but it simply did not work: the two relational fields got completely frozen and were impossible to update, which is less than ideal. Oh well, hopefully it works well enough like this for now. --- forwardport/models/__init__.py | 1 + forwardport/models/project_freeze.py | 24 +++ runbot_merge/__manifest__.py | 1 + runbot_merge/models/__init__.py | 1 + runbot_merge/models/project.py | 37 ++++ .../models/project_freeze/__init__.py | 198 ++++++++++++++++++ runbot_merge/models/project_freeze/views.xml | 52 +++++ runbot_merge/models/pull_requests.py | 10 +- runbot_merge/security/ir.model.access.csv | 2 + runbot_merge/static/project_freeze/index.js | 62 ++++++ runbot_merge/tests/test_multirepo.py | 151 +++++++++++++ runbot_merge/views/runbot_merge_project.xml | 29 ++- 12 files changed, 560 insertions(+), 8 deletions(-) create mode 100644 forwardport/models/project_freeze.py create mode 100644 runbot_merge/models/project_freeze/__init__.py create mode 100644 runbot_merge/models/project_freeze/views.xml create mode 100644 runbot_merge/static/project_freeze/index.js diff --git a/forwardport/models/__init__.py b/forwardport/models/__init__.py index f3fb02c7..96d53ab5 100644 --- a/forwardport/models/__init__.py +++ b/forwardport/models/__init__.py @@ -1,3 +1,4 @@ # -*- coding: utf-8 -*- from . import project +from . import project_freeze from . import forwardport diff --git a/forwardport/models/project_freeze.py b/forwardport/models/project_freeze.py new file mode 100644 index 00000000..8168e14a --- /dev/null +++ b/forwardport/models/project_freeze.py @@ -0,0 +1,24 @@ +from odoo import models + + +class FreezeWizard(models.Model): + """ Override freeze wizard to disable the forward port cron when one is + created (so there's a freeze ongoing) and re-enable it once all freezes are + done. + + If there ever is a case where we have lots of projects, + """ + _inherit = 'runbot_merge.project.freeze' + + def create(self, vals_list): + r = super().create(vals_list) + self.env.ref('forwardport.port_forward').active = False + return r + + def unlink(self): + r = super().unlink() + if not self.search_count([]): + self.env.ref('forwardport.port_forward').active = True + return r + + diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index 6092c4bd..e44f1830 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -9,6 +9,7 @@ 'data/merge_cron.xml', 'views/res_partner.xml', 'views/runbot_merge_project.xml', + 'models/project_freeze/views.xml', 'views/mergebot.xml', 'views/templates.xml', ], diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py index 8e548f8f..b457f468 100644 --- a/runbot_merge/models/__init__.py +++ b/runbot_merge/models/__init__.py @@ -1,3 +1,4 @@ from . import res_partner from . import project from . import pull_requests +from . import project_freeze diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index cf25498f..dc83b8f4 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -42,6 +42,9 @@ class Project(models.Model): "will lead to webhook rejection. Should only use ASCII." ) + freeze_id = fields.Many2one('runbot_merge.project.freeze', compute='_compute_freeze') + freeze_reminder = fields.Text() + def _check_stagings(self, commit=False): for branch in self.search([]).mapped('branch_ids').filtered('active'): staging = branch.active_staging_id @@ -81,3 +84,37 @@ class Project(models.Model): LIMIT 1 """, (self.id, name)) return bool(self.env.cr.rowcount) + + def _next_freeze(self): + prev = self.branch_ids[1:2].name + if not prev: + return None + + m = re.search(r'(\d+)(?:\.(\d+))?$', prev) + if m: + return "%s.%d" % (m[1], (int(m[2] or 0) + 1)) + else: + return f'post-{prev}' + + def _compute_freeze(self): + freezes = { + f.project_id.id: f.id + for f in self.env['runbot_merge.project.freeze'].search([('project_id', 'in', self.ids)]) + } + print(freezes) + for project in self: + project.freeze_id = freezes.get(project.id) or False + + def action_prepare_freeze(self): + """ Initialises the freeze wizard and returns the corresponding action. + """ + self.check_access_rights('write') + self.check_access_rule('write') + Freeze = self.env['runbot_merge.project.freeze'].sudo() + + w = Freeze.search([('project_id', '=', self.id)]) or Freeze.create({ + 'project_id': self.id, + 'branch_name': self._next_freeze(), + 'release_pr_ids': [(0, 0, {'repository_id': repo.id}) for repo in self.repo_ids] + }) + return w.action_freeze() diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py new file mode 100644 index 00000000..f795261d --- /dev/null +++ b/runbot_merge/models/project_freeze/__init__.py @@ -0,0 +1,198 @@ +import enum +import itertools +import logging +import time + +from odoo import models, fields, api +from odoo.exceptions import UserError + +_logger = logging.getLogger(__name__) +class FreezeWizard(models.Model): + _name = 'runbot_merge.project.freeze' + _description = "Wizard for freezing a project('s master)" + + project_id = fields.Many2one('runbot_merge.project', required=True) + branch_name = fields.Char(required=True, help="Name of the new branches to create") + release_pr_ids = fields.One2many( + 'runbot_merge.project.freeze.prs', 'wizard_id', + string="Release pull requests", + help="Pull requests used as tips for the freeze branches, one per repository" + ) + required_pr_ids = fields.Many2many( + 'runbot_merge.pull_requests', string="Required Pull Requests", + domain="[('state', 'not in', ('closed', 'merged'))]", + help="Pull requests which must have been merged before the freeze is allowed", + ) + errors = fields.Text(compute='_compute_errors') + + _sql_constraints = [ + ('unique_per_project', 'unique (project_id)', + "There should be only one ongoing freeze per project"), + ] + + @api.depends('release_pr_ids.pr_id.label', 'required_pr_ids.state') + def _compute_errors(self): + errors = [] + without = self.release_pr_ids.filtered(lambda p: not p.pr_id) + if without: + errors.append("* Every repository must have a release PR, missing release PRs for %s." % ', '.join( + p.repository_id.name for p in without + )) + + labels = set(self.mapped('release_pr_ids.pr_id.label')) + if len(labels) != 1: + errors.append("* All release PRs must have the same label, found %r." % ', '.join(labels)) + + unready = sum(p.state not in ('closed', 'merged') for p in self.required_pr_ids) + if unready: + errors.append(f"* {unready} required PRs not ready.") + + self.errors = '\n'.join(errors) or False + + def action_cancel(self): + self.project_id.check_access_rights('write') + self.project_id.check_access_rule('write') + self.sudo().unlink() + + return {'type': 'ir.actions.act_window_close'} + + def action_freeze(self): + """ Attempts to perform the freeze. + """ + project_id = self.project_id + # if there are still errors, reopen the wizard + if self.errors: + return { + 'type': 'ir.actions.act_window', + 'target': 'new', + 'name': f'Freeze project {project_id.name}', + 'view_mode': 'form', + 'res_model': self._name, + 'res_id': self.id, + } + + # need to create the new branch, but at the same time resequence + # 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:] + seq = itertools.count(start=1) # start reseq at 1 + commands = [ + (1, master.id, {'sequence': next(seq)}), + (0, 0, { + 'name': self.branch_name, + 'sequence': next(seq), + }) + ] + for s, b in zip(seq, rest): + commands.append((1, b.id, {'sequence': s})) + project_id.branch_ids = commands + + # update release PRs to get merged on the newly created branch + new_branch = project_id.branch_ids - master - rest + self.release_pr_ids.mapped('pr_id').write({'target': new_branch.id, 'priority': 0}) + + # create new branch on every repository + errors = [] + repository = None + for repository in project_id.repo_ids: + gh = repository.github() + # annoyance: can't directly alias a ref to an other ref, need to + # resolve the "old" branch explicitely + prev = gh('GET', f'git/refs/heads/{master.name}') + if not prev.ok: + errors.append(f"Unable to resolve branch {master.name} of repository {repository.name} to a commit.") + break + new_branch = gh('POST', 'git/refs', json={ + 'ref': 'refs/heads/' + self.branch_name, + 'sha': prev.json()['object']['sha'], + }, check=False) + if not new_branch.ok: + err = new_branch.json()['message'] + errors.append(f"Unable to create branch {master.name} of repository {repository.name}: {err}.") + break + time.sleep(1) + + # if an error occurred during creation, try to clean up then raise error + if errors: + for r in project_id.repo_ids: + if r == repository: + break + + deletion = r.github().delete(f'git/refs/heads/{self.branch_name}') + if not deletion.ok: + errors.append(f"Consequently unable to delete branch {self.branch_name} of repository {r.name}.") + time.sleep(1) + raise UserError('\n'.join(errors)) + + # delete wizard + self.sudo().unlink() + # managed to create all the things, show reminder text (or close) + if project_id.freeze_reminder: + return { + 'type': 'ir.actions.act_window', + 'target': 'new', + 'name': f'Freeze reminder {project_id.name}', + 'view_mode': 'form', + 'res_model': project_id._name, + 'res_id': project_id.id, + 'view_id': self.env.ref('runbot_merge.project_freeze_reminder').id + } + + return {'type': 'ir.actions.act_window_close'} + +class ReleasePullRequest(models.Model): + _name = 'runbot_merge.project.freeze.prs' + _description = "links to pull requests used to \"cap\" freezes" + + wizard_id = fields.Many2one('runbot_merge.project.freeze', required=True, index=True, ondelete='cascade') + repository_id = fields.Many2one('runbot_merge.repository', required=True) + pr_id = fields.Many2one( + 'runbot_merge.pull_requests', + domain='[("repository", "=", repository_id), ("state", "not in", ("closed", "merged"))]', + string="Release Pull Request", + ) + + def write(self, vals): + # only the pr should be writeable after initial creation + assert 'wizard_id' not in vals + assert 'repository_id' not in vals + # and if the PR gets set, it should match the requested repository + if 'pr_id' in vals: + assert self.env['runbot_merge.pull_requests'].browse(vals['pr_id'])\ + .repository == self.repository_id + + return super().write(vals) + +@enum.unique +class Colors(enum.IntEnum): + No = 0 + Red = 1 + Orange = 2 + Yellow = 3 + LightBlue = 4 + DarkPurple = 5 + Salmon = 6 + MediumBlue = 7 + DarkBlue = 8 + Fuchsia = 9 + Green = 10 + Purple = 11 + +STATE_COLORMAP = { + 'opened': Colors.No, + 'closed': Colors.Orange, + 'validated': Colors.No, + 'approved': Colors.No, + 'ready': Colors.LightBlue, + 'merged': Colors.Green, + 'error': Colors.Red, +} +class PullRequestColor(models.Model): + _inherit = 'runbot_merge.pull_requests' + + state_color = fields.Integer(compute='_compute_state_color') + + @api.depends('state') + def _compute_state_color(self): + for p in self: + p.state_color = STATE_COLORMAP[p.state] diff --git a/runbot_merge/models/project_freeze/views.xml b/runbot_merge/models/project_freeze/views.xml new file mode 100644 index 00000000..255be83d --- /dev/null +++ b/runbot_merge/models/project_freeze/views.xml @@ -0,0 +1,52 @@ + + + + + Freeze Wizard Configuration Screen + runbot_merge.project.freeze + +
+ + + + + + + + + + + + + + + + + + +
+ +
+ +
+ + + diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index c83bcff6..c0eae29d 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1851,14 +1851,10 @@ class Stagings(models.Model): self, prs ) prs.write({'state': 'merged'}) + pseudobranch = None - if self.target == project.branch_ids[:1] and project.branch_ids[1:2]: - prev = project.branch_ids[1:2].name - m = re.search(r'(\d+)(?:\.(\d+))?$', prev) - if m: - pseudobranch = "%s.%d" % (m[1], (int(m[2] or 0) + 1)) - else: - pseudobranch = 'post-' + prev + if self.target == project.branch_ids[:1]: + pseudobranch = project._next_freeze() for pr in prs: self.env['runbot_merge.pull_requests.feedback'].create({ diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 42dc408c..5815aed8 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -1,5 +1,7 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink access_runbot_merge_project_admin,Admin access to project,model_runbot_merge_project,runbot_merge.group_admin,1,1,1,1 +access_runbot_merge_project_freeze,Admin access to freeze wizard,model_runbot_merge_project_freeze,runbot_merge.group_admin,1,1,0,0 +access_runbot_merge_project_freeze_prs,Admin access to freeze wizard prs,model_runbot_merge_project_freeze_prs,runbot_merge.group_admin,1,1,0,0 access_runbot_merge_repository_admin,Admin access to repo,model_runbot_merge_repository,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_repository_status_admin,Admin access to repo statuses,model_runbot_merge_repository_status,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_branch_admin,Admin access to branches,model_runbot_merge_branch,runbot_merge.group_admin,1,1,1,1 diff --git a/runbot_merge/static/project_freeze/index.js b/runbot_merge/static/project_freeze/index.js new file mode 100644 index 00000000..a0568962 --- /dev/null +++ b/runbot_merge/static/project_freeze/index.js @@ -0,0 +1,62 @@ +odoo.define('runbot_merge.index', function (require) { +"use strict"; +const FormController = require('web.FormController'); +const FormView = require('web.FormView'); +const viewRegistry = require('web.view_registry'); + +/** + * Attept at a "smart" controller for the freeze wizard: keeps triggering + * onchange() on the form in order to try and update the error information, as + * some of the "errors" are not under direct operator control. Hopefully this + * allows the operator to just keep the wizard open and wait until the error + * messages disappear so they can proceed. + */ +const FreezeController = FormController.extend({ + async _checkState() { + const record = this.model.get(this.handle) + const requiredPrIds = record.data.required_pr_ids; + + // we're inside the form's mutex, so can use `_applyChange` directly + const changed = await this.model._applyChange(this.handle, { + required_pr_ids: { + operation: 'REPLACE_WITH', + ids: requiredPrIds.res_ids, + } + }); + // not sure why we need to wait for the round *after* the error update + // notification, but even debouncing the rest of the method is not + // sufficient (so it's not just a problem of being behind the mutex, + // there's something wonky going on) + if (!this._updateNext) { + this._updateNext = changed.includes('errors'); + return; + } + + this._updateNext = false; + for(const p of requiredPrIds.data) { + this.renderer.updateState(p.id, {fieldNames: ['state_color']}); + } + this.renderer.updateState(record, {fieldNames: ['errors', 'required_pr_ids']}); + }, + /** + * @override + */ + async start(...args) { + const checker = async () => { + if (this.isDestroyed()) { return; } + await this.model.mutex.exec(() => this._checkState()); + setTimeout(checker, 1000); + }; + const started = await this._super(...args); + const _ = checker(); + return started; + }, +}); + +viewRegistry.add('freeze_wizard', FormView.extend({ + config: Object.assign({}, FormView.prototype.config, { + Controller: FreezeController, + }) +})); +}); + diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 8b1e8f02..f2aaf471 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1074,3 +1074,154 @@ def test_multi_project(env, make_repo, setreviewers, users, config, assert pr2.comments == [ (users['user'], f'[Pull request status dashboard]({pr2_id.url}).'), ] + +def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): + """ Tests the freeze wizard feature (aside from the UI): + + * have a project with 3 repos, and two branches (1.0 and master) each + * have 2 PRs required for the freeze + * prep 3 freeze PRs + * trigger the freeze wizard + * trigger it again (check that the same object is returned, there should + only be one freeze per project at a time) + * configure the freeze + * check that it doesn't go through + * merge required PRs + * check that freeze goes through + * check that reminder is shown + * check that new branches are created w/ correct parent & commit info + """ + project.freeze_reminder = "Don't forget to like and subscribe" + + # have a project with 3 repos, and two branches (1.0 and master) + project.branch_ids = [ + (1, project.branch_ids.id, {'sequence': 1}), + (0, 0, {'name': '1.0', 'sequence': 2}), + ] + + masters = [] + for r in [repo_a, repo_b, repo_c]: + with r: + [root, _] = r.make_commits( + None, + Commit('base', tree={'version': '', 'f': '0'}), + Commit('release 1.0', tree={'version': '1.0'} if r is repo_a else None), + ref='heads/1.0' + ) + masters.extend(r.make_commits(root, Commit('other', tree={'f': '1'}), ref='heads/master')) + + # have 2 PRs required for the freeze + with repo_a: + repo_a.make_commits('master', Commit('super important file', tree={'g': 'x'}), ref='heads/apr') + pr_required_a = repo_a.make_pr(target='master', head='apr') + with repo_c: + repo_c.make_commits('master', Commit('update thing', tree={'f': '2'}), ref='heads/cpr') + pr_required_c = repo_c.make_pr(target='master', head='cpr') + + # have 3 release PRs, only the first one updates the tree (version file) + with repo_a: + repo_a.make_commits( + masters[0], + Commit('Release 1.1 (A)', tree={'version': '1.1'}), + ref='heads/release-1.1' + ) + pr_rel_a = repo_a.make_pr(target='master', head='release-1.1') + with repo_b: + repo_b.make_commits( + masters[1], + Commit('Release 1.1 (B)', tree=None), + ref='heads/release-1.1' + ) + pr_rel_b = repo_b.make_pr(target='master', head='release-1.1') + with repo_c: + repo_c.make_commits( + masters[2], + Commit('Release 1.1 (C)', tree=None), + ref='heads/release-1.1' + ) + pr_rel_c = repo_c.make_pr(target='master', head='release-1.1') + env.run_crons() # process the PRs + + release_prs = { + repo_a.name: to_pr(env, pr_rel_a), + repo_b.name: to_pr(env, pr_rel_b), + repo_c.name: to_pr(env, pr_rel_c), + } + + # trigger the ~~tree~~ freeze wizard + w = project.action_prepare_freeze() + w2 = project.action_prepare_freeze() + assert w == w2, "each project should only have one freeze wizard active at a time" + + w_id = env[w['res_model']].browse([w['res_id']]) + assert w_id.branch_name == '1.1', "check that the forking incremented the minor by 1" + assert len(w_id.release_pr_ids) == len(project.repo_ids), \ + "should ask for a many release PRs as we have repositories" + + # configure required PRs + w_id.required_pr_ids = (to_pr(env, pr_required_a) | to_pr(env, pr_required_c)).ids + # configure releases + for r in w_id.release_pr_ids: + r.pr_id = release_prs[r.repository_id.name].id + r = w_id.action_freeze() + assert r == w, "the freeze is not ready so the wizard should redirect to itself" + assert w_id.errors == "* 2 required PRs not ready." + + with repo_a: + pr_required_a.post_comment('hansen r+', config['role_reviewer']['token']) + repo_a.post_status('apr', 'success', 'ci/runbot') + repo_a.post_status('apr', 'success', 'legal/cla') + with repo_c: + pr_required_c.post_comment('hansen r+', config['role_reviewer']['token']) + repo_c.post_status('cpr', 'success', 'ci/runbot') + repo_c.post_status('cpr', 'success', 'legal/cla') + env.run_crons() + + for repo in [repo_a, repo_b, repo_c]: + with repo: + repo.post_status('staging.master', 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + env.run_crons() + + assert to_pr(env, pr_required_a).state == 'merged' + assert to_pr(env, pr_required_c).state == 'merged' + + assert not w_id.errors + + r = w_id.action_freeze() + # check that the wizard was deleted + assert not w_id.exists() + # check that the wizard pops out a reminder dialog (kinda) + assert r['res_model'] == 'runbot_merge.project' + assert r['res_id'] == project.id + + env.run_crons() # stage the release prs + for repo in [repo_a, repo_b, repo_c]: + with repo: + repo.post_status('staging.1.1', 'success', 'ci/runbot') + repo.post_status('staging.1.1', 'success', 'legal/cla') + env.run_crons() # get the release prs merged + for pr_id in release_prs.values(): + assert pr_id.target.name == '1.1' + assert pr_id.state == 'merged' + + c_a = repo_a.commit('1.1') + assert c_a.message.startswith('Release 1.1 (A)') + assert repo_a.read_tree(c_a) == { + 'f': '1', # from master + 'g': 'x', # from required pr + 'version': '1.1', # from release commit + } + c_a_parent = repo_a.commit(c_a.parents[0]) + assert c_a_parent.message.startswith('super important file') + assert c_a_parent.parents[0] == masters[0] + + c_b = repo_b.commit('1.1') + assert c_b.message.startswith('Release 1.1 (B)') + assert repo_b.read_tree(c_b) == {'f': '1', 'version': ''} + assert c_b.parents[0] == masters[1] + + c_c = repo_c.commit('1.1') + assert c_c.message.startswith('Release 1.1 (C)') + assert repo_c.read_tree(c_c) == {'f': '2', 'version': ''} + assert repo_c.commit(c_c.parents[0]).parents[0] == masters[2] diff --git a/runbot_merge/views/runbot_merge_project.xml b/runbot_merge/views/runbot_merge_project.xml index ee688756..2f670ed3 100644 --- a/runbot_merge/views/runbot_merge_project.xml +++ b/runbot_merge/views/runbot_merge_project.xml @@ -4,8 +4,14 @@ runbot_merge.project
+
-
@@ -27,6 +33,15 @@ + + + + + + + @@ -48,4 +63,16 @@ + + + Project Form + runbot_merge.project + +
+ + + +
+ +