From f845744bb9a05e64cd6df5a3764bdf5ceeed1744 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 4 Nov 2020 20:23:42 -0800 Subject: [PATCH 1/4] Parse `r+ await` --- homu/parse_issue_comment.py | 22 +++++++++- homu/tests/test_parse_issue_comment.py | 60 ++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index ce64c1b..538ce73 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -28,6 +28,13 @@ def approve(cls, approver, commit): command.actor = approver.lstrip('@') return command + @classmethod + def approve_await(cls, approver, commit): + command = cls('approve-await') + command.commit = commit + command.actor = approver.lstrip('@') + return command + @classmethod def unapprove(cls): return cls('unapprove') @@ -183,6 +190,7 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): if i + 1 < len(words) and is_sha(words[i + 1]): approved_sha = words[i + 1] words[i + 1] = None + i += 1 approver = word[len('r='):] if word.startswith('r=') else username @@ -190,8 +198,18 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): if approver == 'me': continue - commands.append( - IssueCommentCommand.approve(approver, approved_sha)) + await_ci = False + if i + 1 < len(words) and words[i + 1] == 'await': + await_ci = True + words[i + 1] = None + i += 1 + + if await_ci: + commands.append( + IssueCommentCommand.approve_await(approver, approved_sha)) + else: + commands.append( + IssueCommentCommand.approve(approver, approved_sha)) elif word == 'r-': commands.append(IssueCommentCommand.unapprove()) diff --git a/homu/tests/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py index 34da29f..76e1784 100644 --- a/homu/tests/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -52,6 +52,36 @@ def test_r_plus_with_sha(): assert command.actor == 'jack' assert command.commit == other_commit +def test_r_plus_await(): + """ + @bors r+ await + """ + + author = "jack" + body = "@bors r+ await" + commands = parse_issue_comment(author, body, commit, "bors") + + assert len(commands) == 1 + command = commands[0] + assert command.action == 'approve-await' + assert command.actor == 'jack' + + +def test_r_plus_await_with_sha(): + """ + @bors r+ {sha} await + """ + + author = "jack" + body = "@bors r+ {} await".format(other_commit) + commands = parse_issue_comment(author, body, commit, "bors") + + assert len(commands) == 1 + command = commands[0] + assert command.action == 'approve-await' + assert command.actor == 'jack' + assert command.commit == other_commit + def test_r_equals(): """ @@ -83,6 +113,36 @@ def test_r_equals_at_user(): assert command.actor == 'jill' +def test_r_equals_await(): + """ + @bors r=jill await + """ + + author = "jack" + body = "@bors r=jill await" + commands = parse_issue_comment(author, body, commit, "bors") + + assert len(commands) == 1 + command = commands[0] + assert command.action == 'approve-await' + assert command.actor == 'jill' + + +def test_r_equals_at_user(): + """ + @bors r=@jill await + """ + + author = "jack" + body = "@bors r=@jill await" + commands = parse_issue_comment(author, body, commit, "bors") + + assert len(commands) == 1 + command = commands[0] + assert command.action == 'approve-await' + assert command.actor == 'jill' + + def test_hidden_r_equals(): author = "bors" body = """ From 6dc88f3b19a7518276ee8a5fd05a1110dd65d1b8 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 4 Nov 2020 21:09:28 -0800 Subject: [PATCH 2/4] Fix mistake --- homu/parse_issue_comment.py | 2 +- homu/tests/test_parse_issue_comment.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index 538ce73..a439eae 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -206,7 +206,7 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): if await_ci: commands.append( - IssueCommentCommand.approve_await(approver, approved_sha)) + IssueCommentCommand.approve_await(approver, approved_sha)) # noqa else: commands.append( IssueCommentCommand.approve(approver, approved_sha)) diff --git a/homu/tests/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py index 76e1784..d636f53 100644 --- a/homu/tests/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -52,6 +52,7 @@ def test_r_plus_with_sha(): assert command.actor == 'jack' assert command.commit == other_commit + def test_r_plus_await(): """ @bors r+ await @@ -128,7 +129,7 @@ def test_r_equals_await(): assert command.actor == 'jill' -def test_r_equals_at_user(): +def test_r_equals_at_user_await(): """ @bors r=@jill await """ From cbe2e223cb3926a7b34b0770b96959f66f8e14e3 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 4 Nov 2020 21:15:03 -0800 Subject: [PATCH 3/4] Implement first part of `r+ await` handler This validates the approval and then records in the database to await CI success. I still need to implement the hard part, which will be handling the CI success event. --- homu/comments.py | 27 +++++++ homu/main.py | 194 ++++++++++++++++++++++++++--------------------- 2 files changed, 136 insertions(+), 85 deletions(-) diff --git a/homu/comments.py b/homu/comments.py index d289931..09e9bd6 100644 --- a/homu/comments.py +++ b/homu/comments.py @@ -45,6 +45,33 @@ def render(self): ) +class ApprovedAwait(Comment): + def __init__(self, bot=None, **args): + # Because homu needs to leave a comment for itself to kick off a build, + # we need to know the correct botname to use. However, we don't want to + # save that botname in our state JSON. So we need a custom constructor + # to grab the botname and delegate the rest of the keyword args to the + # Comment constructor. + super().__init__(**args) + self.bot = bot + + params = ["sha", "approver"] + + def render(self): + # The comment here is required because Homu wants a full, unambiguous, + # pinned commit hash to kick off the build, and this note-to-self is + # how it gets it. This is to safeguard against situations where Homu + # reloads and another commit has been pushed since the approval. + message = ":pushpin: Commit {sha} will be " + \ + "approved by `{approver}` once CI passes\n\n" + \ + "" + return message.format( + sha=self.sha, + approver=self.approver, + bot=self.bot + ) + + class ApprovalIgnoredWip(Comment): def __init__(self, wip_keyword=None, **args): # We want to use the wip keyword in the message, but not in the json diff --git a/homu/main.py b/homu/main.py index 65ccaae..1f5bbc9 100644 --- a/homu/main.py +++ b/homu/main.py @@ -465,6 +465,7 @@ class AuthState(IntEnum): class LabelEvent(Enum): APPROVED = 'approved' + APPROVED_AWAITING_CI = 'approved_awaiting_ci' REJECTED = 'rejected' CONFLICT = 'conflict' SUCCEED = 'succeed' @@ -520,95 +521,27 @@ def parse_commands(body, username, user_id, repo_label, repo_cfg, state, for command in commands: found = True if command.action == 'approve': - if not _reviewer_auth_verified(): + result = _approve_common( + _reviewer_auth_verified=_reviewer_auth_verified, + command=command, + state=state, + comment_factory=comments.Approved, + success_label=LabelEvent.APPROVED, + ) + if result == 'skip': continue - approver = command.actor - cur_sha = command.commit - - # Ignore WIP PRs - is_wip = False - for wip_kw in ['WIP', 'TODO', '[WIP]', '[TODO]', '[DO NOT MERGE]']: - if state.title.upper().startswith(wip_kw): - if realtime: - state.add_comment(comments.ApprovalIgnoredWip( - sha=state.head_sha, - wip_keyword=wip_kw, - )) - is_wip = True - break - if is_wip: + elif command.action == 'approve-await': + result = _approve_common( + _reviewer_auth_verified=_reviewer_auth_verified, + command=command, + state=state, + comment_factory=comments.ApprovedAwait, + success_label=LabelEvent.APPROVED_AWAITING_CI, + ) + if result == 'skip': continue - # Sometimes, GitHub sends the head SHA of a PR as 0000000 - # through the webhook. This is called a "null commit", and - # seems to happen when GitHub internally encounters a race - # condition. Last time, it happened when squashing commits - # in a PR. In this case, we just try to retrieve the head - # SHA manually. - if all(x == '0' for x in state.head_sha): - if realtime: - state.add_comment( - ':bangbang: Invalid head SHA found, retrying: `{}`' - .format(state.head_sha) - ) - - state.head_sha = state.get_repo().pull_request(state.num).head.sha # noqa - state.save() - - assert any(x != '0' for x in state.head_sha) - - if state.approved_by and realtime and username != my_username: - for _state in states[state.repo_label].values(): - if _state.status == 'pending': - break - else: - _state = None - - lines = [] - - if state.status in ['failure', 'error']: - lines.append('- This pull request previously failed. You should add more commits to fix the bug, or use `retry` to trigger a build again.') # noqa - - if _state: - if state == _state: - lines.append('- This pull request is currently being tested. If there\'s no response from the continuous integration service, you may use `retry` to trigger a build again.') # noqa - else: - lines.append('- There\'s another pull request that is currently being tested, blocking this pull request: #{}'.format(_state.num)) # noqa - - if lines: - lines.insert(0, '') - lines.insert(0, ':bulb: This pull request was already approved, no need to approve it again.') # noqa - - state.add_comment('\n'.join(lines)) - - if sha_cmp(cur_sha, state.head_sha): - state.approved_by = approver - state.try_ = False - state.set_status('') - - state.save() - elif realtime and username != my_username: - if cur_sha: - msg = '`{}` is not a valid commit SHA.'.format(cur_sha) - state.add_comment( - ':scream_cat: {} Please try again with `{}`.' - .format(msg, state.head_sha) - ) - else: - state.add_comment(comments.Approved( - sha=state.head_sha, - approver=approver, - bot=my_username, - )) - treeclosed = state.blocked_by_closed_tree() - if treeclosed: - state.add_comment( - ':evergreen_tree: The tree is currently closed for pull requests below priority {}, this pull request will be tested once the tree is reopened' # noqa - .format(treeclosed) - ) - state.change_labels(LabelEvent.APPROVED) - elif command.action == 'unapprove': # Allow the author of a pull request to unapprove their own PR. The # author can already perform other actions that effectively @@ -807,6 +740,97 @@ def parse_commands(body, username, user_id, repo_label, repo_cfg, state, return state_changed +def _approve_common(_reviewer_auth_verified, command, state, realtime, comment_factory, success_label): + if not _reviewer_auth_verified(): + return 'skip' + + approver = command.actor + cur_sha = command.commit + + # Ignore WIP PRs + is_wip = False + for wip_kw in ['WIP', 'TODO', '[WIP]', '[TODO]', '[DO NOT MERGE]']: + if state.title.upper().startswith(wip_kw): + if realtime: + state.add_comment(comments.ApprovalIgnoredWip( + sha=state.head_sha, + wip_keyword=wip_kw, + )) + is_wip = True + break + if is_wip: + return 'skip' + + # Sometimes, GitHub sends the head SHA of a PR as 0000000 + # through the webhook. This is called a "null commit", and + # seems to happen when GitHub internally encounters a race + # condition. Last time, it happened when squashing commits + # in a PR. In this case, we just try to retrieve the head + # SHA manually. + if all(x == '0' for x in state.head_sha): + if realtime: + state.add_comment( + ':bangbang: Invalid head SHA found, retrying: `{}`' + .format(state.head_sha) + ) + + state.head_sha = state.get_repo().pull_request(state.num).head.sha # noqa + state.save() + + assert any(x != '0' for x in state.head_sha) + + if state.approved_by and realtime and username != my_username: + for _state in states[state.repo_label].values(): + if _state.status == 'pending': + break + else: + _state = None + + lines = [] + + if state.status in ['failure', 'error']: + lines.append('- This pull request previously failed. You should add more commits to fix the bug, or use `retry` to trigger a build again.') # noqa + + if _state: + if state == _state: + lines.append('- This pull request is currently being tested. If there\'s no response from the continuous integration service, you may use `retry` to trigger a build again.') # noqa + else: + lines.append('- There\'s another pull request that is currently being tested, blocking this pull request: #{}'.format(_state.num)) # noqa + + if lines: + lines.insert(0, '') + lines.insert(0, ':bulb: This pull request was already approved, no need to approve it again.') # noqa + + state.add_comment('\n'.join(lines)) + + if sha_cmp(cur_sha, state.head_sha): + state.approved_by = approver + state.try_ = False + state.set_status('') + + state.save() + elif realtime and username != my_username: + if cur_sha: + msg = '`{}` is not a valid commit SHA.'.format(cur_sha) + state.add_comment( + ':scream_cat: {} Please try again with `{}`.' + .format(msg, state.head_sha) + ) + else: + state.add_comment(comment_factory( + sha=state.head_sha, + approver=approver, + bot=my_username, + )) + treeclosed = state.blocked_by_closed_tree() + if treeclosed: + state.add_comment( + ':evergreen_tree: The tree is currently closed for pull requests below priority {}, this pull request will be tested once the tree is reopened' # noqa + .format(treeclosed) + ) + state.change_labels(success_label) + + def handle_hook_response(state, hook_cfg, body, extra_data): post_data = {} post_data["pull"] = state.num From 78492f8452574c71023a2b8d241f5dd5bf5a5d45 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 4 Nov 2020 21:18:40 -0800 Subject: [PATCH 4/4] Fix mistakes --- homu/main.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/homu/main.py b/homu/main.py index 1f5bbc9..7880a65 100644 --- a/homu/main.py +++ b/homu/main.py @@ -524,7 +524,10 @@ def parse_commands(body, username, user_id, repo_label, repo_cfg, state, result = _approve_common( _reviewer_auth_verified=_reviewer_auth_verified, command=command, + states=states, state=state, + username=username, + my_username=my_username, comment_factory=comments.Approved, success_label=LabelEvent.APPROVED, ) @@ -535,7 +538,10 @@ def parse_commands(body, username, user_id, repo_label, repo_cfg, state, result = _approve_common( _reviewer_auth_verified=_reviewer_auth_verified, command=command, + states=states, state=state, + username=username, + my_username=my_username, comment_factory=comments.ApprovedAwait, success_label=LabelEvent.APPROVED_AWAITING_CI, ) @@ -740,7 +746,17 @@ def parse_commands(body, username, user_id, repo_label, repo_cfg, state, return state_changed -def _approve_common(_reviewer_auth_verified, command, state, realtime, comment_factory, success_label): +def _approve_common( + _reviewer_auth_verified, + command, + states, + state, + username, + my_username, + realtime, + comment_factory, + success_label, +): if not _reviewer_auth_verified(): return 'skip'