From 82174ae66ee15fa5fdb4d1ad6057c4670e7e2ea8 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 11 Aug 2021 11:36:35 +0200 Subject: [PATCH] [IMP] *: add draft support to mergebot, kinda * Remove the forwardport creating PRs in draft, that was mostly to avoid codeowners triggering but we've removed the github one and hand-rolled it, so not a concern anymore. * Prevent merging `draft` PRs, the mergebot rejects approval on draft PRs and insults people. TBD (maybe): try to create *conflicting* forward-port PRs in draft so it's clearer they need to be *fixed*? Issue of not being able to do that on all private repositories remains so~~ Fixes #500 --- conftest.py | 37 ++++++++++++++++++++++++++-- forwardport/models/forwardport.py | 2 +- forwardport/models/project.py | 9 +------ forwardport/tests/test_conflicts.py | 4 +-- forwardport/tests/test_simple.py | 2 +- forwardport/tests/test_weird.py | 27 +++++++++++++++++++- runbot_merge/controllers/__init__.py | 13 +++++++--- runbot_merge/models/pull_requests.py | 6 ++++- runbot_merge/tests/test_multirepo.py | 1 + 9 files changed, 82 insertions(+), 19 deletions(-) diff --git a/conftest.py b/conftest.py index ed8526d3..9c441d4f 100644 --- a/conftest.py +++ b/conftest.py @@ -688,7 +688,7 @@ class Repo: )).raise_for_status() return PR(self, number) - def make_pr(self, *, title=None, body=None, target, head, token=None): + def make_pr(self, *, title=None, body=None, target, head, draft=False, token=None): assert self.hook self.hook = 2 @@ -717,6 +717,7 @@ class Repo: 'body': body, 'head': head, 'base': target, + 'draft': draft, }, headers=headers, ) @@ -774,6 +775,22 @@ class Comment(tuple): def __getitem__(self, item): return self._c[item] + +PR_SET_READY = ''' +mutation setReady($pid: ID!) { + markPullRequestReadyForReview(input: { pullRequestId: $pid}) { + clientMutationId + } +} +''' + +PR_SET_DRAFT = ''' +mutation setDraft($pid: ID!) { + convertPullRequestToDraft(input: { pullRequestId: $pid }) { + clientMutationId + } +} +''' class PR: def __init__(self, repo, number): self.repo = repo @@ -796,6 +813,22 @@ class PR: raise NotImplementedError() base = base.setter(lambda self, v: self._set_prop('base', v)) + @property + def draft(self): + return self._pr['draft'] + @draft.setter + def draft(self, v): + assert self.repo.hook + # apparently it's not possible to update the draft flag via the v3 API, + # only the V4... + r = self.repo._session.post('https://api.github.com/graphql', json={ + 'query': PR_SET_DRAFT if v else PR_SET_READY, + 'variables': {'pid': self._pr['node_id']} + }) + assert r.ok, r.text + out = r.json() + assert 'errors' not in out, out['errors'] + @property def head(self): return self._pr['head']['sha'] @@ -863,7 +896,7 @@ class PR: r = self.repo._session.patch('https://api.github.com/repos/{}/pulls/{}'.format(self.repo.name, self.number), json={ prop: value }, headers=headers) - assert 200 <= r.status_code < 300, r.json() + assert r.ok, r.text def open(self, token=None): self._set_prop('state', 'open', token=token) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 8caaca33..cb4db9b9 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -123,7 +123,7 @@ class UpdateQueue(models.Model, Queue): ) }) return - # QUESTION: update PR to draft if there are conflicts? + conflicts, working_copy = previous._create_fp_branch( child.target, child.refname, s) if conflicts: diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 86b11e5e..f1ede2e2 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -662,16 +662,9 @@ class PullRequests(models.Model): url = 'https://api.github.com/repos/{}/pulls'.format(pr.repository.name) pr_data = { 'base': target.name, 'head': '%s:%s' % (owner, new_branch), - 'title': title, 'body': body, 'draft': True + 'title': title, 'body': body } r = gh.post(url, json=pr_data) - if r.status_code == 422: - # assume this is a private repo which doesn't support draft PRs - # (github error response doesn't provide any machine - # information, only a human-readable message) so retry without - del pr_data['draft'] - r = gh.post(url, json=pr_data) - if not r.ok: _logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name) # delete all the branches this should automatically close the diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 42523c06..739c27b9 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -92,7 +92,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port time.sleep(5) env.run_crons() assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\ - "CI passing should not have resumed the FP process on a conflicting / draft PR" + "CI passing should not have resumed the FP process on a conflicting PR" # fix the PR, should behave as if this were a normal PR prc = prod.get_pr(prc_id.number) @@ -199,7 +199,7 @@ def test_conflict_deleted(env, config, make_repo): time.sleep(5) env.run_crons() assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\ - "CI passing should not have resumed the FP process on a conflicting / draft PR" + "CI passing should not have resumed the FP process on a conflicting PR" # fix the PR, should behave as if this were a normal PR get_pr = prod.get_pr(pr1.number) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 818f176b..d2698dbb 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -234,7 +234,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port def test_empty(env, config, make_repo, users): """ Cherrypick of an already cherrypicked (or separately implemented) - commit -> create draft PR. + commit -> conflicting pr. """ prod, other = make_basic(env, config, make_repo) # merge change to b diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index e0c2fcd4..57915f4b 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- import pytest -from utils import seen, Commit +from utils import seen, Commit, to_pr def make_basic(env, config, make_repo, *, fp_token, fp_remote): @@ -658,3 +658,28 @@ def test_skip_ci_next(env, config, make_repo): _, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number') assert pr1_id.state == 'opened' assert pr2_id.state == 'opened' + +def test_approve_draft(env, config, make_repo, users): + _, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True) + + with prod: + prod.make_commits('a', Commit('x', tree={'x': '0'}), ref='heads/change') + pr = prod.make_pr(target='a', head='change', draft=True) + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + pr_id = to_pr(env, pr) + assert pr_id.state == 'opened' + assert pr.comments == [ + (users['reviewer'], 'hansen r+'), + seen(env, pr, users), + (users['user'], f"I'm sorry, @{users['reviewer']}. Draft PRs can not be approved."), + ] + + with prod: + pr.draft = False + assert pr.draft is False + with prod: + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + assert pr_id.state == 'approved' diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index c91afcb2..199b4801 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -189,7 +189,14 @@ def handle_pr(env, event): 'head': pr['head']['sha'], 'squash': pr['commits'] == 1, }) - return 'Updated {} to {}'.format(pr_obj.id, pr_obj.head) + return 'Updated {} to {}'.format(pr_obj.display_name, pr_obj.head) + + if event['action'] == 'ready_for_review': + pr_obj.draft = False + return f'Updated {pr_obj.display_name} to ready' + if event['action'] == 'converted_to_draft': + pr_obj.draft = True + return f'Updated {pr_obj.display_name} to draft' # don't marked merged PRs as closed (!!!) if event['action'] == 'closed' and pr_obj.state != 'merged': @@ -201,7 +208,7 @@ def handle_pr(env, event): pr_obj.display_name, oldstate, ) - return 'Closed {}'.format(pr_obj.id) + return 'Closed {}'.format(pr_obj.display_name) else: _logger.warning( '%s tried to close %s (state=%s)', @@ -227,7 +234,7 @@ def handle_pr(env, event): 'squash': pr['commits'] == 1, }) - return 'Reopened {}'.format(pr_obj.id) + return 'Reopened {}'.format(pr_obj.display_name) _logger.info("Ignoring event %s on PR %s", event['action'], pr['number']) return "Not handling {} yet".format(event['action']) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 19d0f2b0..431def74 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -688,6 +688,7 @@ class PullRequests(models.Model): "cross-repository branch-matching" ) message = fields.Text(required=True) + draft = fields.Boolean(default=False, required=True) squash = fields.Boolean(default=False) merge_method = fields.Selection([ ('merge', "merge directly, using the PR as merge commit message"), @@ -999,7 +1000,9 @@ class PullRequests(models.Model): 'number': self.number, }) elif command == 'review': - if param and is_reviewer: + if self.draft: + msg = "Draft PRs can not be approved." + elif param and is_reviewer: oldstate = self.state newstate = RPLUS.get(self.state) if newstate: @@ -1250,6 +1253,7 @@ class PullRequests(models.Model): 'head': description['head']['sha'], 'squash': description['commits'] == 1, 'message': message, + 'draft': description['draft'], }) def write(self, vals): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index ebc90c61..8b1e8f02 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -902,6 +902,7 @@ class TestSubstitutions: }, 'pull_request': { 'state': 'open', + 'draft': False, 'user': {'login': 'bob'}, 'base': { 'repo': {'full_name': r.name},