From de927d89e759f8447010d3879b5586830c77aadd Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Thu, 8 Jul 2021 15:46:37 +0200 Subject: [PATCH] [IMP] runbot: cleanup and improve hook When getting pull info, the alive state can be determined easily, meaning that this field can join the "_compute_branch_infos" familly Hook was catching some changes made on pr and was conditionnaly updating some fields and triggering some other operations conditionaly depending on the action flag. All of the information needed to update the pull info should always be present in the payload body, meaning that all fields can be updated at once in case some hook was missed, and additionnal operation can be triggered based on fields changes. --- runbot/controllers/hook.py | 38 ++++++++------------------------------ runbot/models/branch.py | 22 ++++++++++++++++++++-- runbot/models/repo.py | 14 ++++++++------ 3 files changed, 36 insertions(+), 38 deletions(-) diff --git a/runbot/controllers/hook.py b/runbot/controllers/hook.py index 5b372dc3..d9c91d67 100644 --- a/runbot/controllers/hook.py +++ b/runbot/controllers/hook.py @@ -12,9 +12,8 @@ _logger = logging.getLogger(__name__) class Hook(http.Controller): - @http.route(['/runbot/hook/', '/runbot/hook/org'], type='http', auth="public", website=True, csrf=False) - def hook(self, remote_id=None, **post): - _logger.info('Hook on %s', remote_id) + @http.route(['/runbot/hook/'], type='http', auth="public", website=True, csrf=False) + def hook(self, remote_id=None, **_post): event = request.httprequest.headers.get("X-Github-Event") payload = json.loads(request.params.get('payload', '{}')) if remote_id is None: @@ -32,40 +31,19 @@ class Hook(http.Controller): remote = request.env['runbot.remote'].sudo().browse([remote_id]) # force update of dependencies too in case a hook is lost - if not payload or event == 'push' or (event == 'pull_request' and payload.get('action') in ('synchronize', 'opened', 'reopened')): + if not payload or event == 'push': remote.repo_id.set_hook_time(time.time()) elif event == 'pull_request': pr_number = payload.get('pull_request', {}).get('number', '') branch = request.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', pr_number)]) - if payload and payload.get('action', '') == 'edited' and 'base' in payload.get('changes'): - # handle PR that have been re-targeted - branch._compute_branch_infos(payload.get('pull_request', {})) - _logger.info('Retargeting %s to %s', branch.name, branch.target_branch_name) - base = request.env['runbot.bundle'].search([ - ('name', '=', branch.target_branch_name), - ('is_base', '=', True), - ('project_id', '=', branch.remote_id.repo_id.project_id.id) - ]) - if base and branch.bundle_id.defined_base_id != base: - _logger.info('Changing base of bundle %s to %s(%s)', branch.bundle_id, base.name, base.id) - branch.bundle_id.defined_base_id = base.id - branch.bundle_id._force() - elif payload.get('action') in ('ready_for_review', 'converted_to_draft'): - init_draft = branch.draft - branch._compute_branch_infos(payload.get('pull_request', {})) - if branch.draft != init_draft: - branch.bundle_id._force() - elif payload.get('action') in ('deleted', 'closed'): - _logger.info('Closing pr %s', branch.name) - branch.alive = False - else: - _logger.info('Ignoring unsupported pull request operation %s %s', event, payload.get('action', '')) + branch.recompute_infos(payload.get('pull_request', {})) + if payload.get('action') in ('synchronize', 'opened', 'reopened'): + remote.repo_id.set_hook_time(time.time()) + # remaining recurrent actions: labeled, review_requested, review_request_removed elif event == 'delete': if payload.get('ref_type') == 'branch': branch_ref = payload.get('ref') - _logger.info('Hook for branch deletion %s in repo %s', branch_ref, remote.repo_id.name) + _logger.info('Branch %s in repo %s was deleted', branch_ref, remote.repo_id.name) branch = request.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', branch_ref)]) branch.alive = False - else: - _logger.info('Ignoring unsupported hook %s %s', event, payload.get('action', '')) return "" diff --git a/runbot/models/branch.py b/runbot/models/branch.py index ddec5757..94e89049 100644 --- a/runbot/models/branch.py +++ b/runbot/models/branch.py @@ -104,6 +104,7 @@ class Branch(models.Model): if pi: try: branch.draft = pi.get('draft', False) + branch.alive = pi.get('state', False) != 'closed' branch.target_branch_name = pi['base']['ref'] branch.pull_head_name = pi['head']['label'] pull_head_repo_name = False @@ -199,9 +200,26 @@ class Branch(models.Model): self.name ) - def recompute_infos(self): + def recompute_infos(self, payload=None): """ public method to recompute infos on demand """ - self._compute_branch_infos() + was_draft = self.draft + was_alive = self.alive + init_target_branch_name = self.target_branch_name + self._compute_branch_infos(payload) + if self.target_branch_name != init_target_branch_name: + _logger.info('retargeting %s to %s', self.name, self.target_branch_name) + base = self.env['runbot.bundle'].search([ + ('name', '=', self.target_branch_name), + ('is_base', '=', True), + ('project_id', '=', self.remote_id.repo_id.project_id.id) + ]) + if base and self.bundle_id.defined_base_id != base: + _logger.info('Changing base of bundle %s to %s(%s)', self.bundle_id, base.name, base.id) + self.bundle_id.defined_base_id = base.id + self.bundle_id._force() + + if (not self.draft and was_draft) or (self.alive and not was_alive) or (self.target_branch_name != init_target_branch_name and self.alive): + self.bundle_id._force() @api.model def match_is_base(self, name): diff --git a/runbot/models/repo.py b/runbot/models/repo.py index cddb39d9..cc4b74a4 100644 --- a/runbot/models/repo.py +++ b/runbot/models/repo.py @@ -429,12 +429,14 @@ class Repo(models.Model): 'date': dateutil.parser.parse(date[:19]), }) branch.head = commit - branch.alive = True - # Not perfect, il some case a pr can be closed but still visible in repo. - # The head wont change but on creation the branch will be set alive even if git into said pr is closed - # It is still better to have false open than false closed + if not branch.alive: + if branch.is_pr: + _logger.info('Recomputing infos of dead pr %s', branch.name) + branch._compute_branch_infos() + else: + branch.alive = True - if branch.reference_name and branch.remote_id and branch.remote_id.repo_id._is_branch_forbidden(branch.reference_name ): + if branch.reference_name and branch.remote_id and branch.remote_id.repo_id._is_branch_forbidden(branch.reference_name): message = "This branch name is incorrect. Branch name should be prefixed with a valid version" message = branch.remote_id.repo_id.invalid_branch_message or message branch.head._github_status(False, "Branch naming", 'failure', False, message) @@ -446,7 +448,7 @@ class Repo(models.Model): if bundle.no_build: continue - if bundle.last_batch.state != 'preparing' and commit not in bundle.last_batch.commit_ids: + if bundle.last_batch.state != 'preparing': preparing = self.env['runbot.batch'].create({ 'last_update': fields.Datetime.now(), 'bundle_id': bundle.id,