From f1fd009c53fa90e26b9277b08e21e7c163cfafe3 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 7 Jun 2019 08:15:29 -0500 Subject: [PATCH 01/17] Test authorization code Pull code that creates a comment on a pull request out of the authorization check, and add tests around the authorization checks. After pulling out the code that creates a comment, we still need to know what the text of the comment will be, so change from a function that returns a True on success and a False on failure to a function that returns a True on success and raises an Exception (with the failure comment) on failure. --- .travis.yml | 2 +- homu/auth.py | 49 ++-- homu/main.py | 496 ++++++++++++++++++++-------------------- homu/tests/test_auth.py | 462 +++++++++++++++++++++++++++++++++++++ setup.py | 1 + 5 files changed, 742 insertions(+), 268 deletions(-) create mode 100644 homu/tests/test_auth.py diff --git a/.travis.yml b/.travis.yml index 9a8a947..cb73f3b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,5 +7,5 @@ install: - pip install flake8 script: - flake8 homu - - pip install -e . + - pip install -e .[test] - python setup.py test diff --git a/homu/auth.py b/homu/auth.py index cb25704..4780ffc 100644 --- a/homu/auth.py +++ b/homu/auth.py @@ -1,9 +1,29 @@ import requests +from enum import IntEnum RUST_TEAM_BASE = "https://team-api.infra.rust-lang.org/v1/" +class AuthorizationException(Exception): + """ + The exception thrown when a user is not authorized to perform an action + """ + + comment = None + + def __init__(self, message, comment): + super().__init__(message) + self.comment = comment + + +class AuthState(IntEnum): + # Higher is more privileged + REVIEWER = 3 + TRY = 2 + NONE = 1 + + def fetch_rust_team(repo_label, level): repo = repo_label.replace('-', '_') url = RUST_TEAM_BASE + "permissions/bors." + repo + "." + level + ".json" @@ -31,18 +51,14 @@ def verify_level(username, repo_label, repo_cfg, state, toml_keys, return authorized -def verify(username, repo_label, repo_cfg, state, auth, realtime, my_username): - # The import is inside the function to prevent circular imports: main.py - # requires auth.py and auth.py requires main.py - from .main import AuthState - +def assert_authorized(username, repo_label, repo_cfg, state, auth, botname): # In some cases (e.g. non-fully-qualified r+) we recursively talk to # ourself via a hidden markdown comment in the message. This is so that # when re-synchronizing after shutdown we can parse these comments and # still know the SHA for the approval. # # So comments from self should always be allowed - if username == my_username: + if username == botname: return True authorized = False @@ -59,14 +75,13 @@ def verify(username, repo_label, repo_cfg, state, auth, realtime, my_username): if authorized: return True else: - if realtime: - reply = '@{}: :key: Insufficient privileges: '.format(username) - if auth == AuthState.REVIEWER: - if repo_cfg.get('auth_collaborators', False): - reply += 'Collaborator required' - else: - reply += 'Not in reviewers' - elif auth == AuthState.TRY: - reply += 'not in try users' - state.add_comment(reply) - return False + reply = '@{}: :key: Insufficient privileges: '.format(username) + if auth == AuthState.REVIEWER: + if repo_cfg.get('auth_collaborators', False): + reply += 'Collaborator required' + else: + reply += 'Not in reviewers' + elif auth == AuthState.TRY: + reply += 'not in try users' + raise AuthorizationException( + 'Authorization failed for user {}'.format(username), reply) diff --git a/homu/main.py b/homu/main.py index e29ba81..ea8e702 100644 --- a/homu/main.py +++ b/homu/main.py @@ -7,7 +7,11 @@ from . import comments from . import utils from .parse_issue_comment import parse_issue_comment -from .auth import verify as verify_auth +from .auth import ( + assert_authorized, + AuthorizationException, + AuthState, +) from .utils import lazy_debug import logging from threading import Thread, Lock, Timer @@ -19,7 +23,7 @@ from queue import Queue import os import sys -from enum import IntEnum, Enum +from enum import Enum import subprocess from .git_helper import SSH_KEY_FILE import shlex @@ -424,13 +428,6 @@ def sha_or_blank(sha): return sha if re.match(r'^[0-9a-f]+$', sha) else '' -class AuthState(IntEnum): - # Higher is more privileged - REVIEWER = 3 - TRY = 2 - NONE = 1 - - class LabelEvent(Enum): APPROVED = 'approved' REJECTED = 'rejected' @@ -455,24 +452,22 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, global global_cfg state_changed = False - _reviewer_auth_verified = functools.partial( - verify_auth, + _assert_reviewer_auth_verified = functools.partial( + assert_authorized, username, repo_label, repo_cfg, state, AuthState.REVIEWER, - realtime, my_username, ) - _try_auth_verified = functools.partial( - verify_auth, + _assert_try_auth_verified = functools.partial( + assert_authorized, username, repo_label, repo_cfg, state, AuthState.TRY, - realtime, my_username, ) @@ -483,289 +478,290 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, commands = parse_issue_comment(username, body, sha, my_username, hooks) for command in commands: - found = True - if command.action == 'approve': - if not _reviewer_auth_verified(): - continue - - approver = command.actor - cur_sha = command.commit + try: + found = True + if command.action == 'approve': + _assert_reviewer_auth_verified() + + 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: + continue - # Ignore WIP PRs - is_wip = False - for wip_kw in ['WIP', 'TODO', '[WIP]', '[TODO]', '[DO NOT MERGE]']: - if state.title.upper().startswith(wip_kw): + # 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(comments.ApprovalIgnoredWip( - sha=state.head_sha, - wip_keyword=wip_kw, - )) - is_wip = True - break - if is_wip: - 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.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() + 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) + 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 + 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 = [] + 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.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 _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 + 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)) + state.add_comment('\n'.join(lines)) - if sha_cmp(cur_sha, state.head_sha): - state.approved_by = approver - state.try_ = False - state.set_status('') + 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 + # unapprove the PR (change the target branch, push more + # commits, etc.) so allowing them to directly unapprove it is + # also allowed. + if state.author != username: + assert_authorized(username, repo_label, repo_cfg, state, + AuthState.REVIEWER, my_username) + + state.approved_by = '' 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: + if realtime: + state.change_labels(LabelEvent.REJECTED) + + elif command.action == 'prioritize': + assert_authorized(username, repo_label, repo_cfg, state, + AuthState.TRY, my_username) + + pvalue = command.priority + + if pvalue > global_cfg['max_priority']: + if realtime: 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) + ':stop_sign: Priority higher than {} is ignored.' + .format(global_cfg['max_priority']) ) - 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 - # unapprove the PR (change the target branch, push more commits, - # etc.) so allowing them to directly unapprove it is also allowed. - - # Because verify_auth has side-effects (especially, it may leave a - # comment on the pull request if the user is not authorized), we - # need to do the author check BEFORE the verify_auth check. - if state.author != username: - if not verify_auth(username, repo_label, repo_cfg, state, - AuthState.REVIEWER, realtime, my_username): continue + state.priority = pvalue + state.save() - state.approved_by = '' - state.save() - if realtime: - state.change_labels(LabelEvent.REJECTED) - - elif command.action == 'prioritize': - if not verify_auth(username, repo_label, repo_cfg, state, - AuthState.TRY, realtime, my_username): - continue + elif command.action == 'delegate': + assert_authorized(username, repo_label, repo_cfg, state, + AuthState.REVIEWER, my_username) - pvalue = command.priority + state.delegate = command.delegate_to + state.save() - if pvalue > global_cfg['max_priority']: if realtime: - state.add_comment( - ':stop_sign: Priority higher than {} is ignored.' - .format(global_cfg['max_priority']) - ) - continue - state.priority = pvalue - state.save() + state.add_comment(comments.Delegated( + delegator=username, + delegate=state.delegate + )) - elif command.action == 'delegate': - if not verify_auth(username, repo_label, repo_cfg, state, - AuthState.REVIEWER, realtime, my_username): - continue + elif command.action == 'undelegate': + # TODO: why is this a TRY? + _assert_try_auth_verified() - state.delegate = command.delegate_to - state.save() + state.delegate = '' + state.save() - if realtime: - state.add_comment(comments.Delegated( - delegator=username, - delegate=state.delegate - )) + elif command.action == 'delegate-author': + _assert_reviewer_auth_verified() - elif command.action == 'undelegate': - # TODO: why is this a TRY? - if not _try_auth_verified(): - continue - state.delegate = '' - state.save() + state.delegate = state.get_repo().pull_request(state.num).user.login # noqa + state.save() - elif command.action == 'delegate-author': - if not _reviewer_auth_verified(): - continue + if realtime: + state.add_comment(comments.Delegated( + delegator=username, + delegate=state.delegate + )) - state.delegate = state.get_repo().pull_request(state.num).user.login # noqa - state.save() + elif command.action == 'retry' and realtime: + _assert_try_auth_verified() - if realtime: - state.add_comment(comments.Delegated( - delegator=username, - delegate=state.delegate - )) + state.set_status('') + if realtime: + if state.try_: + event = LabelEvent.TRY + else: + event = LabelEvent.APPROVED + state.record_retry_log(command_src, body) + state.change_labels(event) - elif command.action == 'retry' and realtime: - if not _try_auth_verified(): - continue - state.set_status('') - if realtime: - event = LabelEvent.TRY if state.try_ else LabelEvent.APPROVED - state.record_retry_log(command_src, body) - state.change_labels(event) + elif command.action in ['try', 'untry'] and realtime: + _assert_try_auth_verified() - elif command.action in ['try', 'untry'] and realtime: - if not _try_auth_verified(): - continue - if state.status == '' and state.approved_by: - state.add_comment( - ':no_good: ' - 'Please do not `try` after a pull request has been `r+`ed.' - ' If you need to `try`, unapprove (`r-`) it first.' - ) - continue + if state.status == '' and state.approved_by: + state.add_comment( + ':no_good: ' + 'Please do not `try` after a pull request has' + ' been `r+`ed.' + ' If you need to `try`, unapprove (`r-`) it first.' + ) + continue - state.try_ = command.action == 'try' + state.try_ = command.action == 'try' - state.merge_sha = '' - state.init_build_res([]) + state.merge_sha = '' + state.init_build_res([]) - state.save() - if realtime and state.try_: - # If we've tried before, the status will be 'success', and this - # new try will not be picked up. Set the status back to '' - # so the try will be run again. - state.set_status('') - # `try-` just resets the `try` bit and doesn't correspond to - # any meaningful labeling events. - state.change_labels(LabelEvent.TRY) + state.save() + if realtime and state.try_: + # If we've tried before, the status will be 'success', and + # this new try will not be picked up. Set the status back + # to '' so the try will be run again. + state.set_status('') + # `try-` just resets the `try` bit and doesn't correspond + # to any meaningful labeling events. + state.change_labels(LabelEvent.TRY) - elif command.action == 'rollup': - if not _try_auth_verified(): - continue - state.rollup = command.rollup_value + elif command.action == 'rollup': + _assert_try_auth_verified() - state.save() + state.rollup = command.rollup_value - elif command.action == 'force' and realtime: - if not _try_auth_verified(): - continue - if 'buildbot' in repo_cfg: - with buildbot_sess(repo_cfg) as sess: - res = sess.post( - repo_cfg['buildbot']['url'] + '/builders/_selected/stopselected', # noqa - allow_redirects=False, - data={ - 'selected': repo_cfg['buildbot']['builders'], - 'comments': INTERRUPTED_BY_HOMU_FMT.format(int(time.time())), # noqa - }) + state.save() - if 'authzfail' in res.text: - err = 'Authorization failed' - else: - mat = re.search('(?s)
(.*?)
', res.text) - if mat: - err = mat.group(1).strip() - if not err: - err = 'Unknown error' + elif command.action == 'force' and realtime: + _assert_try_auth_verified() + + if 'buildbot' in repo_cfg: + with buildbot_sess(repo_cfg) as sess: + res = sess.post( + repo_cfg['buildbot']['url'] + '/builders/_selected/stopselected', # noqa + allow_redirects=False, + data={ + 'selected': repo_cfg['buildbot']['builders'], + 'comments': INTERRUPTED_BY_HOMU_FMT.format(int(time.time())), # noqa + }) + + if 'authzfail' in res.text: + err = 'Authorization failed' else: - err = '' + mat = re.search('(?s)
(.*?)
', res.text) # noqa + if mat: + err = mat.group(1).strip() + if not err: + err = 'Unknown error' + else: + err = '' - if err: - state.add_comment( - ':bomb: Buildbot returned an error: `{}`'.format(err) - ) + if err: + state.add_comment( + ':bomb: Buildbot returned an error: `{}`'.format(err) + ) - elif command.action == 'clean' and realtime: - if not _try_auth_verified(): - continue - state.merge_sha = '' - state.init_build_res([]) + elif command.action == 'clean' and realtime: + _assert_try_auth_verified() - state.save() + state.merge_sha = '' + state.init_build_res([]) - elif command.action == 'ping' and realtime: - if command.ping_type == 'portal': - state.add_comment( - ":cake: {}\n\n![]({})".format( - random.choice(PORTAL_TURRET_DIALOG), - PORTAL_TURRET_IMAGE) - ) - else: - state.add_comment(":sleepy: I'm awake I'm awake") + state.save() - elif command.action == 'treeclosed': - if not _reviewer_auth_verified(): - continue - state.change_treeclosed(command.treeclosed_value, command_src) - state.save() + elif command.action == 'ping' and realtime: + if command.ping_type == 'portal': + state.add_comment( + ":cake: {}\n\n![]({})".format( + random.choice(PORTAL_TURRET_DIALOG), + PORTAL_TURRET_IMAGE) + ) + else: + state.add_comment(":sleepy: I'm awake I'm awake") - elif command.action == 'untreeclosed': - if not _reviewer_auth_verified(): - continue - state.change_treeclosed(-1, None) - state.save() + elif command.action == 'treeclosed': + _assert_reviewer_auth_verified() - elif command.action == 'hook': - hook = command.hook_name - hook_cfg = global_cfg['hooks'][hook] - if hook_cfg['realtime'] and not realtime: - continue - if hook_cfg['access'] == "reviewer": - if not _reviewer_auth_verified(): + state.change_treeclosed(command.treeclosed_value, command_src) + state.save() + + elif command.action == 'untreeclosed': + _assert_reviewer_auth_verified() + + state.change_treeclosed(-1, None) + state.save() + + elif command.action == 'hook': + hook = command.hook_name + hook_cfg = global_cfg['hooks'][hook] + if hook_cfg['realtime'] and not realtime: continue + if hook_cfg['access'] == "reviewer": + _assert_reviewer_auth_verified() + else: + _assert_try_auth_verified() + + Thread( + target=handle_hook_response, + args=[state, hook_cfg, body, command.hook_extra] + ).start() + else: - if not _try_auth_verified(): - continue - Thread( - target=handle_hook_response, - args=[state, hook_cfg, body, command.hook_extra] - ).start() + found = False - else: - found = False + if found: + state_changed = True - if found: - state_changed = True + except AuthorizationException as e: + if realtime: + state.add_comment(e.comment) return state_changed diff --git a/homu/tests/test_auth.py b/homu/tests/test_auth.py new file mode 100644 index 0000000..1664d09 --- /dev/null +++ b/homu/tests/test_auth.py @@ -0,0 +1,462 @@ +import pytest +import re +import unittest.mock +import httpretty +from homu.auth import ( + AuthorizationException, + AuthState, + assert_authorized, +) + + +class TestBasic: + """ + Test basic authorization using `reviewers` and `try_users` + """ + + def test_reviewers(self): + state = unittest.mock.Mock() + state.delegate = 'david' + auth = AuthState.REVIEWER + repo_configuration = { + 'reviewers': ['alice'], + 'try_users': ['bob'], + } + + # The bot is successful + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A reviewer is successful + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A try user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*reviewer', + str(context.value.comment)) is not None + + # An unauthorized user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'eve', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*reviewers', + str(context.value.comment)) is not None + + # The delegated user is successful + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + def test_try(self): + state = unittest.mock.Mock() + state.delegate = 'david' + auth = AuthState.TRY + repo_configuration = { + 'reviewers': ['alice'], + 'try_users': ['bob'], + } + + # The bot is successful + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A reviewer is successful + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A try user is successful + result = assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # An unauthorized user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'eve', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*try users', + str(context.value.comment)) is not None + + # The delegated user is successful + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + +class TestCollaborator: + """ + Test situations when auth_collaborators is set + """ + + def test_reviewers(self): + repo = unittest.mock.Mock() + repo.is_collaborator = unittest.mock.Mock() + repo.is_collaborator.return_value = False + + state = unittest.mock.Mock() + state.delegate = 'david' + state.get_repo = unittest.mock.Mock(return_value=repo) + + auth = AuthState.REVIEWER + repo_configuration = { + 'auth_collaborators': True, + 'reviewers': [], + 'try_users': [], + } + + # The bot is successful + repo.is_collaborator.return_value = False + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A collaborator is successful + repo.is_collaborator.return_value = True + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A non-collaborator is not successful + repo.is_collaborator.return_value = False + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*Collaborator required', + str(context.value.comment)) is not None + + # The delegated user is successful + repo.is_collaborator.return_value = False + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + def test_try(self): + repo = unittest.mock.Mock() + repo.is_collaborator = unittest.mock.Mock() + repo.is_collaborator.return_value = False + + state = unittest.mock.Mock() + state.delegate = 'david' + state.get_repo = unittest.mock.Mock(return_value=repo) + + auth = AuthState.TRY + repo_configuration = { + 'auth_collaborators': True, + 'reviewers': [], + 'try_users': [], + } + + # The bot is successful + repo.is_collaborator.return_value = False + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A collaborator is successful + repo.is_collaborator.return_value = True + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A non-collaborator is not successful + repo.is_collaborator.return_value = False + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*try users', + str(context.value.comment)) is not None + + # The delegated user is successful + repo.is_collaborator.return_value = False + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + +class TestAuthRustTeam: + """ + Test situations where rust_team authorization is set + """ + + @httpretty.activate + def test_reviewers(self): + httpretty.register_uri( + httpretty.GET, + 'https://team-api.infra.rust-lang.org/v1/permissions/bors.test.review.json', # noqa + body=""" + { + "github_users": [ + "alice" + ] + } + """) + + state = unittest.mock.Mock() + state.delegate = 'david' + auth = AuthState.REVIEWER + repo_configuration = { + 'rust_team': True, + 'reviewers': ['alice'], + 'try_users': ['bob'], + } + + # The bot is successful + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A reviewer is successful + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A try user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*reviewer', + str(context.value.comment)) is not None + + # An unauthorized user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'eve', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*reviewer', + str(context.value.comment)) is not None + + # The delegated user is successful + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + @httpretty.activate + def test_try(self): + httpretty.register_uri( + httpretty.GET, + 'https://team-api.infra.rust-lang.org/v1/permissions/bors.test.try.json', # noqa + body=""" + { + "github_users": [ + "alice", + "bob" + ] + } + """) + + state = unittest.mock.Mock() + state.delegate = 'david' + auth = AuthState.TRY + repo_configuration = { + 'rust_team': True, + 'reviewers': ['alice'], + 'try_users': ['bob'], + } + + # The bot is successful + result = assert_authorized( + 'bors', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A reviewer is successful + result = assert_authorized( + 'alice', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # A try user is successful + result = assert_authorized( + 'bob', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True + + # An unauthorized user gets rejected + with pytest.raises(AuthorizationException) as context: + assert_authorized( + 'eve', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert re.search( + r'Insufficient privileges.*try users', + str(context.value.comment)) is not None + + # The delegated user is successful + result = assert_authorized( + 'david', + 'test', + repo_configuration, + state, + auth, + 'bors') + + assert result is True diff --git a/setup.py b/setup.py index d69845c..7210967 100644 --- a/setup.py +++ b/setup.py @@ -24,6 +24,7 @@ ], tests_require=[ 'pytest', + 'httpretty', ], package_data={ 'homu': [ From 60538211ade4e7478c0cf6837095a9081b7d2cf6 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 31 May 2019 06:51:58 -0500 Subject: [PATCH 02/17] Replace db_query with LockingDatabase Remove the free function `db_query` in favor of a `LockingDatabase` class that wraps a database connection. This is done so that, in order for a class to use the database, it only needs its `db` instance, instead of needing both a `db` instance and a `db_query` function. This allows us to break out classes into separate files. --- homu/main.py | 98 +++++++++++++++++++++++--------------------------- homu/server.py | 22 ++++++------ 2 files changed, 55 insertions(+), 65 deletions(-) diff --git a/homu/main.py b/homu/main.py index ea8e702..51d99e3 100644 --- a/homu/main.py +++ b/homu/main.py @@ -71,12 +71,20 @@ def buildbot_sess(repo_cfg): sess.get(repo_cfg['buildbot']['url'] + '/logout', allow_redirects=False) -db_query_lock = Lock() +class LockingDatabase: + def __init__(self, db): + self.db = db + self.query_lock = Lock() + + def execute(self, *args): + with self.query_lock: + return self.db.execute(*args) + def fetchone(self, *args): + return self.db.fetchone(*args) -def db_query(db, *args): - with db_query_lock: - db.execute(*args) + def fetchall(self, *args): + return self.db.fetchall(*args) class Repository: @@ -90,8 +98,7 @@ def __init__(self, gh, repo_label, db): self.gh = gh self.repo_label = repo_label self.db = db - db_query( - db, + db.execute( 'SELECT treeclosed, treeclosed_src FROM repos WHERE repo = ?', [repo_label] ) @@ -106,14 +113,12 @@ def __init__(self, gh, repo_label, db): def update_treeclosed(self, value, src): self.treeclosed = value self.treeclosed_src = src - db_query( - self.db, + self.db.execute( 'DELETE FROM repos where repo = ?', [self.repo_label] ) if value > 0: - db_query( - self.db, + self.db.execute( ''' INSERT INTO repos (repo, treeclosed, treeclosed_src) VALUES (?, ?, ?) @@ -228,16 +233,14 @@ def set_status(self, status): self.timeout_timer.cancel() self.timeout_timer = None - db_query( - self.db, + self.db.execute( 'UPDATE pull SET status = ? WHERE repo = ? AND num = ?', [self.status, self.repo_label, self.num] ) # FIXME: self.try_ should also be saved in the database if not self.try_: - db_query( - self.db, + self.db.execute( 'UPDATE pull SET merge_sha = ? WHERE repo = ? AND num = ?', [self.merge_sha, self.repo_label, self.num] ) @@ -252,8 +255,7 @@ def set_mergeable(self, mergeable, *, cause=None, que=True): if mergeable is not None: self.mergeable = mergeable - db_query( - self.db, + self.db.execute( 'INSERT OR REPLACE INTO mergeable (repo, num, mergeable) VALUES (?, ?, ?)', # noqa [self.repo_label, self.num, self.mergeable] ) @@ -263,8 +265,7 @@ def set_mergeable(self, mergeable, *, cause=None, que=True): else: self.mergeable = None - db_query( - self.db, + self.db.execute( 'DELETE FROM mergeable WHERE repo = ? AND num = ?', [self.repo_label, self.num] ) @@ -276,8 +277,7 @@ def init_build_res(self, builders, *, use_db=True): } for x in builders} if use_db: - db_query( - self.db, + self.db.execute( 'DELETE FROM build_res WHERE repo = ? AND num = ?', [self.repo_label, self.num] ) @@ -291,8 +291,7 @@ def set_build_res(self, builder, res, url): 'url': url, } - db_query( - self.db, + self.db.execute( 'INSERT OR REPLACE INTO build_res (repo, num, builder, res, url, merge_sha) VALUES (?, ?, ?, ?, ?, ?)', # noqa [ self.repo_label, @@ -318,8 +317,7 @@ def get_repo(self): return repo def save(self): - db_query( - self.db, + self.db.execute( 'INSERT OR REPLACE INTO pull (repo, num, status, merge_sha, title, body, head_sha, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', # noqa [ self.repo_label, @@ -401,13 +399,11 @@ def timed_out(self): def record_retry_log(self, src, body): # destroy ancient records - db_query( - self.db, + self.db.execute( "DELETE FROM retry_log WHERE repo = ? AND time < date('now', ?)", [self.repo_label, global_cfg.get('retry_log_expire', '-42 days')], ) - db_query( - self.db, + self.db.execute( 'INSERT INTO retry_log (repo, num, src, msg) VALUES (?, ?, ?, ?)', [self.repo_label, self.num, src, body], ) @@ -1474,9 +1470,9 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q repo = gh.repository(repo_cfg['owner'], repo_cfg['name']) - db_query(db, 'DELETE FROM pull WHERE repo = ?', [repo_label]) - db_query(db, 'DELETE FROM build_res WHERE repo = ?', [repo_label]) - db_query(db, 'DELETE FROM mergeable WHERE repo = ?', [repo_label]) + db.execute('DELETE FROM pull WHERE repo = ?', [repo_label]) + db.execute('DELETE FROM build_res WHERE repo = ?', [repo_label]) + db.execute('DELETE FROM mergeable WHERE repo = ?', [repo_label]) saved_states = {} for num, state in states[repo_label].items(): @@ -1489,8 +1485,7 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q repos[repo_label] = Repository(repo, repo_label, db) for pull in repo.iter_pulls(state='open'): - db_query( - db, + db.execute( 'SELECT status FROM pull WHERE repo = ? AND num = ?', [repo_label, pull.number]) row = db.fetchone() @@ -1626,9 +1621,10 @@ def main(): db_conn = sqlite3.connect(db_file, check_same_thread=False, isolation_level=None) - db = db_conn.cursor() + inner_db = db_conn.cursor() + db = LockingDatabase(inner_db) - db_query(db, '''CREATE TABLE IF NOT EXISTS pull ( + db.execute('''CREATE TABLE IF NOT EXISTS pull ( repo TEXT NOT NULL, num INTEGER NOT NULL, status TEXT NOT NULL, @@ -1647,7 +1643,7 @@ def main(): UNIQUE (repo, num) )''') - db_query(db, '''CREATE TABLE IF NOT EXISTS build_res ( + db.execute('''CREATE TABLE IF NOT EXISTS build_res ( repo TEXT NOT NULL, num INTEGER NOT NULL, builder TEXT NOT NULL, @@ -1657,36 +1653,36 @@ def main(): UNIQUE (repo, num, builder) )''') - db_query(db, '''CREATE TABLE IF NOT EXISTS mergeable ( + db.execute('''CREATE TABLE IF NOT EXISTS mergeable ( repo TEXT NOT NULL, num INTEGER NOT NULL, mergeable INTEGER NOT NULL, UNIQUE (repo, num) )''') - db_query(db, '''CREATE TABLE IF NOT EXISTS repos ( + db.execute('''CREATE TABLE IF NOT EXISTS repos ( repo TEXT NOT NULL, treeclosed INTEGER NOT NULL, treeclosed_src TEXT, UNIQUE (repo) )''') - db_query(db, '''CREATE TABLE IF NOT EXISTS retry_log ( + db.execute('''CREATE TABLE IF NOT EXISTS retry_log ( repo TEXT NOT NULL, num INTEGER NOT NULL, time DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, src TEXT NOT NULL, msg TEXT NOT NULL )''') - db_query(db, ''' + db.execute(''' CREATE INDEX IF NOT EXISTS retry_log_time_index ON retry_log (repo, time DESC) ''') # manual DB migration :/ try: - db_query(db, 'SELECT treeclosed_src FROM repos LIMIT 0') + db.execute('SELECT treeclosed_src FROM repos LIMIT 0') except sqlite3.OperationalError: - db_query(db, 'ALTER TABLE repos ADD COLUMN treeclosed_src TEXT') + db.execute('ALTER TABLE repos ADD COLUMN treeclosed_src TEXT') for repo_label, repo_cfg in cfg['repo'].items(): repo_cfgs[repo_label] = repo_cfg @@ -1695,8 +1691,7 @@ def main(): repo_states = {} repos[repo_label] = Repository(None, repo_label, db) - db_query( - db, + db.execute( 'SELECT num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha FROM pull WHERE repo = ?', # noqa [repo_label]) for num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha in db.fetchall(): # noqa @@ -1738,8 +1733,7 @@ def main(): states[repo_label] = repo_states - db_query( - db, + db.execute( 'SELECT repo, num, builder, res, url, merge_sha FROM build_res') for repo_label, num, builder, res, url, merge_sha in db.fetchall(): try: @@ -1749,8 +1743,7 @@ def main(): if state.merge_sha != merge_sha: raise KeyError except KeyError: - db_query( - db, + db.execute( 'DELETE FROM build_res WHERE repo = ? AND num = ? AND builder = ?', # noqa [repo_label, num, builder]) continue @@ -1760,23 +1753,22 @@ def main(): 'url': url, } - db_query(db, 'SELECT repo, num, mergeable FROM mergeable') + db.execute('SELECT repo, num, mergeable FROM mergeable') for repo_label, num, mergeable in db.fetchall(): try: state = states[repo_label][num] except KeyError: - db_query( - db, + db.execute( 'DELETE FROM mergeable WHERE repo = ? AND num = ?', [repo_label, num]) continue state.mergeable = bool(mergeable) if mergeable is not None else None - db_query(db, 'SELECT repo FROM pull GROUP BY repo') + db.execute('SELECT repo FROM pull GROUP BY repo') for repo_label, in db.fetchall(): if repo_label not in repos: - db_query(db, 'DELETE FROM pull WHERE repo = ?', [repo_label]) + db.execute('DELETE FROM pull WHERE repo = ?', [repo_label]) queue_handler_lock = Lock() diff --git a/homu/server.py b/homu/server.py index aad8bec..21f10ef 100644 --- a/homu/server.py +++ b/homu/server.py @@ -4,7 +4,6 @@ from .main import ( PullReqState, parse_commands, - db_query, INTERRUPTED_BY_HOMU_RE, synchronize, LabelEvent, @@ -198,8 +197,7 @@ def retry_log(repo_label): g.cfg['repo'][repo_label]['name'], ) - db_query( - g.db, + g.db.execute( ''' SELECT num, time, src, msg FROM retry_log WHERE repo = ? ORDER BY time DESC @@ -483,12 +481,12 @@ def fail(err): del g.states[repo_label][pull_num] - db_query(g.db, 'DELETE FROM pull WHERE repo = ? AND num = ?', - [repo_label, pull_num]) - db_query(g.db, 'DELETE FROM build_res WHERE repo = ? AND num = ?', - [repo_label, pull_num]) - db_query(g.db, 'DELETE FROM mergeable WHERE repo = ? AND num = ?', - [repo_label, pull_num]) + g.db.execute('DELETE FROM pull WHERE repo = ? AND num = ?', + [repo_label, pull_num]) + g.db.execute('DELETE FROM build_res WHERE repo = ? AND num = ?', + [repo_label, pull_num]) + g.db.execute('DELETE FROM mergeable WHERE repo = ? AND num = ?', + [repo_label, pull_num]) g.queue_handler() @@ -905,9 +903,9 @@ def admin(): repo_label = request.json['repo_label'] repo_cfg = g.repo_cfgs[repo_label] - db_query(g.db, 'DELETE FROM pull WHERE repo = ?', [repo_label]) - db_query(g.db, 'DELETE FROM build_res WHERE repo = ?', [repo_label]) - db_query(g.db, 'DELETE FROM mergeable WHERE repo = ?', [repo_label]) + g.db.execute('DELETE FROM pull WHERE repo = ?', [repo_label]) + g.db.execute('DELETE FROM build_res WHERE repo = ?', [repo_label]) + g.db.execute('DELETE FROM mergeable WHERE repo = ?', [repo_label]) del g.states[repo_label] del g.repos[repo_label] From 47e01174c9f02c77466a1c5dd1ca8a2732cabaa5 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 31 May 2019 07:06:40 -0500 Subject: [PATCH 03/17] Pull PullReqState into its own file Extract PullReqState into its own file for code readability. Because PullReqState shares some constants with main and server, extract those constants into a `consts.py` file as well. --- homu/consts.py | 38 ++++ homu/main.py | 334 +----------------------------------- homu/parse_issue_comment.py | 11 +- homu/pull_req_state.py | 296 ++++++++++++++++++++++++++++++++ homu/server.py | 6 +- 5 files changed, 348 insertions(+), 337 deletions(-) create mode 100644 homu/consts.py create mode 100644 homu/pull_req_state.py diff --git a/homu/consts.py b/homu/consts.py new file mode 100644 index 0000000..6ca6fa4 --- /dev/null +++ b/homu/consts.py @@ -0,0 +1,38 @@ +import re +from enum import Enum + +STATUS_TO_PRIORITY = { + 'success': 0, + 'pending': 1, + 'approved': 2, + '': 3, + 'error': 4, + 'failure': 5, +} + +INTERRUPTED_BY_HOMU_FMT = 'Interrupted by Homu ({})' +INTERRUPTED_BY_HOMU_RE = re.compile(r'Interrupted by Homu \((.+?)\)') +DEFAULT_TEST_TIMEOUT = 3600 * 10 + +WORDS_TO_ROLLUP = { + 'rollup-': 0, + 'rollup': 1, + 'rollup=maybe': 0, + 'rollup=never': -1, + 'rollup=always': 1, +} + + +class LabelEvent(Enum): + APPROVED = 'approved' + REJECTED = 'rejected' + CONFLICT = 'conflict' + SUCCEED = 'succeed' + FAILED = 'failed' + TRY = 'try' + TRY_SUCCEED = 'try_succeed' + TRY_FAILED = 'try_failed' + EXEMPTED = 'exempted' + TIMED_OUT = 'timed_out' + INTERRUPTED = 'interrupted' + PUSHED = 'pushed' diff --git a/homu/main.py b/homu/main.py index 51d99e3..abc74a4 100644 --- a/homu/main.py +++ b/homu/main.py @@ -13,8 +13,13 @@ AuthState, ) from .utils import lazy_debug +from .consts import ( + INTERRUPTED_BY_HOMU_FMT, + DEFAULT_TEST_TIMEOUT, + LabelEvent, +) import logging -from threading import Thread, Lock, Timer +from threading import Thread, Lock import time import traceback import sqlite3 @@ -23,36 +28,14 @@ from queue import Queue import os import sys -from enum import Enum import subprocess from .git_helper import SSH_KEY_FILE import shlex import random -import weakref - -STATUS_TO_PRIORITY = { - 'success': 0, - 'pending': 1, - 'approved': 2, - '': 3, - 'error': 4, - 'failure': 5, -} - -INTERRUPTED_BY_HOMU_FMT = 'Interrupted by Homu ({})' -INTERRUPTED_BY_HOMU_RE = re.compile(r'Interrupted by Homu \((.+?)\)') -DEFAULT_TEST_TIMEOUT = 3600 * 10 +from .pull_req_state import PullReqState global_cfg = {} -WORDS_TO_ROLLUP = { - 'rollup-': 0, - 'rollup': 1, - 'rollup=maybe': 0, - 'rollup=never': -1, - 'rollup=always': 1, -} - @contextmanager def buildbot_sess(repo_cfg): @@ -130,292 +113,6 @@ def __lt__(self, other): return self.gh < other.gh -class PullReqState: - num = 0 - priority = 0 - rollup = 0 - title = '' - body = '' - head_ref = '' - base_ref = '' - assignee = '' - delegate = '' - - def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, - gh, owner, name, label_events, repos): - self.head_advanced('', use_db=False) - - self.num = num - self.head_sha = head_sha - self.status = status - self.db = db - self.repo_label = repo_label - self.mergeable_que = mergeable_que - self.gh = gh - self.owner = owner - self.name = name - self.repos = repos - self.timeout_timer = None - self.test_started = time.time() - self.label_events = label_events - - def head_advanced(self, head_sha, *, use_db=True): - self.head_sha = head_sha - self.approved_by = '' - self.status = '' - self.merge_sha = '' - self.build_res = {} - self.try_ = False - self.mergeable = None - - if use_db: - self.set_status('') - self.set_mergeable(None) - self.init_build_res([]) - - def __repr__(self): - fmt = 'PullReqState:{}/{}#{}(approved_by={}, priority={}, status={})' - return fmt.format( - self.owner, - self.name, - self.num, - self.approved_by, - self.priority, - self.status, - ) - - def sort_key(self): - return [ - STATUS_TO_PRIORITY.get(self.get_status(), -1), - 1 if self.mergeable is False else 0, - 0 if self.approved_by else 1, - # Sort rollup=always to the bottom of the queue, but treat all - # other rollup statuses as equivalent - 1 if WORDS_TO_ROLLUP['rollup=always'] == self.rollup else 0, - -self.priority, - self.num, - ] - - def __lt__(self, other): - return self.sort_key() < other.sort_key() - - def get_issue(self): - issue = getattr(self, 'issue', None) - if not issue: - issue = self.issue = self.get_repo().issue(self.num) - return issue - - def add_comment(self, comment): - if isinstance(comment, comments.Comment): - comment = "%s\n" % ( - comment.render(), comment.jsonify(), - ) - self.get_issue().create_comment(comment) - - def change_labels(self, event): - event = self.label_events.get(event.value, {}) - removes = event.get('remove', []) - adds = event.get('add', []) - unless = event.get('unless', []) - if not removes and not adds: - return - - issue = self.get_issue() - labels = {label.name for label in issue.iter_labels()} - if labels.isdisjoint(unless): - labels.difference_update(removes) - labels.update(adds) - issue.replace_labels(list(labels)) - - def set_status(self, status): - self.status = status - if self.timeout_timer: - self.timeout_timer.cancel() - self.timeout_timer = None - - self.db.execute( - 'UPDATE pull SET status = ? WHERE repo = ? AND num = ?', - [self.status, self.repo_label, self.num] - ) - - # FIXME: self.try_ should also be saved in the database - if not self.try_: - self.db.execute( - 'UPDATE pull SET merge_sha = ? WHERE repo = ? AND num = ?', - [self.merge_sha, self.repo_label, self.num] - ) - - def get_status(self): - if self.status == '' and self.approved_by: - if self.mergeable is not False: - return 'approved' - return self.status - - def set_mergeable(self, mergeable, *, cause=None, que=True): - if mergeable is not None: - self.mergeable = mergeable - - self.db.execute( - 'INSERT OR REPLACE INTO mergeable (repo, num, mergeable) VALUES (?, ?, ?)', # noqa - [self.repo_label, self.num, self.mergeable] - ) - else: - if que: - self.mergeable_que.put([self, cause]) - else: - self.mergeable = None - - self.db.execute( - 'DELETE FROM mergeable WHERE repo = ? AND num = ?', - [self.repo_label, self.num] - ) - - def init_build_res(self, builders, *, use_db=True): - self.build_res = {x: { - 'res': None, - 'url': '', - } for x in builders} - - if use_db: - self.db.execute( - 'DELETE FROM build_res WHERE repo = ? AND num = ?', - [self.repo_label, self.num] - ) - - def set_build_res(self, builder, res, url): - if builder not in self.build_res: - raise Exception('Invalid builder: {}'.format(builder)) - - self.build_res[builder] = { - 'res': res, - 'url': url, - } - - self.db.execute( - 'INSERT OR REPLACE INTO build_res (repo, num, builder, res, url, merge_sha) VALUES (?, ?, ?, ?, ?, ?)', # noqa - [ - self.repo_label, - self.num, - builder, - res, - url, - self.merge_sha, - ]) - - def build_res_summary(self): - return ', '.join('{}: {}'.format(builder, data['res']) - for builder, data in self.build_res.items()) - - def get_repo(self): - repo = self.repos[self.repo_label].gh - if not repo: - repo = self.gh.repository(self.owner, self.name) - self.repos[self.repo_label].gh = repo - - assert repo.owner.login == self.owner - assert repo.name == self.name - return repo - - def save(self): - self.db.execute( - 'INSERT OR REPLACE INTO pull (repo, num, status, merge_sha, title, body, head_sha, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', # noqa - [ - self.repo_label, - self.num, - self.status, - self.merge_sha, - self.title, - self.body, - self.head_sha, - self.head_ref, - self.base_ref, - self.assignee, - self.approved_by, - self.priority, - self.try_, - self.rollup, - self.delegate, - ]) - - def refresh(self): - issue = self.get_repo().issue(self.num) - - self.title = issue.title - self.body = issue.body - - def fake_merge(self, repo_cfg): - if not repo_cfg.get('linear', False): - return - if repo_cfg.get('autosquash', False): - return - - issue = self.get_issue() - title = issue.title - # We tell github to close the PR via the commit message, but it - # doesn't know that constitutes a merge. Edit the title so that it's - # clearer. - merged_prefix = '[merged] ' - if not title.startswith(merged_prefix): - title = merged_prefix + title - issue.edit(title=title) - - def change_treeclosed(self, value, src): - self.repos[self.repo_label].update_treeclosed(value, src) - - def blocked_by_closed_tree(self): - treeclosed = self.repos[self.repo_label].treeclosed - return treeclosed if self.priority < treeclosed else None - - def start_testing(self, timeout): - self.test_started = time.time() # FIXME: Save in the local database - self.set_status('pending') - - wm = weakref.WeakMethod(self.timed_out) - - def timed_out(): - m = wm() - if m: - m() - timer = Timer(timeout, timed_out) - timer.start() - self.timeout_timer = timer - - def timed_out(self): - print('* Test timed out: {}'.format(self)) - - self.merge_sha = '' - self.save() - self.set_status('failure') - - utils.github_create_status( - self.get_repo(), - self.head_sha, - 'failure', - '', - 'Test timed out', - context='homu') - self.add_comment(comments.TimedOut()) - self.change_labels(LabelEvent.TIMED_OUT) - - def record_retry_log(self, src, body): - # destroy ancient records - self.db.execute( - "DELETE FROM retry_log WHERE repo = ? AND time < date('now', ?)", - [self.repo_label, global_cfg.get('retry_log_expire', '-42 days')], - ) - self.db.execute( - 'INSERT INTO retry_log (repo, num, src, msg) VALUES (?, ?, ?, ?)', - [self.repo_label, self.num, src, body], - ) - - @property - def author(self): - """ - Get the GitHub login name of the author of the pull request - """ - return self.get_issue().user.login - - def sha_cmp(short, full): return len(short) >= 4 and short == full[:len(short)] @@ -424,21 +121,6 @@ def sha_or_blank(sha): return sha if re.match(r'^[0-9a-f]+$', sha) else '' -class LabelEvent(Enum): - APPROVED = 'approved' - REJECTED = 'rejected' - CONFLICT = 'conflict' - SUCCEED = 'succeed' - FAILED = 'failed' - TRY = 'try' - TRY_SUCCEED = 'try_succeed' - TRY_FAILED = 'try_failed' - EXEMPTED = 'exempted' - TIMED_OUT = 'timed_out' - INTERRUPTED = 'interrupted' - PUSHED = 'pushed' - - PORTAL_TURRET_DIALOG = ["Target acquired", "Activated", "There you are"] PORTAL_TURRET_IMAGE = "https://cloud.githubusercontent.com/assets/1617736/22222924/c07b2a1c-e16d-11e6-91b3-ac659550585c.png" # noqa @@ -638,7 +320,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, event = LabelEvent.TRY else: event = LabelEvent.APPROVED - state.record_retry_log(command_src, body) + state.record_retry_log(command_src, body, global_cfg) state.change_labels(event) elif command.action in ['try', 'untry'] and realtime: diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index 8d53bc0..77fbfc5 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -1,6 +1,8 @@ from itertools import chain import re +from .consts import WORDS_TO_ROLLUP + class IssueCommentCommand: """ @@ -93,15 +95,6 @@ def hook(cls, hook_name, hook_extra=None): return command -WORDS_TO_ROLLUP = { - 'rollup-': 0, - 'rollup': 1, - 'rollup=maybe': 0, - 'rollup=never': -1, - 'rollup=always': 1, -} - - def is_sha(sha): """ Try to determine if the input is a git sha diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py new file mode 100644 index 0000000..8e13806 --- /dev/null +++ b/homu/pull_req_state.py @@ -0,0 +1,296 @@ +import weakref +from threading import Timer +import time +from . import utils +from . import comments +from .consts import ( + STATUS_TO_PRIORITY, + WORDS_TO_ROLLUP, + LabelEvent, +) + + +class PullReqState: + num = 0 + priority = 0 + rollup = 0 + title = '' + body = '' + head_ref = '' + base_ref = '' + assignee = '' + delegate = '' + + def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, + gh, owner, name, label_events, repos): + self.head_advanced('', use_db=False) + + self.num = num + self.head_sha = head_sha + self.status = status + self.db = db + self.repo_label = repo_label + self.mergeable_que = mergeable_que + self.gh = gh + self.owner = owner + self.name = name + self.repos = repos + self.timeout_timer = None + self.test_started = time.time() + self.label_events = label_events + + def head_advanced(self, head_sha, *, use_db=True): + self.head_sha = head_sha + self.approved_by = '' + self.status = '' + self.merge_sha = '' + self.build_res = {} + self.try_ = False + self.mergeable = None + + if use_db: + self.set_status('') + self.set_mergeable(None) + self.init_build_res([]) + + def __repr__(self): + fmt = 'PullReqState:{}/{}#{}(approved_by={}, priority={}, status={})' + return fmt.format( + self.owner, + self.name, + self.num, + self.approved_by, + self.priority, + self.status, + ) + + def sort_key(self): + return [ + STATUS_TO_PRIORITY.get(self.get_status(), -1), + 1 if self.mergeable is False else 0, + 0 if self.approved_by else 1, + # Sort rollup=always to the bottom of the queue, but treat all + # other rollup statuses as equivalent + 1 if WORDS_TO_ROLLUP['rollup=always'] == self.rollup else 0, + -self.priority, + self.num, + ] + + def __lt__(self, other): + return self.sort_key() < other.sort_key() + + def get_issue(self): + issue = getattr(self, 'issue', None) + if not issue: + issue = self.issue = self.get_repo().issue(self.num) + return issue + + def add_comment(self, comment): + if isinstance(comment, comments.Comment): + comment = "%s\n" % ( + comment.render(), comment.jsonify(), + ) + self.get_issue().create_comment(comment) + + def change_labels(self, event): + event = self.label_events.get(event.value, {}) + removes = event.get('remove', []) + adds = event.get('add', []) + unless = event.get('unless', []) + if not removes and not adds: + return + + issue = self.get_issue() + labels = {label.name for label in issue.iter_labels()} + if labels.isdisjoint(unless): + labels.difference_update(removes) + labels.update(adds) + issue.replace_labels(list(labels)) + + def set_status(self, status): + self.status = status + if self.timeout_timer: + self.timeout_timer.cancel() + self.timeout_timer = None + + self.db.execute( + 'UPDATE pull SET status = ? WHERE repo = ? AND num = ?', + [self.status, self.repo_label, self.num] + ) + + # FIXME: self.try_ should also be saved in the database + if not self.try_: + self.db.execute( + 'UPDATE pull SET merge_sha = ? WHERE repo = ? AND num = ?', + [self.merge_sha, self.repo_label, self.num] + ) + + def get_status(self): + if self.status == '' and self.approved_by: + if self.mergeable is not False: + return 'approved' + return self.status + + def set_mergeable(self, mergeable, *, cause=None, que=True): + if mergeable is not None: + self.mergeable = mergeable + + self.db.execute( + 'INSERT OR REPLACE INTO mergeable (repo, num, mergeable) VALUES (?, ?, ?)', # noqa + [self.repo_label, self.num, self.mergeable] + ) + else: + if que: + self.mergeable_que.put([self, cause]) + else: + self.mergeable = None + + self.db.execute( + 'DELETE FROM mergeable WHERE repo = ? AND num = ?', + [self.repo_label, self.num] + ) + + def init_build_res(self, builders, *, use_db=True): + self.build_res = {x: { + 'res': None, + 'url': '', + } for x in builders} + + if use_db: + self.db.execute( + 'DELETE FROM build_res WHERE repo = ? AND num = ?', + [self.repo_label, self.num] + ) + + def set_build_res(self, builder, res, url): + if builder not in self.build_res: + raise Exception('Invalid builder: {}'.format(builder)) + + self.build_res[builder] = { + 'res': res, + 'url': url, + } + + self.db.execute( + 'INSERT OR REPLACE INTO build_res (repo, num, builder, res, url, merge_sha) VALUES (?, ?, ?, ?, ?, ?)', # noqa + [ + self.repo_label, + self.num, + builder, + res, + url, + self.merge_sha, + ]) + + def build_res_summary(self): + return ', '.join('{}: {}'.format(builder, data['res']) + for builder, data in self.build_res.items()) + + def get_repo(self): + repo = self.repos[self.repo_label].gh + if not repo: + repo = self.gh.repository(self.owner, self.name) + self.repos[self.repo_label].gh = repo + + assert repo.owner.login == self.owner + assert repo.name == self.name + return repo + + def save(self): + self.db.execute( + 'INSERT OR REPLACE INTO pull (repo, num, status, merge_sha, title, body, head_sha, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', # noqa + [ + self.repo_label, + self.num, + self.status, + self.merge_sha, + self.title, + self.body, + self.head_sha, + self.head_ref, + self.base_ref, + self.assignee, + self.approved_by, + self.priority, + self.try_, + self.rollup, + self.delegate, + ]) + + def refresh(self): + issue = self.get_repo().issue(self.num) + + self.title = issue.title + self.body = issue.body + + def fake_merge(self, repo_cfg): + if not repo_cfg.get('linear', False): + return + if repo_cfg.get('autosquash', False): + return + + issue = self.get_issue() + title = issue.title + # We tell github to close the PR via the commit message, but it + # doesn't know that constitutes a merge. Edit the title so that it's + # clearer. + merged_prefix = '[merged] ' + if not title.startswith(merged_prefix): + title = merged_prefix + title + issue.edit(title=title) + + def change_treeclosed(self, value, src): + self.repos[self.repo_label].update_treeclosed(value, src) + + def blocked_by_closed_tree(self): + treeclosed = self.repos[self.repo_label].treeclosed + return treeclosed if self.priority < treeclosed else None + + def start_testing(self, timeout): + self.test_started = time.time() # FIXME: Save in the local database + self.set_status('pending') + + wm = weakref.WeakMethod(self.timed_out) + + def timed_out(): + m = wm() + if m: + m() + timer = Timer(timeout, timed_out) + timer.start() + self.timeout_timer = timer + + def timed_out(self): + print('* Test timed out: {}'.format(self)) + + self.merge_sha = '' + self.save() + self.set_status('failure') + + utils.github_create_status( + self.get_repo(), + self.head_sha, + 'failure', + '', + 'Test timed out', + context='homu') + self.add_comment(comments.TimedOut()) + self.change_labels(LabelEvent.TIMED_OUT) + + def record_retry_log(self, src, body, cfg): + # destroy ancient records + self.db.execute( + "DELETE FROM retry_log WHERE repo = ? AND time < date('now', ?)", + [self.repo_label, cfg.get('retry_log_expire', '-42 days')], + ) + self.db.execute( + 'INSERT INTO retry_log (repo, num, src, msg) VALUES (?, ?, ?, ?)', + [self.repo_label, self.num, src, body], + ) + + @property + def author(self): + """ + Get the GitHub login name of the author of the pull request + """ + return self.get_issue().user.login diff --git a/homu/server.py b/homu/server.py index 21f10ef..7e53e4a 100644 --- a/homu/server.py +++ b/homu/server.py @@ -2,12 +2,14 @@ import json import urllib.parse from .main import ( - PullReqState, parse_commands, - INTERRUPTED_BY_HOMU_RE, synchronize, LabelEvent, ) +from .consts import ( + INTERRUPTED_BY_HOMU_RE, +) +from .pull_req_state import PullReqState from . import comments from . import utils from .utils import lazy_debug From 4c327b49d208f299a3cb43b3ecd4afb3b09f304e Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 7 Jun 2019 10:26:38 -0500 Subject: [PATCH 04/17] Parse homu state out of comments For some critical comments, Homu adds extra information about its state in the form of a JSON blob to the comment that isn't visible to the user but is visible in the source for the comment. For example, Homu may leave a comment like the following, where the JSON blob is not visible because of the `` markdown/html comments. :hourglass: Trying commit abcdef with merge 012345... This change parses this extra information out of the comments and makes it available to the initial synchronization algorithm. --- homu/parse_issue_comment.py | 18 ++++++++++++++++-- homu/tests/test_parse_issue_comment.py | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index 77fbfc5..81f99f3 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -1,5 +1,6 @@ from itertools import chain import re +import json from .consts import WORDS_TO_ROLLUP @@ -94,6 +95,12 @@ def hook(cls, hook_name, hook_extra=None): command.hook_extra = hook_extra return command + @classmethod + def homu_state(cls, state): + command = cls('homu-state') + command.homu_state = state + return command + def is_sha(sha): """ @@ -144,6 +151,15 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): E.g. `['hook1', 'hook2', 'hook3']` """ + commands = [] + + states = chain.from_iterable(re.findall(r'', x) + for x + in body.splitlines()) + + for state in states: + commands.append(IssueCommentCommand.homu_state(json.loads(state))) + botname_regex = re.compile(r'^.*(?=@' + botname + ')') # All of the 'words' after and including the botname @@ -153,8 +169,6 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): in body.splitlines() if '@' + botname in x)) # noqa - commands = [] - if words[1:] == ["are", "you", "still", "there?"]: commands.append(IssueCommentCommand.ping('portal')) diff --git a/homu/tests/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py index e7da8ef..451ae9e 100644 --- a/homu/tests/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -560,3 +560,26 @@ def test_ignore_commands_after_bors_line(): command = commands[0] assert command.action == 'approve' assert command.actor == 'jack' + + +def test_homu_state(): + """ + Test that when a comment has a Homu state in it, we return that state. + """ + + author = "bors" + body = """ + :hourglass: Trying commit 3d67c2da893aed40bc36b6ac9148c593aa0a868a with merge b7a0ff78ba2ba0b3f5e1a8e89464a84dc386aa81... + + """ # noqa + + commands = parse_issue_comment(author, body, commit, "bors") + + assert len(commands) == 1 + command = commands[0] + assert command.action == 'homu-state' + assert command.homu_state == { + 'type': 'TryBuildStarted', + 'head_sha': '3d67c2da893aed40bc36b6ac9148c593aa0a868a', + 'merge_sha': 'b7a0ff78ba2ba0b3f5e1a8e89464a84dc386aa81', + } From a5fe2e6c19d92c6ccb177b628b0a84d02cdb573d Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 7 Jun 2019 11:14:29 -0500 Subject: [PATCH 05/17] Structure for `process_event`; test approvals Create the general structure for `process_event` and its testing, and get a long way toward testing approval comments. --- homu/parse_issue_comment.py | 8 +- homu/pull_req_state.py | 501 +++++++++++++++++++++++++ homu/pull_request_events.py | 284 ++++++++++++++ homu/tests/test_parse_issue_comment.py | 6 + homu/tests/test_process_event.py | 397 ++++++++++++++++++++ 5 files changed, 1194 insertions(+), 2 deletions(-) create mode 100644 homu/pull_request_events.py create mode 100644 homu/tests/test_process_event.py diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index 81f99f3..7ca3c91 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -16,10 +16,12 @@ def __init__(self, action): self.action = action @classmethod - def approve(cls, approver, commit): + def approve(cls, approver, commit, commit_was_specified): command = cls('approve') command.commit = commit command.actor = approver + # Whether or not the commit was explicitely listed. + command.commit_was_specified = commit_was_specified return command @classmethod @@ -186,10 +188,12 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): if word == 'r+' or word.startswith('r='): approved_sha = sha + is_explicit = False if i + 1 < len(words) and is_sha(words[i + 1]): approved_sha = words[i + 1] words[i + 1] = None + is_explicit = True approver = word[len('r='):] if word.startswith('r=') else username @@ -198,7 +202,7 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): continue commands.append( - IssueCommentCommand.approve(approver, approved_sha)) + IssueCommentCommand.approve(approver, approved_sha, is_explicit)) elif word == 'r-': commands.append(IssueCommentCommand.unapprove()) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 8e13806..209de3d 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -1,6 +1,7 @@ import weakref from threading import Timer import time +import functools from . import utils from . import comments from .consts import ( @@ -8,6 +9,35 @@ WORDS_TO_ROLLUP, LabelEvent, ) +from .parse_issue_comment import parse_issue_comment +from .auth import ( + assert_authorized, + AuthorizationException, + AuthState, +) + + +class ProcessEventResult: + """ + The object returned from PullReqState::process_event that contains + information about what changed and what needs to happen. + """ + def __init__(self): + self.changed = False + self.comments = [] + self.label_events = [] + + def __repr__(self): + return 'ProcessEventResult'.format( + self.changed, self.comments, self.label_events) + + +def sha_cmp(short, full): + return len(short) >= 4 and short == full[:len(short)] + + +def sha_or_blank(sha): + return sha if re.match(r'^[0-9a-f]+$', sha) else '' class PullReqState: @@ -288,6 +318,477 @@ def record_retry_log(self, src, body, cfg): [self.repo_label, self.num, src, body], ) + def process_event(self, event): + """ + Process a GitHub event (in the form of either a Timeline Event from + GitHub's Timeline API or an Event from GitHub's Webhooks) and update + the state of the pull request accordingly. + + Returns an object that contains information about the change, with at + least the following properties: + + changed: bool -- Whether or not the state of the pull request was + affected by this event + + comments: [string] -- Comments that can be made on the pull request + as a result of this event. In realtime mode, these should then be + applied to the pull request. In synchronization mode, they may be + dropped. (In testing mode, they should be tested.) + + label_events: [LabelEvent] -- Any label events that need to be + applied as a result of this event. + """ + + result = ProcessEventResult() + if event.event_type == 'PullRequestCommit': + result.changed = self.head_sha != event['commit']['oid'] + self.head_sha = event['commit']['oid'] + # New commits come in: no longer approved + result.changed = result.changed or self.approved_by != '' + self.approved_by = '' + + elif event.event_type == 'HeadRefForcePushedEvent': + result.changed = self.head_sha != event['afterCommit']['oid'] + self.head_sha = event['afterCommit']['oid'] + # New commits come in: no longer approved + result.changed = result.changed or self.approved_by != '' + self.approved_by = '' + + elif event.event_type == 'IssueComment': + comments = parse_issue_comment( + username=event['author']['login'], + body=event['body'], + sha=self.head_sha, + # TODO: Don't hardcode 'bors' + botname='bors', + # TODO: Hooks! + hooks=[]) + + for comment in comments: + subresult = self.process_issue_comment(event, comment) + result.changed = result.changed or subresult.changed + result.comments.extend(subresult.comments) + result.label_events.extend(subresult.label_events) + + elif event.event_type == 'RenamedTitleEvent': + result.changed = self.title != event['currentTitle'] + self.title = event['currentTitle'] + + elif event.event_type == 'AssignedEvent': + result.changed = self.assignee != event['user']['login'] + self.assignee = event['user']['login'] + + elif event.event_type == 'PullRequestReview': + # TODO: Pull commands from review comments + pass + + elif event.event_type == 'MergedEvent': + # TODO: Something here. + #result.changed = self.github_state != 'merged' + #self.github_state = 'merged' + pass + + elif event.event_type == 'ClosedEvent': + # TODO: Something here. + #if self.github_state != 'merged': + # changed = self.github_state != 'closed' + # self.github_state = 'closed' + pass + + elif event.event_type == 'ReopenedEvent': + # TODO: Something here. + #changed = self.github_state != 'open' + #self.github_state = 'open' + pass + + elif event.event_type in [ + 'SubscribedEvent', + 'MentionedEvent', + 'LabeledEvent', + 'UnlabeledEvent', + 'ReferencedEvent', + 'CrossReferencedEvent']: + # We don't care about any of these events. + pass + + else: + # Ooops, did we miss this event type? Or is it new? + print("Unknown event type: {}".format(event.event_type)) + + return result + +# def process_issue_comment(self, event, command): +# result = ProcessEventResult() +# if command.action == 'homu-state': +# return self.process_homu_state(event, command) +# +# if command.action == 'approve': +# # TODO: Something with states +# result.changed = self.approved_by != command.actor +# self.approved_by = command.actor +# +# if command.action == 'unapprove': +# # TODO: Something with states +# result.changed = self.approved_by != '' +# self.approved_by = None +# +# # if command.action == 'try': +# # changed = True +# # self.tries.append(PullRequestTry(1, self.head_sha, None)) +# return result + + def process_issue_comment(self, event, command): + # TODO: Don't hardcode botname + botname = 'bors' + username = event['author']['login'] + _assert_reviewer_auth_verified = functools.partial( + assert_authorized, + username, + self.repo_label, + {}, # repo_cfg + self, + AuthState.REVIEWER, + botname, + ) + _assert_try_auth_verified = functools.partial( + assert_authorized, + username, + self.repo_label, + {}, # repo_cfg + self, + AuthState.TRY, + botname, + ) + result = ProcessEventResult() + try: + found = True + if command.action == 'approve': + _assert_reviewer_auth_verified() + + 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 self.title.upper().startswith(wip_kw): + result.comments.append(comments.ApprovalIgnoredWip( + sha=self.head_sha, + wip_keyword=wip_kw, + )) + is_wip = True + break + if is_wip: + return result + +# # 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 self.head_sha): +# result.commens.append( +# ':bangbang: Invalid head SHA found, retrying: `{}`' +# .format(self.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 self.approved_by and username != botname: + lines = [] + + if self.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 lines: + lines.insert(0, '') + lines.insert(0, ':bulb: This pull request was already approved, no need to approve it again.') # noqa + + result.comments.append('\n'.join(lines)) + + elif not sha_cmp(cur_sha, self.head_sha): + if username != botname: + msg = '`{}` is not a valid commit SHA.'.format(cur_sha) + result.comments.append( + ':scream_cat: {} Please try again with `{}`.' + .format(msg, self.head_sha) + ) + else: + # Somehow, the bot specified an invalid sha? + pass + + else: + self.approved_by = approver + self.try_ = False + self.status = '' + result.changed = True + result.label_events.append(LabelEvent.APPROVED) + + if username != botname: + result.comments.append(comments.Approved( + sha=self.head_sha, + approver=approver, + bot=botname, + )) + treeclosed = self.blocked_by_closed_tree() + if treeclosed: + result.comments.append( + ':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) + ) + +# 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 +# # unapprove the PR (change the target branch, push more +# # commits, etc.) so allowing them to directly unapprove it is +# # also allowed. +# if state.author != username: +# assert_authorized(username, repo_label, repo_cfg, state, +# AuthState.REVIEWER, my_username) +# +# state.approved_by = '' +# state.save() +# if realtime: +# state.change_labels(LabelEvent.REJECTED) +# +# elif command.action == 'prioritize': +# assert_authorized(username, repo_label, repo_cfg, state, +# AuthState.TRY, my_username) +# +# pvalue = command.priority +# +# if pvalue > global_cfg['max_priority']: +# if realtime: +# state.add_comment( +# ':stop_sign: Priority higher than {} is ignored.' +# .format(global_cfg['max_priority']) +# ) +# #continue +# state.priority = pvalue +# state.save() +# +# elif command.action == 'delegate': +# assert_authorized(username, repo_label, repo_cfg, state, +# AuthState.REVIEWER, my_username) +# +# state.delegate = command.delegate_to +# state.save() +# +# if realtime: +# state.add_comment(comments.Delegated( +# delegator=username, +# delegate=state.delegate +# )) +# +# elif command.action == 'undelegate': +# # TODO: why is this a TRY? +# _assert_try_auth_verified() +# +# state.delegate = '' +# state.save() +# +# elif command.action == 'delegate-author': +# _assert_reviewer_auth_verified() +# +# state.delegate = state.get_repo().pull_request(state.num).user.login # noqa +# state.save() +# +# if realtime: +# state.add_comment(comments.Delegated( +# delegator=username, +# delegate=state.delegate +# )) +# +# elif command.action == 'retry' and realtime: +# _assert_try_auth_verified() +# +# state.set_status('') +# if realtime: +# if state.try_: +# event = LabelEvent.TRY +# else: +# event = LabelEvent.APPROVED +# state.record_retry_log(command_src, body, global_cfg) +# state.change_labels(event) +# +# elif command.action in ['try', 'untry'] and realtime: +# _assert_try_auth_verified() +# +# if state.status == '' and state.approved_by: +# state.add_comment( +# ':no_good: ' +# 'Please do not `try` after a pull request has' +# ' been `r+`ed.' +# ' If you need to `try`, unapprove (`r-`) it first.' +# ) +# #continue +# +# state.try_ = command.action == 'try' +# +# state.merge_sha = '' +# state.init_build_res([]) +# +# state.save() +# if realtime and state.try_: +# # If we've tried before, the status will be 'success', and +# # this new try will not be picked up. Set the status back +# # to '' so the try will be run again. +# state.set_status('') +# # `try-` just resets the `try` bit and doesn't correspond +# # to any meaningful labeling events. +# state.change_labels(LabelEvent.TRY) +# +# elif command.action == 'rollup': +# _assert_try_auth_verified() +# +# state.rollup = command.rollup_value +# +# state.save() +# +# elif command.action == 'force' and realtime: +# _assert_try_auth_verified() +# +# if 'buildbot' in repo_cfg: +# with buildbot_sess(repo_cfg) as sess: +# res = sess.post( +# repo_cfg['buildbot']['url'] + '/builders/_selected/stopselected', # noqa +# allow_redirects=False, +# data={ +# 'selected': repo_cfg['buildbot']['builders'], +# 'comments': INTERRUPTED_BY_HOMU_FMT.format(int(time.time())), # noqa +# }) +# +# if 'authzfail' in res.text: +# err = 'Authorization failed' +# else: +# mat = re.search('(?s)
(.*?)
', res.text) # noqa +# if mat: +# err = mat.group(1).strip() +# if not err: +# err = 'Unknown error' +# else: +# err = '' +# +# if err: +# state.add_comment( +# ':bomb: Buildbot returned an error: `{}`'.format(err) +# ) +# +# elif command.action == 'clean' and realtime: +# _assert_try_auth_verified() +# +# state.merge_sha = '' +# state.init_build_res([]) +# +# state.save() +# +# elif command.action == 'ping' and realtime: +# if command.ping_type == 'portal': +# state.add_comment( +# ":cake: {}\n\n![]({})".format( +# random.choice(PORTAL_TURRET_DIALOG), +# PORTAL_TURRET_IMAGE) +# ) +# else: +# state.add_comment(":sleepy: I'm awake I'm awake") +# +# elif command.action == 'treeclosed': +# _assert_reviewer_auth_verified() +# +# state.change_treeclosed(command.treeclosed_value, command_src) +# state.save() +# +# elif command.action == 'untreeclosed': +# _assert_reviewer_auth_verified() +# +# state.change_treeclosed(-1, None) +# state.save() +# +# elif command.action == 'hook': +# hook = command.hook_name +# hook_cfg = global_cfg['hooks'][hook] +# if hook_cfg['realtime'] and not realtime: +# #continue +# pass +# if hook_cfg['access'] == "reviewer": +# _assert_reviewer_auth_verified() +# else: +# _assert_try_auth_verified() +# +# Thread( +# target=handle_hook_response, +# args=[state, hook_cfg, body, command.hook_extra] +# ).start() + + else: + found = False + + if found: + state_changed = True + + except AuthorizationException as e: + result.comments.append(e.comment) + + return result + + def process_homu_state(self, event, command): + result = ProcessEventResult() + state = command.homu_state + + if state['type'] == 'Approved': + # TODO: Something with states + result.changed = self.approval_state != 'approved' + result.changed = result.changed or self.approver != state['approver'] + self.approval_state = 'approved' + self.approved_by = state['approver'] + + elif state['type'] == 'BuildStarted': + # TODO: Something with states + result.changed = True + self.build_state = 'pending' + + elif state['type'] == 'BuildCompleted': + # TODO: Something with states + result.changed = True + self.build_state = 'completed' + + elif state['type'] == 'BuildFailed': + # TODO: Something with states + result.changed = True + self.build_state = 'failure' + + elif state['type'] == 'TryBuildStarted': + # TODO: Multiple tries? + result.changed = True + self.tries.append(PullRequestTry( + len(self.tries) + 1, + state['head_sha'], + state['merge_sha'], + event['publishedAt']) + ) + + elif state['type'] == 'TryBuildCompleted': + item = next((try_ + for try_ in self.tries + if try_.state == 'pending' + and try_.merge_sha == state['merge_sha']), + None) + + if item: + result.changed = True + # TODO: Multiple tries? + item.ended_at = event['publishedAt'] + item.state = 'completed' + item.builders = state['builders'] + + return result + @property def author(self): """ diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py new file mode 100644 index 0000000..489c49c --- /dev/null +++ b/homu/pull_request_events.py @@ -0,0 +1,284 @@ +import requests +import time + + +QUERY = """ +query ($repoName: String!, $repoOwner: String!, $pull: Int!, $after: String) { + repository(name: $repoName, owner: $repoOwner) { + pullRequest(number: $pull) { + author { + login + } + title + state + headRefOid + mergeable + timelineItems(first: 100, after: $after) { + pageInfo { + hasNextPage + endCursor + } + nodes { + eventType: __typename + ... on PullRequestCommit { + commit { + oid + } + } + ... on AssignedEvent { + actor { + login + } + user { + login + } + } + ... on UnassignedEvent { + actor { + login + } + user { + login + } + } + ... on IssueComment { + author { + login + } + body + publishedAt + } + ... on SubscribedEvent { + actor { + login + } + } + ... on LabeledEvent { + actor { + login + } + label { + name + } + } + ... on UnlabeledEvent { + actor { + login + } + label { + name + } + } + ... on BaseRefChangedEvent { + actor { + login + } + } + ... on HeadRefForcePushedEvent { + actor { + login + } + beforeCommit { + oid + } + afterCommit { + oid + } + } + ... on RenamedTitleEvent { + actor { + login + } + previousTitle + currentTitle + } + ... on MentionedEvent { + actor { + login + } + } + } + } + } + } +} +""" + + +class PullRequestResponse: + def __init__(self): + self.events = [] + + @property + def initial_title(self): + if not hasattr(self, '_initial_title'): + for event in self.events: + if event.event_type == 'RenamedTitleEvent': + self._initial_title = event.data['previousTitle'] + break + + # The title never changed. That means that the initial title is + # the same as the current title. + if not hasattr(self, '_initial_title'): + self._initial_title = self.title + + return self._initial_title + + +class PullRequestEvent: + def __init__(self, data): + self.data = data + + def __getitem__(self, key): + return self.data[key] + + @property + def event_type(self): + return self.data['eventType'] + + @staticmethod + def _actor(s): + return "\x1b[1m@" + s + "\x1b[0m" + + @staticmethod + def _label(s): + return "\x1b[100m" + s + "\x1b[0m" + + @staticmethod + def _commit(s): + return "\x1b[93m" + s[0:7] + "\x1b[0m" + + @staticmethod + def _comment_summary(comment): + # line_1 = comment.splitlines()[0] + # if len(line_1) > 40: + # return line_1[0:37] + '...' + # else: + # return line_1 + return '\n'.join([' \x1b[90m> \x1b[37m' + c + '\x1b[0m' + for c + in comment.splitlines()]) + + def format(self): + d = { + 'IssueComment': lambda e: + "{} left a comment:\n{}".format( + self._actor(e['author']['login']), + self._comment_summary(e['body'])), + 'SubscribedEvent': lambda e: + # "{} was subscribed".format( + # self._actor(e['actor']['login'])), + None, + 'MentionedEvent': lambda e: + # "{} was mentioned".format( + # self._actor(e['actor']['login'])), + None, + 'RenamedTitleEvent': lambda e: + "Renamed from '{}' to '{}' by {}".format( + e['previousTitle'], + e['currentTitle'], + self._actor(e['actor']['login'])), + 'LabeledEvent': lambda e: + "Label {} added by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'UnlabeledEvent': lambda e: + "Label {} removed by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'ReferencedEvent': lambda e: + # "Referenced", + None, + 'HeadRefForcePushedEvent': lambda e: + "{} force-pushed from {} to {}".format( + self._actor(e['actor']['login']), + self._commit(e['beforeCommit']['oid']), + self._commit(e['afterCommit']['oid'])), + 'AssignedEvent': lambda e: + "Assigned to {} by {}".format( + self._actor(e['user']['login']), + self._actor(e['actor']['login'])), + 'CrossReferencedEvent': lambda e: + # "Cross referenced", + None, + 'PullRequestReview': lambda e: + "Reviewed", + 'PullRequestCommit': lambda e: + "New commit {} pushed".format( + self._commit(self.data['commit']['oid'])), + 'MergedEvent': lambda e: + "Merged!", + 'ClosedEvent': lambda e: + "Closed.", + 'ReopenedEvent': lambda e: + "Reopened.", + } + + if self.event_type in d: + r = d[self.event_type](self) + if r: + return r + else: + return None + else: + return None + + +def all(access_token, owner, repo, pull): + after = None + result = PullRequestResponse() + result.owner = owner + result.repo = repo + result.pull = pull + + while True: + response = one(access_token=access_token, + owner=owner, + repo=repo, + pull=pull, + after=after) + if response.status_code == 502: + # 502s happen sometimes when talking to GitHub. Try again. + time.sleep(1) + continue + + r = response.json() + + pull_request = r['data']['repository']['pullRequest'] + page_info = pull_request['timelineItems']['pageInfo'] + events = pull_request['timelineItems']['nodes'] + + result.title = pull_request['title'] + result.author = pull_request['author']['login'] + result.state = pull_request['state'] + result.head_sha = pull_request['headRefOid'] + result.mergeable = pull_request['mergeable'] + + result.events.extend([PullRequestEvent(e) for e in events]) + + if not page_info['hasNextPage']: + break + after = page_info['endCursor'] + + return result + + +def one(access_token, owner, repo, pull, after): + headers = { + 'authorization': 'bearer ' + access_token, + 'accept': 'application/json', + } + json = { + 'query': QUERY, + 'variables': { + 'repoName': repo, + 'repoOwner': owner, + 'pull': int(pull), + 'after': after, + } + } + result = requests.post('https://api.github.com/graphql', + headers=headers, + json=json) + + return result diff --git a/homu/tests/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py index 451ae9e..a0a2bf1 100644 --- a/homu/tests/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -19,6 +19,8 @@ def test_r_plus(): command = commands[0] assert command.action == 'approve' assert command.actor == 'jack' + assert command.commit_was_specified is False + assert command.commit == commit def test_r_plus_with_colon(): @@ -51,6 +53,7 @@ def test_r_plus_with_sha(): assert command.action == 'approve' assert command.actor == 'jack' assert command.commit == other_commit + assert command.commit_was_specified is True def test_r_equals(): @@ -66,6 +69,8 @@ def test_r_equals(): command = commands[0] assert command.action == 'approve' assert command.actor == 'jill' + assert command.commit == commit + assert command.commit_was_specified is False def test_hidden_r_equals(): @@ -82,6 +87,7 @@ def test_hidden_r_equals(): assert command.action == 'approve' assert command.actor == 'jack' assert command.commit == commit + assert command.commit_was_specified is True def test_r_me(): diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py new file mode 100644 index 0000000..6459c3a --- /dev/null +++ b/homu/tests/test_process_event.py @@ -0,0 +1,397 @@ +import unittest.mock +import re +from homu.comments import Comment + +from homu.pull_req_state import ( + PullReqState, + # ProcessEventResult, +) +from homu.pull_request_events import ( + PullRequestEvent +) + + +def new_state(num=1, head_sha='abcdef', status='', title='A change'): + repo = unittest.mock.Mock() + repo.treeclosed = False + + state = PullReqState( + num=num, + head_sha=head_sha, + status=status, + db=None, + repo_label='test', + mergeable_que=None, + gh=None, + owner='test-org', + name='test-repo', + label_events=[], + repos={ + 'test': repo, + }) + + state.title = title + + return state + + +def render_comment(comment): + if isinstance(comment, Comment): + return comment.render() + else: + return comment + + +def assert_comment(pattern, comments): + for comment in comments: + if re.search(pattern, render_comment(comment)) is not None: + return True + + return False + + +def create_event(event): + return PullRequestEvent(event) + + +def create_events(*args, into=None): + if into is None: + into = [] + + for arg in args: + into.append(create_event(arg)) + + return into + + +def test_baseline(): + """ + Test that a new pull request does not have any state + """ + + state = new_state() + assert state.get_status() == '' + + +def test_current_sha(): + """ + Test that a pull request gets the current head sha + """ + + state = new_state() + event = create_event({ + 'eventType': 'PullRequestCommit', + 'commit': { + 'oid': '012345', + } + }) + result = state.process_event(event) + assert result.changed is True + assert state.head_sha == '012345' + + state = new_state() + event = create_event({ + 'eventType': 'HeadRefForcePushedEvent', + 'actor': { + 'login': 'ferris', + }, + 'beforeCommit': { + 'oid': 'abcdef', + }, + 'afterCommit': { + 'oid': '012345', + }, + }) + result = state.process_event(event) + assert result.changed is True + assert state.head_sha == '012345' + + +def return_true(a, b, c, d, e, f): + return True + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', side_effect=return_true) +def test_approved(_): + """ + Test that a pull request that has been approved is still approved + """ + + # Typical approval + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is True + assert assert_comment(r'Commit abcdef has been approved', result.comments) + assert state.approved_by == 'ferris' + assert state.get_status() == 'approved' + + # Approval by someone else + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r=someone', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is True + assert assert_comment(r'Commit abcdef has been approved', result.comments) + assert state.approved_by == 'someone' + assert state.get_status() == 'approved' + + # Approval with commit sha + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+ abcdef', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is True + assert assert_comment(r'Commit abcdef has been approved', result.comments) + assert state.get_status() == 'approved' + + # Approval with commit sha by bors + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': 'Commit abcdef has been approved\n\n', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is True + for comment in result.comments: + print(render_comment(comment)) + assert len(result.comments) == 0 + assert state.get_status() == 'approved' + + # Approval of WIP + state = new_state(head_sha='abcdef', title="[WIP] A change") + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is False + assert assert_comment(r'still in progress', result.comments) + assert state.get_status() == '' + + # Approval with invalid commit sha + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+ 012345', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is False + assert assert_comment(r'`012345` is not a valid commit SHA', result.comments) + assert state.get_status() == '' + + # Approval of already approved state + state = new_state(head_sha='abcdef') + event1 = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + }) + event2 = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bill', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:01:00Z', + }) + result1 = state.process_event(event1) + assert result1.changed is True + assert state.get_status() == 'approved' + result2 = state.process_event(event2) + assert result2.changed is False + assert assert_comment(r'already approved', result2.comments) + assert state.get_status() == 'approved' + +# +#def test_tried(): +# """ +# Test that a pull request that has been tried shows up as tried +# """ +# +# +#def test_tried_and_approved(): +# """ +# Test that a pull request that has been approved AND tried shows up as +# approved AND tried +# """ +# +# +#def test_approved_unapproved(): +# """ +# Test that a pull request that was r+'ed, but then r-'ed shows up as +# unapproved. I.e., there isn't a bug that allows an unapproved item to +# all of a sudden be approved after a restart. +# """ +# +# state = new_state() +# events = create_events({ +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors r+', +# 'publishedAt': '1985-04-21T00:00:00Z', +# }, { +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors r-', +# 'publishedAt': '1985-04-21T00:01:00Z', +# }) +# +# for event in events: +# state.process_event(event) +# +# assert state.get_status() == '' +# +# +#def test_approved_changed_push(): +# """ +# Test that a pull request that was r+'ed, but then had more commits +# pushed is not listed as approved. +# """ +# +# # Regular push +# state = new_state() +# events = create_events({ +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors r+', +# 'publishedAt': '1985-04-21T00:00:00Z', +# }, { +# 'eventType': 'PullRequestCommit', +# 'commit': { +# 'oid': '012345', +# }, +# }) +# +# for event in events: +# state.process_event(event) +# +# assert state.get_status() == '' +# assert state.head_sha == '012345' +# +# # Force push +# state = new_state() +# events = create_events({ +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors r+', +# 'publishedAt': '1985-04-21T00:00:00Z', +# }, { +# 'eventType': 'HeadRefForcePushedEvent', +# 'actor': { +# 'login': 'ferris', +# }, +# 'beforeCommit': { +# 'oid': 'abcdef', +# }, +# 'afterCommit': { +# 'oid': '012345', +# }, +# }) +# +# for event in events: +# state.process_event(event) +# +# assert state.get_status() == '' +# assert state.head_sha == '012345' +# +# +#def test_approved_changed_base(): +# """ +# Test that a pull request that was r+'ed, but then changed its base is +# not listed as approved. +# """ +# +# state = new_state() +# events = create_events({ +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors r+', +# 'publishedAt': '1985-04-21T00:00:00Z', +# }, { +# 'eventType': 'BaseRefChanged', +# 'actor': { +# 'login': 'ferris', +# }, +# }) +# +# for event in events: +# state.process_event(event) +# +# assert state.get_status() == '' +# +#def test_pending(): +# """ +# Test that a pull request that started before the service was restarted +# but didn't finish is still marked as pending. +# +# Currently we don't reach out to see if the build is still running or if +# it finished while we were off. +# """ +# +# +#def test_priority(): +# """ +# Test that priority values stick +# """ +# +# state = new_state() +# event = create_event({ +# 'eventType': 'IssueComment', +# 'author': { +# 'login': 'ferris', +# }, +# 'body': '@bors p=20', +# 'publishedAt': '1985-04-21T00:00:00Z', +# }) +# state.process_event(event) +# assert state.priority == 20 +# +#def test_rollup(): +# """ +# Test that rollup values stick +# """ From b693b15a3fe51783647abb53027dfe4c45f7f0d6 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Thu, 13 Jun 2019 14:15:24 -0500 Subject: [PATCH 06/17] More tests written and passing --- homu/pull_req_state.py | 97 +++++---- homu/tests/test_process_event.py | 338 +++++++++++++++++-------------- 2 files changed, 240 insertions(+), 195 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 209de3d..12eade0 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -339,6 +339,11 @@ def process_event(self, event): applied as a result of this event. """ + # TODO: Don't hardcode botname! + botname = 'bors' + # TODO: Don't hardcode hooks! + hooks = [] + result = ProcessEventResult() if event.event_type == 'PullRequestCommit': result.changed = self.head_sha != event['commit']['oid'] @@ -354,14 +359,18 @@ def process_event(self, event): result.changed = result.changed or self.approved_by != '' self.approved_by = '' + elif event.event_type == 'BaseRefChangedEvent': + # Base ref changed: no longer approved + result.changed = self.approved_by != '' + self.approved_by = '' + + elif event.event_type == 'IssueComment': comments = parse_issue_comment( username=event['author']['login'], body=event['body'], sha=self.head_sha, - # TODO: Don't hardcode 'bors' - botname='bors', - # TODO: Hooks! + botname=botname, hooks=[]) for comment in comments: @@ -441,11 +450,13 @@ def process_issue_comment(self, event, command): # TODO: Don't hardcode botname botname = 'bors' username = event['author']['login'] + # TODO: Don't hardcode repo_cfg + repo_cfg = {} _assert_reviewer_auth_verified = functools.partial( assert_authorized, username, self.repo_label, - {}, # repo_cfg + repo_cfg, self, AuthState.REVIEWER, botname, @@ -454,7 +465,7 @@ def process_issue_comment(self, event, command): assert_authorized, username, self.repo_label, - {}, # repo_cfg + repo_cfg, self, AuthState.TRY, botname, @@ -542,37 +553,38 @@ def process_issue_comment(self, event, command): .format(treeclosed) ) -# 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 -# # unapprove the PR (change the target branch, push more -# # commits, etc.) so allowing them to directly unapprove it is -# # also allowed. -# if state.author != username: -# assert_authorized(username, repo_label, repo_cfg, state, -# AuthState.REVIEWER, my_username) -# -# state.approved_by = '' -# state.save() -# if realtime: -# state.change_labels(LabelEvent.REJECTED) -# -# elif command.action == 'prioritize': -# assert_authorized(username, repo_label, repo_cfg, state, -# AuthState.TRY, my_username) -# -# pvalue = command.priority -# -# if pvalue > global_cfg['max_priority']: -# if realtime: -# state.add_comment( -# ':stop_sign: Priority higher than {} is ignored.' -# .format(global_cfg['max_priority']) -# ) -# #continue -# state.priority = pvalue -# state.save() -# + 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 + # unapprove the PR (change the target branch, push more + # commits, etc.) so allowing them to directly unapprove it is + # also allowed. + if self.author != username: + assert_authorized(username, self.repo_label, repo_cfg, self, + AuthState.REVIEWER, botname) + + self.approved_by = '' + result.changed = True + result.label_events.append(LabelEvent.REJECTED) + + elif command.action == 'prioritize': + assert_authorized(username, self.repo_label, repo_cfg, self, + AuthState.TRY, botname) + + pvalue = command.priority + + # TODO: Don't hardcode max_priority + # global_cfg['max_priority'] + max_priority = 9001 + if pvalue > max_priority: + result.comments.append( + ':stop_sign: Priority higher than {} is ignored.' + .format(max_priority) + ) + return result + result.changed = self.priority != pvalue + self.priority = pvalue + # elif command.action == 'delegate': # assert_authorized(username, repo_label, repo_cfg, state, # AuthState.REVIEWER, my_username) @@ -644,13 +656,12 @@ def process_issue_comment(self, event, command): # # to any meaningful labeling events. # state.change_labels(LabelEvent.TRY) # -# elif command.action == 'rollup': -# _assert_try_auth_verified() -# -# state.rollup = command.rollup_value -# -# state.save() -# + elif command.action == 'rollup': + _assert_try_auth_verified() + + result.changed = self.rollup != command.rollup_value + self.rollup = command.rollup_value + # elif command.action == 'force' and realtime: # _assert_try_auth_verified() # diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 6459c3a..e70d6ad 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -1,6 +1,9 @@ import unittest.mock import re from homu.comments import Comment +from homu.consts import ( + LabelEvent, +) from homu.pull_req_state import ( PullReqState, @@ -54,16 +57,6 @@ def create_event(event): return PullRequestEvent(event) -def create_events(*args, into=None): - if into is None: - into = [] - - for arg in args: - into.append(create_event(arg)) - - return into - - def test_baseline(): """ Test that a new pull request does not have any state @@ -111,7 +104,8 @@ def return_true(a, b, c, d, e, f): return True -@unittest.mock.patch('homu.pull_req_state.assert_authorized', side_effect=return_true) +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) def test_approved(_): """ Test that a pull request that has been approved is still approved @@ -171,7 +165,11 @@ def test_approved(_): 'author': { 'login': 'bors', }, - 'body': 'Commit abcdef has been approved\n\n', + 'body': ''' + Commit abcdef has been approved + + + ''', 'publishedAt': '1985-04-21T00:00:00Z', }) result = state.process_event(event) @@ -208,7 +206,8 @@ def test_approved(_): }) result = state.process_event(event) assert result.changed is False - assert assert_comment(r'`012345` is not a valid commit SHA', result.comments) + assert assert_comment(r'`012345` is not a valid commit SHA', + result.comments) assert state.get_status() == '' # Approval of already approved state @@ -237,7 +236,7 @@ def test_approved(_): assert assert_comment(r'already approved', result2.comments) assert state.get_status() == 'approved' -# + #def test_tried(): # """ # Test that a pull request that has been tried shows up as tried @@ -249,121 +248,125 @@ def test_approved(_): # Test that a pull request that has been approved AND tried shows up as # approved AND tried # """ -# -# -#def test_approved_unapproved(): -# """ -# Test that a pull request that was r+'ed, but then r-'ed shows up as -# unapproved. I.e., there isn't a bug that allows an unapproved item to -# all of a sudden be approved after a restart. -# """ -# -# state = new_state() -# events = create_events({ -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors r+', -# 'publishedAt': '1985-04-21T00:00:00Z', -# }, { -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors r-', -# 'publishedAt': '1985-04-21T00:01:00Z', -# }) -# -# for event in events: -# state.process_event(event) -# -# assert state.get_status() == '' -# -# -#def test_approved_changed_push(): -# """ -# Test that a pull request that was r+'ed, but then had more commits -# pushed is not listed as approved. -# """ -# -# # Regular push -# state = new_state() -# events = create_events({ -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors r+', -# 'publishedAt': '1985-04-21T00:00:00Z', -# }, { -# 'eventType': 'PullRequestCommit', -# 'commit': { -# 'oid': '012345', -# }, -# }) -# -# for event in events: -# state.process_event(event) -# -# assert state.get_status() == '' -# assert state.head_sha == '012345' -# -# # Force push -# state = new_state() -# events = create_events({ -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors r+', -# 'publishedAt': '1985-04-21T00:00:00Z', -# }, { -# 'eventType': 'HeadRefForcePushedEvent', -# 'actor': { -# 'login': 'ferris', -# }, -# 'beforeCommit': { -# 'oid': 'abcdef', -# }, -# 'afterCommit': { -# 'oid': '012345', -# }, -# }) -# -# for event in events: -# state.process_event(event) -# -# assert state.get_status() == '' -# assert state.head_sha == '012345' -# -# -#def test_approved_changed_base(): -# """ -# Test that a pull request that was r+'ed, but then changed its base is -# not listed as approved. -# """ -# -# state = new_state() -# events = create_events({ -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors r+', -# 'publishedAt': '1985-04-21T00:00:00Z', -# }, { -# 'eventType': 'BaseRefChanged', -# 'actor': { -# 'login': 'ferris', -# }, -# }) -# -# for event in events: -# state.process_event(event) -# -# assert state.get_status() == '' -# + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_approved_unapproved(_): + """ + Test that a pull request that was r+'ed, but then r-'ed shows up as + unapproved. I.e., there isn't a bug that allows an unapproved item to + all of a sudden be approved after a restart. + """ + + state = new_state() + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.get_status() != '' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r-', + 'publishedAt': '1985-04-21T00:01:00Z', + })) + assert state.get_status() == '' + assert state.approved_by == '' + assert result.changed is True + assert len(result.label_events) == 1 + assert result.label_events[0] == LabelEvent.REJECTED + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_approved_changed_push(_): + """ + Test that a pull request that was r+'ed, but then had more commits + pushed is not listed as approved. + """ + + # Regular push + state = new_state() + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + state.process_event(create_event({ + 'eventType': 'PullRequestCommit', + 'commit': { + 'oid': '012345', + }, + })) + + assert state.get_status() == '' + assert state.head_sha == '012345' + + # Force push + state = new_state() + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + state.process_event(create_event({ + 'eventType': 'HeadRefForcePushedEvent', + 'actor': { + 'login': 'ferris', + }, + 'beforeCommit': { + 'oid': 'abcdef', + }, + 'afterCommit': { + 'oid': '012345', + }, + })) + + assert state.get_status() == '' + assert state.head_sha == '012345' + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_approved_changed_base(_): + """ + Test that a pull request that was r+'ed, but then changed its base is + not listed as approved. + """ + + state = new_state() + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + state.process_event(create_event({ + 'eventType': 'BaseRefChangedEvent', + 'actor': { + 'login': 'ferris', + }, + })) + + assert state.get_status() == '' + + #def test_pending(): # """ # Test that a pull request that started before the service was restarted @@ -372,26 +375,57 @@ def test_approved(_): # Currently we don't reach out to see if the build is still running or if # it finished while we were off. # """ -# -# -#def test_priority(): -# """ -# Test that priority values stick -# """ -# -# state = new_state() -# event = create_event({ -# 'eventType': 'IssueComment', -# 'author': { -# 'login': 'ferris', -# }, -# 'body': '@bors p=20', -# 'publishedAt': '1985-04-21T00:00:00Z', -# }) -# state.process_event(event) -# assert state.priority == 20 -# -#def test_rollup(): -# """ -# Test that rollup values stick -# """ + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_priority(_): + """ + Test that priority values stick + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors p=20', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.priority == 20 + assert result.changed is True + assert len(result.comments) == 0 + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors p=9002', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.priority == 0 + assert result.changed is False + assert assert_comment(r'Priority.*is ignored', result.comments) + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_rollup(_): + """ + Test that rollup values stick + """ + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors rollup=always', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.rollup == 1 + assert result.changed is True + assert len(result.comments) == 0 From 7f9443b04f94fdea2ab66e6d0e9c606814b83c70 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 14 Jun 2019 06:31:14 -0500 Subject: [PATCH 07/17] Use `process_event` in synchronization --- homu/main.py | 128 ++++++++++++++++++------------- homu/pull_req_state.py | 16 +++- homu/pull_request_events.py | 11 +++ homu/tests/test_process_event.py | 1 + 4 files changed, 101 insertions(+), 55 deletions(-) diff --git a/homu/main.py b/homu/main.py index abc74a4..92a3b55 100644 --- a/homu/main.py +++ b/homu/main.py @@ -33,6 +33,7 @@ import shlex import random from .pull_req_state import PullReqState +from .pull_request_events import all as all_pull_request_events global_cfg = {} @@ -1166,66 +1167,85 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q states[repo_label] = {} repos[repo_label] = Repository(repo, repo_label, db) + print("Getting pulls...") for pull in repo.iter_pulls(state='open'): - db.execute( - 'SELECT status FROM pull WHERE repo = ? AND num = ?', - [repo_label, pull.number]) - row = db.fetchone() - if row: - status = row[0] - else: - status = '' - for info in utils.github_iter_statuses(repo, pull.head.sha): - if info.context == 'homu': - status = info.state - break +# db.execute( +# 'SELECT status FROM pull WHERE repo = ? AND num = ?', +# [repo_label, pull.number]) +# row = db.fetchone() +# if row: +# status = row[0] +# else: +# status = '' +# for info in utils.github_iter_statuses(repo, pull.head.sha): +# if info.context == 'homu': +# status = info.state +# break + + if pull.number in [60966, 60730, 60547, 59312]: + # TODO: WHY DOES THIS HAPPEN!? + print("Skipping {} because GraphQL never returns a success!".format(pull.number)) + continue + + print("{}/{}#{}".format(repo_cfg['owner'], repo_cfg['name'], pull.number)) + access_token = global_cfg['github']['access_token'] + response = all_pull_request_events(access_token, repo_cfg['owner'], repo_cfg['name'], pull.number) + status = '' state = PullReqState(pull.number, pull.head.sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa - state.title = pull.title + state.cfg = repo_cfg + state.title = response.initial_title state.body = pull.body state.head_ref = pull.head.repo[0] + ':' + pull.head.ref state.base_ref = pull.base.ref - state.set_mergeable(None) - state.assignee = pull.assignee.login if pull.assignee else '' - - for comment in pull.iter_comments(): - if comment.original_commit_id == pull.head.sha: - parse_commands( - comment.body, - comment.user.login, - repo_label, - repo_cfg, - state, - my_username, - db, - states, - sha=comment.original_commit_id, - command_src=comment.to_json()['html_url'], - # FIXME switch to `comment.html_url` - # after updating github3 to 1.3.0+ - ) - - for comment in pull.iter_issue_comments(): - parse_commands( - comment.body, - comment.user.login, - repo_label, - repo_cfg, - state, - my_username, - db, - states, - command_src=comment.to_json()['html_url'], - # FIXME switch to `comment.html_url` - # after updating github3 to 1.3.0+ - ) - - saved_state = saved_states.get(pull.number) - if saved_state: - for key, val in saved_state.items(): - setattr(state, key, val) - - state.save() + if response.mergeable == 'MERGEABLE': + state.set_mergeable(True) + elif response.mergeable == 'CONFLICTING': + state.set_mergeable(False) + else: + state.set_mergeable(None) + state.assignee = '' + +# for comment in pull.iter_comments(): +# if comment.original_commit_id == pull.head.sha: +# parse_commands( +# comment.body, +# comment.user.login, +# repo_label, +# repo_cfg, +# state, +# my_username, +# db, +# states, +# sha=comment.original_commit_id, +# command_src=comment.to_json()['html_url'], +# # FIXME switch to `comment.html_url` +# # after updating github3 to 1.3.0+ +# ) +# +# for comment in pull.iter_issue_comments(): +# parse_commands( +# comment.body, +# comment.user.login, +# repo_label, +# repo_cfg, +# state, +# my_username, +# db, +# states, +# command_src=comment.to_json()['html_url'], +# # FIXME switch to `comment.html_url` +# # after updating github3 to 1.3.0+ +# ) +# +# saved_state = saved_states.get(pull.number) +# if saved_state: +# for key, val in saved_state.items(): +# setattr(state, key, val) +# +# state.save() + for event in response.events: + state.process_event(event) states[repo_label][pull.number] = state diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 12eade0..409adbc 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -420,6 +420,17 @@ def process_event(self, event): # We don't care about any of these events. pass + elif event.event_type in [ + 'UnassignedEvent', + 'MilestonedEvent', + 'DemilestonedEvent', + 'ReviewRequestedEvent', + 'ReviewDismissedEvent', + 'CommentDeletedEvent']: + # TODO! Review these events to see if we care about any of them. + # These events were seen as "Unknown event type: {}" when doing initial testing. + pass + else: # Ooops, did we miss this event type? Or is it new? print("Unknown event type: {}".format(event.event_type)) @@ -451,7 +462,9 @@ def process_issue_comment(self, event, command): botname = 'bors' username = event['author']['login'] # TODO: Don't hardcode repo_cfg - repo_cfg = {} + #repo_cfg = {} + repo_cfg = self.cfg + _assert_reviewer_auth_verified = functools.partial( assert_authorized, username, @@ -744,6 +757,7 @@ def process_issue_comment(self, event, command): state_changed = True except AuthorizationException as e: + print("{} is unauthorized".format(event['author']['login'])) result.comments.append(e.comment) return result diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py index 489c49c..911d40f 100644 --- a/homu/pull_request_events.py +++ b/homu/pull_request_events.py @@ -244,6 +244,17 @@ def all(access_token, owner, repo, pull): r = response.json() + if 'errors' in r: + print("GraphQL query failed:") + for error in r['errors']: + print(" * {}".format(error['message'])) + time.sleep(1) + continue + + if 'data' not in r: + print("response.status_code = {}".format(response.status_code)) + print("r = {}".format(r)) + pull_request = r['data']['repository']['pullRequest'] page_info = pull_request['timelineItems']['pageInfo'] events = pull_request['timelineItems']['nodes'] diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index e70d6ad..f02f054 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -34,6 +34,7 @@ def new_state(num=1, head_sha='abcdef', status='', title='A change'): }) state.title = title + state.cfg = {} return state From 2c6d3b331aa946629893aa41f68fadd132f5f113 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 14 Jun 2019 14:16:44 -0500 Subject: [PATCH 08/17] Test builds and try builds --- homu/pull_req_state.py | 87 ++++++++----- homu/tests/test_process_event.py | 215 ++++++++++++++++++++++++++++++- 2 files changed, 266 insertions(+), 36 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 409adbc..31f4643 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -426,7 +426,8 @@ def process_event(self, event): 'DemilestonedEvent', 'ReviewRequestedEvent', 'ReviewDismissedEvent', - 'CommentDeletedEvent']: + 'CommentDeletedEvent', + 'PullRequestCommitCommentThread']: # TODO! Review these events to see if we care about any of them. # These events were seen as "Unknown event type: {}" when doing initial testing. pass @@ -750,6 +751,12 @@ def process_issue_comment(self, event, command): # args=[state, hook_cfg, body, command.hook_extra] # ).start() + elif command.action == 'homu-state' and username == botname: + subresult = self.process_homu_state(event, command) + result.comments.extend(subresult.comments) + result.label_events.extend(subresult.label_events) + result.changed = subresult.changed + else: found = False @@ -766,51 +773,71 @@ def process_homu_state(self, event, command): result = ProcessEventResult() state = command.homu_state + if state['type'] == 'Approved': - # TODO: Something with states - result.changed = self.approval_state != 'approved' - result.changed = result.changed or self.approver != state['approver'] - self.approval_state = 'approved' + result.changed = self.approved_by != state['approver'] self.approved_by = state['approver'] elif state['type'] == 'BuildStarted': - # TODO: Something with states result.changed = True - self.build_state = 'pending' + self.try_ = False + self.status = 'pending' + # TODO: Something with states +# result.changed = True +# self.build_state = 'pending' + pass elif state['type'] == 'BuildCompleted': - # TODO: Something with states result.changed = True - self.build_state = 'completed' + self.try_ = False + self.status = 'completed' + # TODO: Something with states +# result.changed = True +# self.build_state = 'completed' + pass elif state['type'] == 'BuildFailed': - # TODO: Something with states result.changed = True - self.build_state = 'failure' + self.try_ = False + self.status = 'failure' + # TODO: Something with states +# result.changed = True +# self.build_state = 'failure' + pass elif state['type'] == 'TryBuildStarted': - # TODO: Multiple tries? result.changed = True - self.tries.append(PullRequestTry( - len(self.tries) + 1, - state['head_sha'], - state['merge_sha'], - event['publishedAt']) - ) + self.try_ = True + self.status = 'pending' + # TODO: Multiple tries? + # result.changed = True + # self.tries.append(PullRequestTry( + # len(self.tries) + 1, + # state['head_sha'], + # state['merge_sha'], + # event['publishedAt']) + # ) elif state['type'] == 'TryBuildCompleted': - item = next((try_ - for try_ in self.tries - if try_.state == 'pending' - and try_.merge_sha == state['merge_sha']), - None) - - if item: - result.changed = True - # TODO: Multiple tries? - item.ended_at = event['publishedAt'] - item.state = 'completed' - item.builders = state['builders'] + result.changed = True + self.status = 'success' + # TODO: Multiple tries? + # item = next((try_ + # for try_ in self.tries + # if try_.state == 'pending' + # and try_.merge_sha == state['merge_sha']), + # None) + # + # if item: + # result.changed = True + # # TODO: Multiple tries? + # item.ended_at = event['publishedAt'] + # item.state = 'completed' + # item.builders = state['builders'] + + elif state['type'] == 'TryBuildFailed': + result.changed = True + self.status = 'failure' return result diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index f02f054..b08b95b 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -238,12 +238,215 @@ def test_approved(_): assert state.get_status() == 'approved' -#def test_tried(): -# """ -# Test that a pull request that has been tried shows up as tried -# """ -# -# +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_homu_state_approval(_): + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + Commit abcdef has been approved + + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is True + assert len(result.comments) == 0 + assert state.get_status() == 'approved' + assert state.approved_by == 'ferris' + + # Nobody but bors can use homu state + state = new_state(head_sha='abcdef') + event = create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': ''' + Commit abcdef has been approved + + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + }) + result = state.process_event(event) + assert result.changed is False + assert len(result.comments) == 0 + assert state.get_status() == '' + assert state.approved_by == '' + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_tried(_): + """ + Test that a pull request that has been tried shows up as tried + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_try_failed(_): + """ + Test that a pull request that has been tried shows up as tried + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'failure' + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_build(_): + """ + Test that a pull request that has been built shows up as built. This is + maybe a bad test because a PR that has been built and succeeds will likely + be merged and removed. + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Building commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'pending' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'success' + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_build_failed(_): + """ + Test that a pull request that has been built and failed shows up that way. + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Building commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Build failed - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'failure' + + #def test_tried_and_approved(): # """ # Test that a pull request that has been approved AND tried shows up as From 2526ffe70376e4f831a964cbe75404525bd04dd5 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Thu, 20 Jun 2019 15:31:35 -0500 Subject: [PATCH 09/17] Pushed commits reset the `try_` state --- homu/main.py | 14 +++++--- homu/pull_req_state.py | 6 ++++ homu/pull_request_events.py | 5 +++ homu/tests/test_process_event.py | 59 ++++++++++++++++++++++++++++++-- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/homu/main.py b/homu/main.py index 92a3b55..8b81163 100644 --- a/homu/main.py +++ b/homu/main.py @@ -1182,14 +1182,18 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q # status = info.state # break - if pull.number in [60966, 60730, 60547, 59312]: - # TODO: WHY DOES THIS HAPPEN!? - print("Skipping {} because GraphQL never returns a success!".format(pull.number)) - continue +# if pull.number in [60966, 60730, 60547, 59312]: +# # TODO: WHY DOES THIS HAPPEN!? +# # Reported to GitHub. They're working on it. +# print("Skipping {} because GraphQL never returns a success!".format(pull.number)) +# continue print("{}/{}#{}".format(repo_cfg['owner'], repo_cfg['name'], pull.number)) access_token = global_cfg['github']['access_token'] - response = all_pull_request_events(access_token, repo_cfg['owner'], repo_cfg['name'], pull.number) + try: + response = all_pull_request_events(access_token, repo_cfg['owner'], repo_cfg['name'], pull.number) + except: + continue status = '' state = PullReqState(pull.number, pull.head.sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 31f4643..10be612 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -351,6 +351,11 @@ def process_event(self, event): # New commits come in: no longer approved result.changed = result.changed or self.approved_by != '' self.approved_by = '' + result.changed = result.changed or self.try_ != False + self.try_ = False + # TODO: Do we *always* reset the state? + result.changed = result.changed or self.status != '' + self.status = '' elif event.event_type == 'HeadRefForcePushedEvent': result.changed = self.head_sha != event['afterCommit']['oid'] @@ -412,6 +417,7 @@ def process_event(self, event): elif event.event_type in [ 'SubscribedEvent', + 'UnsubscribedEvent', 'MentionedEvent', 'LabeledEvent', 'UnlabeledEvent', diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py index 911d40f..ead12c1 100644 --- a/homu/pull_request_events.py +++ b/homu/pull_request_events.py @@ -231,6 +231,8 @@ def all(access_token, owner, repo, pull): result.repo = repo result.pull = pull + attempt = 1 + while True: response = one(access_token=access_token, owner=owner, @@ -245,6 +247,9 @@ def all(access_token, owner, repo, pull): r = response.json() if 'errors' in r: + if attempt == 10: + raise Exception("Too many errors") + attempt += 1 print("GraphQL query failed:") for error in r['errors']: print(" * {}".format(error['message'])) diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index b08b95b..df23dc9 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -363,6 +363,59 @@ def test_try_failed(_): assert state.get_status() == 'failure' +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_try_reset_by_push(_): + """ + Test that a pull request that has been tried, and new commits pushed, does + not show up as tried + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + + result = state.process_event(create_event({ + 'eventType': 'PullRequestCommit', + 'commit': { + 'oid': '012345', + } + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == '' + + @unittest.mock.patch('homu.pull_req_state.assert_authorized', side_effect=return_true) def test_build(_): @@ -403,7 +456,7 @@ def test_build(_): assert result.changed is True assert state.try_ is False - assert state.get_status() == 'success' + assert state.get_status() == 'completed' @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -427,7 +480,7 @@ def test_build_failed(_): })) assert result.changed is True - assert state.try_ is True + assert state.try_ is False assert state.get_status() == 'pending' result = state.process_event(create_event({ @@ -443,7 +496,7 @@ def test_build_failed(_): })) assert result.changed is True - assert state.try_ is True + assert state.try_ is False assert state.get_status() == 'failure' From a024fbe1f0c972159cc099361a9e990e2e3975ff Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Tue, 2 Jul 2019 09:14:59 -0500 Subject: [PATCH 10/17] Keep track of cursor and break up status Break up the "status" field into multiple orthogonal state fields: * build state (whether the primary is running or has succeeded or failed) * try state (whether the most recent try is running or has succeeded or failed) * approval state (whether the pull request is approved) Previously (and still) these were mostly possible to determine by looking at `state.get_status()` and `state.try_`, but storing them separately helps make state changes more explicit. Also, keep track of the current github synchronization cursor in the pull request state, so that we can use it later. --- homu/pull_req_state.py | 56 +++++++++--- homu/pull_request_events.py | 152 +++++++++++++++++-------------- homu/tests/test_process_event.py | 48 +++++++++- 3 files changed, 173 insertions(+), 83 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 10be612..a8b9440 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -15,6 +15,7 @@ AuthorizationException, AuthState, ) +from enum import Enum class ProcessEventResult: @@ -40,6 +41,26 @@ def sha_or_blank(sha): return sha if re.match(r'^[0-9a-f]+$', sha) else '' + +class BuildState(Enum): + """ + The state of a merge build or a try build + """ + NONE = 'none' + PENDING = 'pending' + SUCCESS = 'success' + FAILURE = 'failure' + ERROR = 'error' + + +class ApprovalState(Enum): + """ + The approval state for a pull request. + """ + UNAPPROVED = 'unapproved' + APPROVED = 'approved' + + class PullReqState: num = 0 priority = 0 @@ -50,6 +71,9 @@ class PullReqState: base_ref = '' assignee = '' delegate = '' + last_github_cursor = None + build_state = BuildState.NONE + try_state = BuildState.NONE def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, gh, owner, name, label_events, repos): @@ -94,6 +118,13 @@ def __repr__(self): self.status, ) + @property + def approval_state(self): + if self.approved_by != '': + return ApprovalState.APPROVED + else: + return ApprovalState.UNAPPROVED + def sort_key(self): return [ STATUS_TO_PRIORITY.get(self.get_status(), -1), @@ -339,6 +370,8 @@ def process_event(self, event): applied as a result of this event. """ + self.last_github_cursor = event.cursor + # TODO: Don't hardcode botname! botname = 'bors' # TODO: Don't hardcode hooks! @@ -356,6 +389,10 @@ def process_event(self, event): # TODO: Do we *always* reset the state? result.changed = result.changed or self.status != '' self.status = '' + result.changed = result.changed or self.try_state != BuildState.NONE + self.try_state = BuildState.NONE + result.changed = result.changed or self.build_state != BuildState.NONE + self.build_state = BuildState.NONE elif event.event_type == 'HeadRefForcePushedEvent': result.changed = self.head_sha != event['afterCommit']['oid'] @@ -363,6 +400,7 @@ def process_event(self, event): # New commits come in: no longer approved result.changed = result.changed or self.approved_by != '' self.approved_by = '' + # TODO: Do we need to reset the state here? elif event.event_type == 'BaseRefChangedEvent': # Base ref changed: no longer approved @@ -788,33 +826,25 @@ def process_homu_state(self, event, command): result.changed = True self.try_ = False self.status = 'pending' - # TODO: Something with states -# result.changed = True -# self.build_state = 'pending' - pass + self.build_state = BuildState.PENDING elif state['type'] == 'BuildCompleted': result.changed = True self.try_ = False self.status = 'completed' - # TODO: Something with states -# result.changed = True -# self.build_state = 'completed' - pass + self.build_state = BuildState.SUCCESS elif state['type'] == 'BuildFailed': result.changed = True self.try_ = False self.status = 'failure' - # TODO: Something with states -# result.changed = True -# self.build_state = 'failure' - pass + self.build_state = BuildState.FAILURE elif state['type'] == 'TryBuildStarted': result.changed = True self.try_ = True self.status = 'pending' + self.try_state = BuildState.PENDING # TODO: Multiple tries? # result.changed = True # self.tries.append(PullRequestTry( @@ -827,6 +857,7 @@ def process_homu_state(self, event, command): elif state['type'] == 'TryBuildCompleted': result.changed = True self.status = 'success' + self.try_state = BuildState.SUCCESS # TODO: Multiple tries? # item = next((try_ # for try_ in self.tries @@ -844,6 +875,7 @@ def process_homu_state(self, event, command): elif state['type'] == 'TryBuildFailed': result.changed = True self.status = 'failure' + self.try_state = BuildState.FAILURE return result diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py index ead12c1..3e6f065 100644 --- a/homu/pull_request_events.py +++ b/homu/pull_request_events.py @@ -18,89 +18,98 @@ hasNextPage endCursor } - nodes { - eventType: __typename - ... on PullRequestCommit { - commit { - oid + edges { + cursor + node { + eventType: __typename + ... on PullRequestCommit { + commit { + oid + } } - } - ... on AssignedEvent { - actor { - login + ... on AssignedEvent { + actor { + login + } + user { + login + } } - user { - login + ... on UnassignedEvent { + actor { + login + } + user { + login + } } - } - ... on UnassignedEvent { - actor { - login + ... on IssueComment { + author { + login + } + body + publishedAt } - user { - login + ... on SubscribedEvent { + actor { + login + } } - } - ... on IssueComment { - author { - login + ... on LabeledEvent { + actor { + login + } + label { + name + } } - body - publishedAt - } - ... on SubscribedEvent { - actor { - login + ... on UnlabeledEvent { + actor { + login + } + label { + name + } } - } - ... on LabeledEvent { - actor { - login + ... on BaseRefChangedEvent { + actor { + login + } } - label { - name + ... on HeadRefForcePushedEvent { + actor { + login + } + beforeCommit { + oid + } + afterCommit { + oid + } } - } - ... on UnlabeledEvent { - actor { - login - } - label { - name + ... on RenamedTitleEvent { + actor { + login + } + previousTitle + currentTitle } - } - ... on BaseRefChangedEvent { - actor { - login - } - } - ... on HeadRefForcePushedEvent { - actor { - login - } - beforeCommit { - oid - } - afterCommit { - oid - } - } - ... on RenamedTitleEvent { - actor { - login - } - previousTitle - currentTitle - } - ... on MentionedEvent { - actor { - login + ... on MentionedEvent { + actor { + login + } } } } } } } + rateLimit { + limit + cost + remaining + resetAt + } } """ @@ -126,7 +135,8 @@ def initial_title(self): class PullRequestEvent: - def __init__(self, data): + def __init__(self, cursor, data): + self.cursor = cursor self.data = data def __getitem__(self, key): @@ -262,7 +272,7 @@ def all(access_token, owner, repo, pull): pull_request = r['data']['repository']['pullRequest'] page_info = pull_request['timelineItems']['pageInfo'] - events = pull_request['timelineItems']['nodes'] + events = pull_request['timelineItems']['edges'] result.title = pull_request['title'] result.author = pull_request['author']['login'] @@ -270,7 +280,9 @@ def all(access_token, owner, repo, pull): result.head_sha = pull_request['headRefOid'] result.mergeable = pull_request['mergeable'] - result.events.extend([PullRequestEvent(e) for e in events]) + result.events.extend([PullRequestEvent(e['cursor'], e['node']) + for e + in events]) if not page_info['hasNextPage']: break diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index df23dc9..9987909 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -7,6 +7,8 @@ from homu.pull_req_state import ( PullReqState, + ApprovalState, + BuildState, # ProcessEventResult, ) from homu.pull_request_events import ( @@ -54,8 +56,14 @@ def assert_comment(pattern, comments): return False +global_cursor = 1 + + def create_event(event): - return PullRequestEvent(event) + global global_cursor + global_cursor += 1 + cursor = "{0:010d}".format(global_cursor) + return PullRequestEvent(cursor, event) def test_baseline(): @@ -65,6 +73,9 @@ def test_baseline(): state = new_state() assert state.get_status() == '' + assert state.approval_state == ApprovalState.UNAPPROVED + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.NONE def test_current_sha(): @@ -127,6 +138,7 @@ def test_approved(_): assert assert_comment(r'Commit abcdef has been approved', result.comments) assert state.approved_by == 'ferris' assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED # Approval by someone else state = new_state(head_sha='abcdef') @@ -143,6 +155,7 @@ def test_approved(_): assert assert_comment(r'Commit abcdef has been approved', result.comments) assert state.approved_by == 'someone' assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED # Approval with commit sha state = new_state(head_sha='abcdef') @@ -158,6 +171,7 @@ def test_approved(_): assert result.changed is True assert assert_comment(r'Commit abcdef has been approved', result.comments) assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED # Approval with commit sha by bors state = new_state(head_sha='abcdef') @@ -179,6 +193,7 @@ def test_approved(_): print(render_comment(comment)) assert len(result.comments) == 0 assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED # Approval of WIP state = new_state(head_sha='abcdef', title="[WIP] A change") @@ -194,6 +209,7 @@ def test_approved(_): assert result.changed is False assert assert_comment(r'still in progress', result.comments) assert state.get_status() == '' + assert state.approval_state == ApprovalState.UNAPPROVED # Approval with invalid commit sha state = new_state(head_sha='abcdef') @@ -210,6 +226,7 @@ def test_approved(_): assert assert_comment(r'`012345` is not a valid commit SHA', result.comments) assert state.get_status() == '' + assert state.approval_state == ApprovalState.UNAPPROVED # Approval of already approved state state = new_state(head_sha='abcdef') @@ -232,10 +249,12 @@ def test_approved(_): result1 = state.process_event(event1) assert result1.changed is True assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED result2 = state.process_event(event2) assert result2.changed is False assert assert_comment(r'already approved', result2.comments) assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -259,6 +278,7 @@ def test_homu_state_approval(_): assert len(result.comments) == 0 assert state.get_status() == 'approved' assert state.approved_by == 'ferris' + assert state.approval_state == ApprovalState.APPROVED # Nobody but bors can use homu state state = new_state(head_sha='abcdef') @@ -279,6 +299,7 @@ def test_homu_state_approval(_): assert len(result.comments) == 0 assert state.get_status() == '' assert state.approved_by == '' + assert state.approval_state == ApprovalState.UNAPPROVED @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -304,6 +325,8 @@ def test_tried(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -320,6 +343,8 @@ def test_tried(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -345,6 +370,8 @@ def test_try_failed(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -361,6 +388,8 @@ def test_try_failed(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'failure' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.FAILURE @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -387,6 +416,7 @@ def test_try_reset_by_push(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'pending' + assert state.try_state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -403,6 +433,7 @@ def test_try_reset_by_push(_): assert result.changed is True assert state.try_ is True assert state.get_status() == 'success' + assert state.try_state == BuildState.SUCCESS result = state.process_event(create_event({ 'eventType': 'PullRequestCommit', @@ -414,6 +445,7 @@ def test_try_reset_by_push(_): assert result.changed is True assert state.try_ is False assert state.get_status() == '' + assert state.try_state == BuildState.NONE @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -441,6 +473,8 @@ def test_build(_): assert result.changed is True assert state.try_ is False assert state.get_status() == 'pending' + assert state.build_state == BuildState.PENDING + assert state.try_state == BuildState.NONE result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -457,6 +491,8 @@ def test_build(_): assert result.changed is True assert state.try_ is False assert state.get_status() == 'completed' + assert state.build_state == BuildState.SUCCESS + assert state.try_state == BuildState.NONE @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -482,6 +518,8 @@ def test_build_failed(_): assert result.changed is True assert state.try_ is False assert state.get_status() == 'pending' + assert state.build_state == BuildState.PENDING + assert state.try_state == BuildState.NONE result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -498,6 +536,8 @@ def test_build_failed(_): assert result.changed is True assert state.try_ is False assert state.get_status() == 'failure' + assert state.build_state == BuildState.FAILURE + assert state.try_state == BuildState.NONE #def test_tried_and_approved(): @@ -526,6 +566,7 @@ def test_approved_unapproved(_): 'publishedAt': '1985-04-21T00:00:00Z', })) assert state.get_status() != '' + assert state.approval_state == ApprovalState.APPROVED result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -537,6 +578,7 @@ def test_approved_unapproved(_): })) assert state.get_status() == '' assert state.approved_by == '' + assert state.approval_state == ApprovalState.UNAPPROVED assert result.changed is True assert len(result.label_events) == 1 assert result.label_events[0] == LabelEvent.REJECTED @@ -580,6 +622,7 @@ def test_approved_changed_push(_): 'body': '@bors r+', 'publishedAt': '1985-04-21T00:00:00Z', })) + assert state.approval_state == ApprovalState.APPROVED state.process_event(create_event({ 'eventType': 'HeadRefForcePushedEvent', 'actor': { @@ -595,6 +638,7 @@ def test_approved_changed_push(_): assert state.get_status() == '' assert state.head_sha == '012345' + assert state.approval_state == ApprovalState.UNAPPROVED @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -614,6 +658,7 @@ def test_approved_changed_base(_): 'body': '@bors r+', 'publishedAt': '1985-04-21T00:00:00Z', })) + assert state.approval_state == ApprovalState.APPROVED state.process_event(create_event({ 'eventType': 'BaseRefChangedEvent', 'actor': { @@ -622,6 +667,7 @@ def test_approved_changed_base(_): })) assert state.get_status() == '' + assert state.approval_state == ApprovalState.UNAPPROVED #def test_pending(): From a9234c7e162473ea3ca0f878b1c8ee371c87fac9 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Tue, 2 Jul 2019 09:38:40 -0500 Subject: [PATCH 11/17] Test that @bors retry resets the state Test that issuing a `@bors retry` command moves the state from 'pending' to '' for pending pull requests. This is frequently used as a way to yield the current build to a different pull request. --- homu/pull_req_state.py | 27 ++++++++++--------- homu/tests/test_process_event.py | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index a8b9440..44b42e2 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -675,18 +675,21 @@ def process_issue_comment(self, event, command): # delegate=state.delegate # )) # -# elif command.action == 'retry' and realtime: -# _assert_try_auth_verified() -# -# state.set_status('') -# if realtime: -# if state.try_: -# event = LabelEvent.TRY -# else: -# event = LabelEvent.APPROVED -# state.record_retry_log(command_src, body, global_cfg) -# state.change_labels(event) -# + elif command.action == 'retry': + _assert_try_auth_verified() + + self.status = '' + if self.try_: + event = LabelEvent.TRY + self.try_state = BuildState.NONE + else: + event = LabelEvent.APPROVED + self.build_state = BuildState.NONE + # TODO: re-enable the retry log! + #self.record_retry_log(command_src, body, global_cfg) + result.label_events.append(event) + result.changed = True + # elif command.action in ['try', 'untry'] and realtime: # _assert_try_auth_verified() # diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 9987909..3ed5b7b 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -540,6 +540,52 @@ def test_build_failed(_): assert state.try_state == BuildState.NONE +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_build_retry_cancels(_): + """ + Test that a pull request that has started a build and then gets a 'retry' + command cancels the build. + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Building commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'pending' + assert state.build_state == BuildState.PENDING + assert state.try_state == BuildState.NONE + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': ''' + @bors retry + ''', + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == '' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.NONE + # TODO: does issuing this retry emit label events? + + #def test_tried_and_approved(): # """ # Test that a pull request that has been approved AND tried shows up as From 2b8359683eeedd38d44516b0291a13e7195e16ca Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Tue, 2 Jul 2019 10:14:31 -0500 Subject: [PATCH 12/17] Handle build timeout comments Homu creates a message when a try or build timeout occurs. Handle this to keep the state properly updated. --- homu/pull_req_state.py | 6 +++++ homu/tests/test_process_event.py | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 44b42e2..c544053 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -880,6 +880,12 @@ def process_homu_state(self, event, command): self.status = 'failure' self.try_state = BuildState.FAILURE + elif state['type'] == 'TimedOut': + result.changed = True + self.status = 'failure' + # TODO: Do we need to determine if a try or a build failed? + self.try_state = BuildState.FAILURE + return result @property diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 3ed5b7b..5c0deb4 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -392,6 +392,51 @@ def test_try_failed(_): assert state.try_state == BuildState.FAILURE +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_try_timed_out(_): + """ + Test that a pull request that has been tried shows up as tried + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :boom: Test timed out + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'failure' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.FAILURE + + @unittest.mock.patch('homu.pull_req_state.assert_authorized', side_effect=return_true) def test_try_reset_by_push(_): From 3b2562ffa5735e2d94b4146ef3022120f6cd677c Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Mon, 8 Jul 2019 11:35:16 -0500 Subject: [PATCH 13/17] Add GitHubPullRequestState --- homu/pull_req_state.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index c544053..cc747fc 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -61,6 +61,12 @@ class ApprovalState(Enum): APPROVED = 'approved' +class GitHubPullRequestState(Enum): + CLOSED = 'closed' + MERGED = 'merged' + OPEN = 'open' + + class PullReqState: num = 0 priority = 0 @@ -74,6 +80,7 @@ class PullReqState: last_github_cursor = None build_state = BuildState.NONE try_state = BuildState.NONE + github_pr_state = GitHubPullRequestState.OPEN def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, gh, owner, name, label_events, repos): @@ -435,23 +442,20 @@ def process_event(self, event): pass elif event.event_type == 'MergedEvent': - # TODO: Something here. - #result.changed = self.github_state != 'merged' - #self.github_state = 'merged' - pass + # TODO: Test. + changed = self.github_pr_state != GitHubPullRequestState.MERGED + self.github_pr_state = GitHubPullRequestState.MERGED elif event.event_type == 'ClosedEvent': - # TODO: Something here. - #if self.github_state != 'merged': - # changed = self.github_state != 'closed' - # self.github_state = 'closed' - pass + # TODO: Test. + if self.github_pr_state != GitHubPullRequestState.MERGED: + changed = self.github_pr_state != GitHubPullRequestState.CLOSED + self.github_pr_state = GitHubPullRequestState.CLOSED elif event.event_type == 'ReopenedEvent': - # TODO: Something here. - #changed = self.github_state != 'open' - #self.github_state = 'open' - pass + # TODO: Test. + changed = self.github_pr_state != GitHubPullRequestState.OPEN + self.github_pr_state = GitHubPullRequestState.OPEN elif event.event_type in [ 'SubscribedEvent', From c2b343ac7257c121457c49b0a57338acb86c8fa6 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Tue, 30 Jul 2019 11:06:50 -0500 Subject: [PATCH 14/17] Pass around less repository state! Add more of the state to the Repository class, and make each PullReqState reference it's Repository and get information from there. --- homu/main.py | 133 ++----------------------------- homu/pull_req_state.py | 51 ++++++------ homu/repository.py | 48 +++++++++++ homu/server.py | 18 ++--- homu/tests/test_process_event.py | 15 ++-- 5 files changed, 96 insertions(+), 169 deletions(-) create mode 100644 homu/repository.py diff --git a/homu/main.py b/homu/main.py index 8b81163..2eaa000 100644 --- a/homu/main.py +++ b/homu/main.py @@ -32,6 +32,7 @@ from .git_helper import SSH_KEY_FILE import shlex import random +from .repository import Repository from .pull_req_state import PullReqState from .pull_request_events import all as all_pull_request_events @@ -71,49 +72,6 @@ def fetchall(self, *args): return self.db.fetchall(*args) -class Repository: - treeclosed = -1 - treeclosed_src = None - gh = None - label = None - db = None - - def __init__(self, gh, repo_label, db): - self.gh = gh - self.repo_label = repo_label - self.db = db - db.execute( - 'SELECT treeclosed, treeclosed_src FROM repos WHERE repo = ?', - [repo_label] - ) - row = db.fetchone() - if row: - self.treeclosed = row[0] - self.treeclosed_src = row[1] - else: - self.treeclosed = -1 - self.treeclosed_src = None - - def update_treeclosed(self, value, src): - self.treeclosed = value - self.treeclosed_src = src - self.db.execute( - 'DELETE FROM repos where repo = ?', - [self.repo_label] - ) - if value > 0: - self.db.execute( - ''' - INSERT INTO repos (repo, treeclosed, treeclosed_src) - VALUES (?, ?, ?) - ''', - [self.repo_label, value, src] - ) - - def __lt__(self, other): - return self.gh < other.gh - - def sha_cmp(short, full): return len(short) >= 4 and short == full[:len(short)] @@ -1148,11 +1106,10 @@ def fetch_mergeability(mergeable_que): mergeable_que.task_done() -def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_que, my_username, repo_labels): # noqa +def synchronize(repository, logger, gh, states, db, mergeable_que, my_username, repo_labels): # noqa + repo_label = repository.repo_label logger.info('Synchronizing {}...'.format(repo_label)) - repo = gh.repository(repo_cfg['owner'], repo_cfg['name']) - db.execute('DELETE FROM pull WHERE repo = ?', [repo_label]) db.execute('DELETE FROM build_res WHERE repo = ?', [repo_label]) db.execute('DELETE FROM mergeable WHERE repo = ?', [repo_label]) @@ -1165,7 +1122,6 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q } states[repo_label] = {} - repos[repo_label] = Repository(repo, repo_label, db) print("Getting pulls...") for pull in repo.iter_pulls(state='open'): @@ -1188,15 +1144,15 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q # print("Skipping {} because GraphQL never returns a success!".format(pull.number)) # continue - print("{}/{}#{}".format(repo_cfg['owner'], repo_cfg['name'], pull.number)) + print("{}/{}#{}".format(repository.owner, repository.name, pull.number)) access_token = global_cfg['github']['access_token'] try: - response = all_pull_request_events(access_token, repo_cfg['owner'], repo_cfg['name'], pull.number) + response = all_pull_request_events(access_token, repository.owner, repository.name, pull.number) except: continue status = '' - state = PullReqState(pull.number, pull.head.sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa + state = PullReqState(repository, pull.number, pull.head.sha, status, db, mergeable_que, pull.user.login) state.cfg = repo_cfg state.title = response.initial_title state.body = pull.body @@ -1395,81 +1351,8 @@ def main(): repo_labels[repo_cfg['owner'], repo_cfg['name']] = repo_label repo_states = {} - repos[repo_label] = Repository(None, repo_label, db) - - db.execute( - 'SELECT num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha FROM pull WHERE repo = ?', # noqa - [repo_label]) - for num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha in db.fetchall(): # noqa - state = PullReqState(num, head_sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa - state.title = title - state.body = body - state.head_ref = head_ref - state.base_ref = base_ref - state.assignee = assignee - - state.approved_by = approved_by - state.priority = int(priority) - state.try_ = bool(try_) - state.rollup = rollup - state.delegate = delegate - builders = [] - if merge_sha: - if 'buildbot' in repo_cfg: - builders += repo_cfg['buildbot']['builders'] - if 'travis' in repo_cfg: - builders += ['travis'] - if 'status' in repo_cfg: - builders += ['status-' + key for key, value in repo_cfg['status'].items() if 'context' in value] # noqa - if 'checks' in repo_cfg: - builders += ['checks-' + key for key, value in repo_cfg['checks'].items() if 'name' in value] # noqa - if len(builders) == 0: - raise RuntimeError('Invalid configuration') - - state.init_build_res(builders, use_db=False) - state.merge_sha = merge_sha - - elif state.status == 'pending': - # FIXME: There might be a better solution - state.status = '' - - state.save() - - repo_states[num] = state - - states[repo_label] = repo_states - - db.execute( - 'SELECT repo, num, builder, res, url, merge_sha FROM build_res') - for repo_label, num, builder, res, url, merge_sha in db.fetchall(): - try: - state = states[repo_label][num] - if builder not in state.build_res: - raise KeyError - if state.merge_sha != merge_sha: - raise KeyError - except KeyError: - db.execute( - 'DELETE FROM build_res WHERE repo = ? AND num = ? AND builder = ?', # noqa - [repo_label, num, builder]) - continue - - state.build_res[builder] = { - 'res': bool(res) if res is not None else None, - 'url': url, - } - - db.execute('SELECT repo, num, mergeable FROM mergeable') - for repo_label, num, mergeable in db.fetchall(): - try: - state = states[repo_label][num] - except KeyError: - db.execute( - 'DELETE FROM mergeable WHERE repo = ? AND num = ?', - [repo_label, num]) - continue - - state.mergeable = bool(mergeable) if mergeable is not None else None + repository = Repository(gh, repo_label, db, repo_cfg['owner'], repo_cfg['name'], repo_cfg) + repos[repo_label] = repository db.execute('SELECT repo FROM pull GROUP BY repo') for repo_label, in db.fetchall(): diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index cc747fc..1c1b398 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -82,23 +82,38 @@ class PullReqState: try_state = BuildState.NONE github_pr_state = GitHubPullRequestState.OPEN - def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, - gh, owner, name, label_events, repos): + def __init__(self, repository, num, head_sha, status, db, mergeable_que, author): self.head_advanced('', use_db=False) + self.repository = repository self.num = num self.head_sha = head_sha self.status = status self.db = db - self.repo_label = repo_label self.mergeable_que = mergeable_que - self.gh = gh - self.owner = owner - self.name = name - self.repos = repos + self.author = author self.timeout_timer = None self.test_started = time.time() - self.label_events = label_events + + @property + def repo_label(self): + return self.repository.repo_label + + @property + def owner(self): + return self.repository.owner + + @property + def name(self): + return self.repository.name + + @property + def gh(self): + return self.repository.gh + + @property + def label_events(self): + return self.repository.cfg.get('labels', {}) def head_advanced(self, head_sha, *, use_db=True): self.head_sha = head_sha @@ -255,14 +270,7 @@ def build_res_summary(self): for builder, data in self.build_res.items()) def get_repo(self): - repo = self.repos[self.repo_label].gh - if not repo: - repo = self.gh.repository(self.owner, self.name) - self.repos[self.repo_label].gh = repo - - assert repo.owner.login == self.owner - assert repo.name == self.name - return repo + return self.repository.github_repo def save(self): self.db.execute( @@ -308,10 +316,10 @@ def fake_merge(self, repo_cfg): issue.edit(title=title) def change_treeclosed(self, value, src): - self.repos[self.repo_label].update_treeclosed(value, src) + self.repository.update_treeclosed(value, src) def blocked_by_closed_tree(self): - treeclosed = self.repos[self.repo_label].treeclosed + treeclosed = self.repository.treeclosed return treeclosed if self.priority < treeclosed else None def start_testing(self, timeout): @@ -891,10 +899,3 @@ def process_homu_state(self, event, command): self.try_state = BuildState.FAILURE return result - - @property - def author(self): - """ - Get the GitHub login name of the author of the pull request - """ - return self.get_issue().user.login diff --git a/homu/repository.py b/homu/repository.py new file mode 100644 index 0000000..bdebdc0 --- /dev/null +++ b/homu/repository.py @@ -0,0 +1,48 @@ +class Repository: + treeclosed = -1 + treeclosed_src = None + gh = None + label = None + db = None + + def __init__(self, gh, repo_label, db, owner, name, cfg): + self.gh = gh + self.github_repo = gh.repository(owner, name) + self.repo_label = repo_label + self.db = db + self.owner = owner + self.name = name + self.cfg = cfg + db.execute( + 'SELECT treeclosed, treeclosed_src FROM repos WHERE repo = ?', + [repo_label] + ) + row = db.fetchone() + if row: + self.treeclosed = row[0] + self.treeclosed_src = row[1] + else: + self.treeclosed = -1 + self.treeclosed_src = None + + def update_treeclosed(self, value, src): + self.treeclosed = value + self.treeclosed_src = src + self.db.execute( + 'DELETE FROM repos where repo = ?', + [self.repo_label] + ) + if value > 0: + self.db.execute( + ''' + INSERT INTO repos (repo, treeclosed, treeclosed_src) + VALUES (?, ?, ?) + ''', + [self.repo_label, value, src] + ) + + def __lt__(self, other): + if self.owner == other.owner: + return self.name < other.name + + return self.owner < other.owner diff --git a/homu/server.py b/homu/server.py index 7e53e4a..f8b45c6 100644 --- a/homu/server.py +++ b/homu/server.py @@ -358,6 +358,7 @@ def github(): owner = owner_info.get('login') or owner_info['name'] repo_label = g.repo_labels[owner, info['repository']['name']] repo_cfg = g.repo_cfgs[repo_label] + repository = g.repos[repo_label] hmac_method, hmac_sig = request.headers['X-Hub-Signature'].split('=') if hmac_sig != hmac.new( @@ -413,12 +414,7 @@ def github(): state.save() elif action in ['opened', 'reopened']: - state = PullReqState(pull_num, head_sha, '', g.db, repo_label, - g.mergeable_que, g.gh, - info['repository']['owner']['login'], - info['repository']['name'], - repo_cfg.get('labels', {}), - g.repos) + state = PullReqState(repository, pull_num, head_sha, '', g.db, g.mergeable_que, info['pull_request']['user']['login']) state.title = info['pull_request']['title'] state.body = info['pull_request']['body'] state.head_ref = info['pull_request']['head']['repo']['owner']['login'] + ':' + info['pull_request']['head']['ref'] # noqa @@ -856,7 +852,7 @@ def synch(user_gh, state, repo_label, repo_cfg, repo): abort(400, 'Homu does not have write access on the repository') raise e - Thread(target=synchronize, args=[repo_label, repo_cfg, g.logger, + Thread(target=synchronize, args=[repository, g.logger, g.gh, g.states, g.repos, g.db, g.mergeable_que, g.my_username, g.repo_labels]).start() @@ -866,9 +862,9 @@ def synch(user_gh, state, repo_label, repo_cfg, repo): def synch_all(): @retry(wait_exponential_multiplier=1000, wait_exponential_max=600000) - def sync_repo(repo_label, g): + def sync_repo(repository, g): try: - synchronize(repo_label, g.repo_cfgs[repo_label], g.logger, g.gh, + synchronize(repository, g.logger, g.gh, g.states, g.repos, g.db, g.mergeable_que, g.my_username, g.repo_labels) except Exception: @@ -876,8 +872,8 @@ def sync_repo(repo_label, g): traceback.print_exc() raise - for repo_label in g.repos: - sync_repo(repo_label, g) + for repo_label, repository in g.repos: + sync_repo(repository, g) print('* Done synchronizing all') diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 5c0deb4..1785e8a 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -19,21 +19,20 @@ def new_state(num=1, head_sha='abcdef', status='', title='A change'): repo = unittest.mock.Mock() repo.treeclosed = False + repo.repo_label = 'test' + repo.owner = 'test-org' + repo.name = 'test-repo' + repo.gh = None + repo.github_repo = None state = PullReqState( + repository=repo, num=num, head_sha=head_sha, status=status, db=None, - repo_label='test', mergeable_que=None, - gh=None, - owner='test-org', - name='test-repo', - label_events=[], - repos={ - 'test': repo, - }) + author='ferris') state.title = title state.cfg = {} From 678a1956b8d9a32de71273654073c98475a788e8 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Tue, 30 Jul 2019 13:18:36 -0500 Subject: [PATCH 15/17] Switch to github_v4.py --- homu/github_v4.py | 451 ++++++++++++++++++++++++++++++++++++ homu/main.py | 13 +- homu/pull_request_events.py | 312 ------------------------- 3 files changed, 461 insertions(+), 315 deletions(-) create mode 100644 homu/github_v4.py delete mode 100644 homu/pull_request_events.py diff --git a/homu/github_v4.py b/homu/github_v4.py new file mode 100644 index 0000000..5ce2568 --- /dev/null +++ b/homu/github_v4.py @@ -0,0 +1,451 @@ +import requests +import time + +PULL_REQUESTS_QUERY = """ +query ($repoName: String!, $repoOwner: String!, $after: String) { + repository(name: $repoName, owner: $repoOwner) { + pullRequests(first: 100, after: $after, states: OPEN) { + pageInfo { + hasNextPage + endCursor + } + nodes { + author { + login + } + number + title + state + baseRefName + headRepositoryOwner { + login + } + body + headRefName + headRefOid + mergeable + timelineItems(last: 1) { + pageInfo { + endCursor + } + } + } + } + } + rateLimit { + limit + cost + remaining + resetAt + } +} +""" + +PULL_REQUEST_QUERY = """ +query ($repoName: String!, $repoOwner: String!, $pull: Int!, $after: String) { + repository(name: $repoName, owner: $repoOwner) { + pullRequest(number: $pull) { + author { + login + } + title + state + headRefOid + mergeable + timelineItems(first: 100, after: $after) { + pageInfo { + hasNextPage + endCursor + } + edges { + cursor + node { + eventType: __typename + ... on PullRequestCommit { + commit { + oid + } + } + ... on AssignedEvent { + actor { + login + } + user { + login + } + } + ... on UnassignedEvent { + actor { + login + } + user { + login + } + } + ... on IssueComment { + author { + login + } + body + publishedAt + } + ... on SubscribedEvent { + actor { + login + } + } + ... on LabeledEvent { + actor { + login + } + label { + name + } + } + ... on UnlabeledEvent { + actor { + login + } + label { + name + } + } + ... on BaseRefChangedEvent { + actor { + login + } + } + ... on HeadRefForcePushedEvent { + actor { + login + } + beforeCommit { + oid + } + afterCommit { + oid + } + } + ... on RenamedTitleEvent { + actor { + login + } + previousTitle + currentTitle + } + ... on MentionedEvent { + actor { + login + } + } + } + } + } + } + } + rateLimit { + limit + cost + remaining + resetAt + } +} +""" + + +class PullRequestsItem: + def __init__(self, data): + self.number = data['number'] + self.body = data['body'] + self.author = data['author']['login'] + self.head_ref = "{}:{}".format(data['headRepositoryOwner']['login'], data['headRefName']) # noqa + self.head_sha = data['headRefOid'] + self.base_ref = data['baseRefName'] + self.timeline_cursor = data['timelineItems']['pageInfo']['endCursor'] + + +class PullRequestResponse: + def __init__(self): + self.events = [] + + @property + def initial_title(self): + if not hasattr(self, '_initial_title'): + for event in self.events: + if event.event_type == 'RenamedTitleEvent': + self._initial_title = event.data['previousTitle'] + break + + # The title never changed. That means that the initial title is + # the same as the current title. + if not hasattr(self, '_initial_title'): + self._initial_title = self.title + + return self._initial_title + + +class PullRequestEvent: + def __init__(self, cursor, data): + self.cursor = cursor + self.data = data + + def __getitem__(self, key): + return self.data[key] + + @property + def event_type(self): + return self.data['eventType'] + + @staticmethod + def _actor(s): + return "\x1b[1m@" + s + "\x1b[0m" + + @staticmethod + def _label(s): + return "\x1b[100m" + s + "\x1b[0m" + + @staticmethod + def _commit(s): + return "\x1b[93m" + s[0:7] + "\x1b[0m" + + @staticmethod + def _comment_summary(comment): + # line_1 = comment.splitlines()[0] + # if len(line_1) > 40: + # return line_1[0:37] + '...' + # else: + # return line_1 + return '\n'.join([' \x1b[90m> \x1b[37m' + c + '\x1b[0m' + for c + in comment.splitlines()]) + + def format(self): + d = { + 'IssueComment': lambda e: + "{} left a comment:\n{}".format( + self._actor(e['author']['login']), + self._comment_summary(e['body'])), + 'SubscribedEvent': lambda e: + # "{} was subscribed".format( + # self._actor(e['actor']['login'])), + None, + 'MentionedEvent': lambda e: + # "{} was mentioned".format( + # self._actor(e['actor']['login'])), + None, + 'RenamedTitleEvent': lambda e: + "Renamed from '{}' to '{}' by {}".format( + e['previousTitle'], + e['currentTitle'], + self._actor(e['actor']['login'])), + 'LabeledEvent': lambda e: + "Label {} added by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'UnlabeledEvent': lambda e: + "Label {} removed by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'ReferencedEvent': lambda e: + # "Referenced", + None, + 'HeadRefForcePushedEvent': lambda e: + "{} force-pushed from {} to {}".format( + self._actor(e['actor']['login']), + self._commit(e['beforeCommit']['oid']), + self._commit(e['afterCommit']['oid'])), + 'AssignedEvent': lambda e: + "Assigned to {} by {}".format( + self._actor(e['user']['login']), + self._actor(e['actor']['login'])), + 'CrossReferencedEvent': lambda e: + # "Cross referenced", + None, + 'PullRequestReview': lambda e: + "Reviewed", + 'PullRequestCommit': lambda e: + "New commit {} pushed".format( + self._commit(self.data['commit']['oid'])), + 'MergedEvent': lambda e: + "Merged!", + 'ClosedEvent': lambda e: + "Closed.", + 'ReopenedEvent': lambda e: + "Reopened.", + } + + if self.event_type in d: + r = d[self.event_type](self) + if r: + return r + else: + return None + else: + return None + + +class GitHubV4: + def __init__(self, access_token): + self.access_token = access_token + + def _update_rate_limit(self, rate_limit): + self.rate_limit = rate_limit['limit'] + self.rate_remaining = rate_limit['remaining'] + self.rate_reset = rate_limit['resetAt'] + + def pull_requests(self, owner, repo, after=None): + results = [] + + attempt = 1 + + while True: + response = self._pull_requests_one( + owner=owner, + repo=repo, + after=after) + if response.status_code == 502: + # 502s happen sometimes when talking to GitHub. Try again. + time.sleep(1) + continue + + r = response.json() + + if 'errors' in r: + if attempt == 10: + raise Exception("Too many errors") + attempt += 1 +# print("GraphQL query failed:") +# for error in r['errors']: +# print(" * {}".format(error['message'])) + time.sleep(1) + continue + + if 'data' not in r: + print("response.status_code = {}".format(response.status_code)) + print("r = {}".format(r)) + + rate_limit = r['data']['rateLimit'] + self._update_rate_limit(rate_limit) + + page_info = r['data']['repository']['pullRequests']['pageInfo'] + pull_requests = r['data']['repository']['pullRequests']['nodes'] + + results.extend([PullRequestsItem(e) + for e + in pull_requests]) + +# print("Page info: hasNextPage={0} endCursor={1}" +# .format( +# page_info['hasNextPage'], +# page_info['endCursor'])) + if not page_info['hasNextPage']: + break + after = page_info['endCursor'] + + return results + + def _pull_requests_one(self, owner, repo, after): + headers = { + 'authorization': 'bearer ' + self.access_token, + 'accept': 'application/json', + } + json = { + 'query': PULL_REQUESTS_QUERY, + 'variables': { + 'repoName': repo, + 'repoOwner': owner, + 'after': after, + } + } + result = requests.post('https://api.github.com/graphql', + headers=headers, + json=json) + + return result + + def pull_request(self, owner, repo, pull, after=None): + result = PullRequestResponse() + result.owner = owner + result.repo = repo + result.pull = pull + + attempt = 1 + + while True: + response = self._pull_request_one( + owner=owner, + repo=repo, + pull=pull, + after=after) + if response.status_code == 502: + # 502s happen sometimes when talking to GitHub. Try again. + time.sleep(1) + continue + + r = response.json() + + if 'errors' in r: + if attempt == 3: + raise Exception("Too many errors") + attempt += 1 +# print("GraphQL query failed:") +# for error in r['errors']: +# print(" * {}".format(error['message'])) + time.sleep(1) + continue + + if 'data' not in r: + print("response.status_code = {}".format(response.status_code)) + print("r = {}".format(r)) + + rate_limit = r['data']['rateLimit'] + self._update_rate_limit(rate_limit) +# print("Rate limit: limit={0} cost={1} remaining={2} resetAt={3}" +# .format( +# rate_limit['limit'], +# rate_limit['cost'], +# rate_limit['remaining'], +# rate_limit['resetAt'])) + pull_request = r['data']['repository']['pullRequest'] + page_info = pull_request['timelineItems']['pageInfo'] + events = pull_request['timelineItems']['edges'] + + result.title = pull_request['title'] + result.author = pull_request['author']['login'] + result.state = pull_request['state'] + result.head_sha = pull_request['headRefOid'] + result.mergeable = pull_request['mergeable'] + + result.events.extend([PullRequestEvent(e['cursor'], e['node']) + for e + in events]) + +# print("Page info: hasNextPage={0} endCursor={1}" +# .format( +# page_info['hasNextPage'], +# page_info['endCursor'])) + if not page_info['hasNextPage']: + break + after = page_info['endCursor'] + + return result + + def _pull_request_one(self, owner, repo, pull, after): + headers = { + 'authorization': 'bearer ' + self.access_token, + 'accept': 'application/json', + } + json = { + 'query': PULL_REQUEST_QUERY, + 'variables': { + 'repoName': repo, + 'repoOwner': owner, + 'pull': int(pull), + 'after': after, + } + } + result = requests.post('https://api.github.com/graphql', + headers=headers, + json=json) + + return result diff --git a/homu/main.py b/homu/main.py index 2eaa000..f3d729d 100644 --- a/homu/main.py +++ b/homu/main.py @@ -34,7 +34,7 @@ import random from .repository import Repository from .pull_req_state import PullReqState -from .pull_request_events import all as all_pull_request_events +from .github_v4 import GitHubV4 global_cfg = {} @@ -1027,6 +1027,8 @@ def start_build_or_rebuild(state, repo_cfgs, *args): def process_queue(states, repos, repo_cfgs, logger, buildbot_slots, db, git_cfg): for repo_label, repo in repos.items(): + if repo_label not in states: + states[repo_label] = {} repo_states = sorted(states[repo_label].values()) for state in repo_states: @@ -1110,6 +1112,8 @@ def synchronize(repository, logger, gh, states, db, mergeable_que, my_username, repo_label = repository.repo_label logger.info('Synchronizing {}...'.format(repo_label)) + ghv4 = gh.v4 + db.execute('DELETE FROM pull WHERE repo = ?', [repo_label]) db.execute('DELETE FROM build_res WHERE repo = ?', [repo_label]) db.execute('DELETE FROM mergeable WHERE repo = ?', [repo_label]) @@ -1123,6 +1127,8 @@ def synchronize(repository, logger, gh, states, db, mergeable_que, my_username, states[repo_label] = {} + repository = repos[repo_label] + print("Getting pulls...") for pull in repo.iter_pulls(state='open'): # db.execute( @@ -1145,9 +1151,9 @@ def synchronize(repository, logger, gh, states, db, mergeable_que, my_username, # continue print("{}/{}#{}".format(repository.owner, repository.name, pull.number)) - access_token = global_cfg['github']['access_token'] + #access_token = global_cfg['github']['access_token'] try: - response = all_pull_request_events(access_token, repository.owner, repository.name, pull.number) + response = ghv4.pull_request(repository.owner, repository.name, pull.number) except: continue status = '' @@ -1255,6 +1261,7 @@ def main(): global_cfg = cfg gh = github3.login(token=cfg['github']['access_token']) + gh.v4 = GitHubV4(cfg['github']['access_token']) user = gh.user() cfg_git = cfg.get('git', {}) user_email = cfg_git.get('email') diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py deleted file mode 100644 index 3e6f065..0000000 --- a/homu/pull_request_events.py +++ /dev/null @@ -1,312 +0,0 @@ -import requests -import time - - -QUERY = """ -query ($repoName: String!, $repoOwner: String!, $pull: Int!, $after: String) { - repository(name: $repoName, owner: $repoOwner) { - pullRequest(number: $pull) { - author { - login - } - title - state - headRefOid - mergeable - timelineItems(first: 100, after: $after) { - pageInfo { - hasNextPage - endCursor - } - edges { - cursor - node { - eventType: __typename - ... on PullRequestCommit { - commit { - oid - } - } - ... on AssignedEvent { - actor { - login - } - user { - login - } - } - ... on UnassignedEvent { - actor { - login - } - user { - login - } - } - ... on IssueComment { - author { - login - } - body - publishedAt - } - ... on SubscribedEvent { - actor { - login - } - } - ... on LabeledEvent { - actor { - login - } - label { - name - } - } - ... on UnlabeledEvent { - actor { - login - } - label { - name - } - } - ... on BaseRefChangedEvent { - actor { - login - } - } - ... on HeadRefForcePushedEvent { - actor { - login - } - beforeCommit { - oid - } - afterCommit { - oid - } - } - ... on RenamedTitleEvent { - actor { - login - } - previousTitle - currentTitle - } - ... on MentionedEvent { - actor { - login - } - } - } - } - } - } - } - rateLimit { - limit - cost - remaining - resetAt - } -} -""" - - -class PullRequestResponse: - def __init__(self): - self.events = [] - - @property - def initial_title(self): - if not hasattr(self, '_initial_title'): - for event in self.events: - if event.event_type == 'RenamedTitleEvent': - self._initial_title = event.data['previousTitle'] - break - - # The title never changed. That means that the initial title is - # the same as the current title. - if not hasattr(self, '_initial_title'): - self._initial_title = self.title - - return self._initial_title - - -class PullRequestEvent: - def __init__(self, cursor, data): - self.cursor = cursor - self.data = data - - def __getitem__(self, key): - return self.data[key] - - @property - def event_type(self): - return self.data['eventType'] - - @staticmethod - def _actor(s): - return "\x1b[1m@" + s + "\x1b[0m" - - @staticmethod - def _label(s): - return "\x1b[100m" + s + "\x1b[0m" - - @staticmethod - def _commit(s): - return "\x1b[93m" + s[0:7] + "\x1b[0m" - - @staticmethod - def _comment_summary(comment): - # line_1 = comment.splitlines()[0] - # if len(line_1) > 40: - # return line_1[0:37] + '...' - # else: - # return line_1 - return '\n'.join([' \x1b[90m> \x1b[37m' + c + '\x1b[0m' - for c - in comment.splitlines()]) - - def format(self): - d = { - 'IssueComment': lambda e: - "{} left a comment:\n{}".format( - self._actor(e['author']['login']), - self._comment_summary(e['body'])), - 'SubscribedEvent': lambda e: - # "{} was subscribed".format( - # self._actor(e['actor']['login'])), - None, - 'MentionedEvent': lambda e: - # "{} was mentioned".format( - # self._actor(e['actor']['login'])), - None, - 'RenamedTitleEvent': lambda e: - "Renamed from '{}' to '{}' by {}".format( - e['previousTitle'], - e['currentTitle'], - self._actor(e['actor']['login'])), - 'LabeledEvent': lambda e: - "Label {} added by {}".format( - self._label(e['label']['name']), - self._actor(e['actor']['login'])), - 'UnlabeledEvent': lambda e: - "Label {} removed by {}".format( - self._label(e['label']['name']), - self._actor(e['actor']['login'])), - 'ReferencedEvent': lambda e: - # "Referenced", - None, - 'HeadRefForcePushedEvent': lambda e: - "{} force-pushed from {} to {}".format( - self._actor(e['actor']['login']), - self._commit(e['beforeCommit']['oid']), - self._commit(e['afterCommit']['oid'])), - 'AssignedEvent': lambda e: - "Assigned to {} by {}".format( - self._actor(e['user']['login']), - self._actor(e['actor']['login'])), - 'CrossReferencedEvent': lambda e: - # "Cross referenced", - None, - 'PullRequestReview': lambda e: - "Reviewed", - 'PullRequestCommit': lambda e: - "New commit {} pushed".format( - self._commit(self.data['commit']['oid'])), - 'MergedEvent': lambda e: - "Merged!", - 'ClosedEvent': lambda e: - "Closed.", - 'ReopenedEvent': lambda e: - "Reopened.", - } - - if self.event_type in d: - r = d[self.event_type](self) - if r: - return r - else: - return None - else: - return None - - -def all(access_token, owner, repo, pull): - after = None - result = PullRequestResponse() - result.owner = owner - result.repo = repo - result.pull = pull - - attempt = 1 - - while True: - response = one(access_token=access_token, - owner=owner, - repo=repo, - pull=pull, - after=after) - if response.status_code == 502: - # 502s happen sometimes when talking to GitHub. Try again. - time.sleep(1) - continue - - r = response.json() - - if 'errors' in r: - if attempt == 10: - raise Exception("Too many errors") - attempt += 1 - print("GraphQL query failed:") - for error in r['errors']: - print(" * {}".format(error['message'])) - time.sleep(1) - continue - - if 'data' not in r: - print("response.status_code = {}".format(response.status_code)) - print("r = {}".format(r)) - - pull_request = r['data']['repository']['pullRequest'] - page_info = pull_request['timelineItems']['pageInfo'] - events = pull_request['timelineItems']['edges'] - - result.title = pull_request['title'] - result.author = pull_request['author']['login'] - result.state = pull_request['state'] - result.head_sha = pull_request['headRefOid'] - result.mergeable = pull_request['mergeable'] - - result.events.extend([PullRequestEvent(e['cursor'], e['node']) - for e - in events]) - - if not page_info['hasNextPage']: - break - after = page_info['endCursor'] - - return result - - -def one(access_token, owner, repo, pull, after): - headers = { - 'authorization': 'bearer ' + access_token, - 'accept': 'application/json', - } - json = { - 'query': QUERY, - 'variables': { - 'repoName': repo, - 'repoOwner': owner, - 'pull': int(pull), - 'after': after, - } - } - result = requests.post('https://api.github.com/graphql', - headers=headers, - json=json) - - return result From 68bb03083956e4f95395caa32becf182b93ec1ca Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 16 Aug 2019 10:53:48 -0500 Subject: [PATCH 16/17] Include build history when parsing history Include a history of all of the tries and all of the builds when parsing the history of a pull request. --- homu/pull_req_state.py | 172 ++++++-- homu/tests/test_process_event.py | 17 +- homu/tests/test_process_event_multiple.py | 479 ++++++++++++++++++++++ 3 files changed, 641 insertions(+), 27 deletions(-) create mode 100644 homu/tests/test_process_event_multiple.py diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 1c1b398..3cc3d16 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -51,6 +51,10 @@ class BuildState(Enum): SUCCESS = 'success' FAILURE = 'failure' ERROR = 'error' + # The build timed out + TIMEDOUT = 'timedout' + # The build was cancelled by an action, like a retry or something else + CANCELLED = 'cancelled' class ApprovalState(Enum): @@ -67,6 +71,15 @@ class GitHubPullRequestState(Enum): OPEN = 'open' +class BuildHistoryItem: + def __init__(self, head_sha=None, merge_sha=None, state=None, started_at=None, ended_at=None): + self.head_sha = head_sha + self.merge_sha = merge_sha + self.state = state + self.started_at = started_at + self.ended_at = ended_at + + class PullReqState: num = 0 priority = 0 @@ -94,6 +107,8 @@ def __init__(self, repository, num, head_sha, status, db, mergeable_que, author) self.author = author self.timeout_timer = None self.test_started = time.time() + self.try_history = [] + self.build_history = [] @property def repo_label(self): @@ -115,6 +130,20 @@ def gh(self): def label_events(self): return self.repository.cfg.get('labels', {}) + @property + def last_build(self): + if len(self.build_history) == 0: + return None + + return self.build_history[-1] + + @property + def last_try(self): + if len(self.try_history) == 0: + return None + + return self.try_history[-1] + def head_advanced(self, head_sha, *, use_db=True): self.head_sha = head_sha self.approved_by = '' @@ -694,9 +723,15 @@ def process_issue_comment(self, event, command): if self.try_: event = LabelEvent.TRY self.try_state = BuildState.NONE + if self.last_try is not None: + self.last_try.state = BuildState.CANCELLED + # self.ended_at = else: event = LabelEvent.APPROVED self.build_state = BuildState.NONE + if self.last_build is not None: + self.last_build.state = BuildState.CANCELLED + # TODO: re-enable the retry log! #self.record_retry_log(command_src, body, global_cfg) result.label_events.append(event) @@ -832,7 +867,6 @@ def process_homu_state(self, event, command): result = ProcessEventResult() state = command.homu_state - if state['type'] == 'Approved': result.changed = self.approved_by != state['approver'] self.approved_by = state['approver'] @@ -843,59 +877,145 @@ def process_homu_state(self, event, command): self.status = 'pending' self.build_state = BuildState.PENDING + self.build_history.append( + BuildHistoryItem( + head_sha=state['head_sha'], + merge_sha=state['merge_sha'], + started_at=event['publishedAt'], + state=BuildState.PENDING + )) + elif state['type'] == 'BuildCompleted': result.changed = True self.try_ = False self.status = 'completed' self.build_state = BuildState.SUCCESS + item = next((item for item in self.build_history + if item.state == BuildState.PENDING + and item.merge_sha == state['merge_sha']), None) + + if item: + item.state = BuildState.SUCCESS + item.ended_at = event['publishedAt'] + elif state['type'] == 'BuildFailed': result.changed = True self.try_ = False self.status = 'failure' self.build_state = BuildState.FAILURE + item = None + if 'merge_sha' in state: + # Sweet! We can find it by sha and we're good. + item = next((item for item in self.build_history + if item.state == BuildState.PENDING + and item.merge_sha == state['merge_sha']), None) + else: + # merge_sha was not found, so we need to guess. We'll guess the + # last one. + if self.build_history[-1].state == BuildState.PENDING: + item = self.build_history[-1] + + if item: + item.state = BuildState.FAILURE + item.ended_at = event['publishedAt'] + elif state['type'] == 'TryBuildStarted': result.changed = True self.try_ = True self.status = 'pending' self.try_state = BuildState.PENDING - # TODO: Multiple tries? - # result.changed = True - # self.tries.append(PullRequestTry( - # len(self.tries) + 1, - # state['head_sha'], - # state['merge_sha'], - # event['publishedAt']) - # ) + self.try_history.append( + BuildHistoryItem( + head_sha=state['head_sha'], + merge_sha=state['merge_sha'], + started_at=event['publishedAt'], + state=BuildState.PENDING + )) elif state['type'] == 'TryBuildCompleted': result.changed = True self.status = 'success' self.try_state = BuildState.SUCCESS - # TODO: Multiple tries? - # item = next((try_ - # for try_ in self.tries - # if try_.state == 'pending' - # and try_.merge_sha == state['merge_sha']), - # None) - # - # if item: - # result.changed = True - # # TODO: Multiple tries? - # item.ended_at = event['publishedAt'] - # item.state = 'completed' - # item.builders = state['builders'] + + item = next((item for item in self.try_history + if item.state == BuildState.PENDING + and item.merge_sha == state['merge_sha']), None) + + if item: + item.state = BuildState.SUCCESS + item.ended_at = event['publishedAt'] elif state['type'] == 'TryBuildFailed': result.changed = True self.status = 'failure' self.try_state = BuildState.FAILURE + item = None + if 'merge_sha' in state: + # Sweet! We can find it by sha and we're good. + item = next((item for item in self.try_history + if item.state == BuildState.PENDING + and item.merge_sha == state['merge_sha']), None) + else: + # merge_sha was not found, so we need to guess. We'll guess the + # last one. + if self.try_history[-1].state == BuildState.PENDING: + item = self.try_history[-1] + + if item: + item.state = BuildState.FAILURE + item.ended_at = event['publishedAt'] + elif state['type'] == 'TimedOut': - result.changed = True - self.status = 'failure' - # TODO: Do we need to determine if a try or a build failed? - self.try_state = BuildState.FAILURE + # TimedOut doesn't tell us whether a try or a build timed out. + + build_type = None + + last_try = self.last_try + last_build = self.last_build + + # We know basically nothing. Use whatever is newer of the most + # recent build and try. + if last_try is None and last_build is None: + # What timed out? + pass + elif last_try is not None and last_build is None: + # We only have tries + if last_try.state == BuildState.PENDING: + last_try.state = BuildState.TIMEDOUT + build_type = 'try' + elif last_try is None and last_build is not None: + # We only have builds + if last_build.state == BuildState.PENDING: + last_build.state = BuildState.TIMEDOUT + build_type = 'build' + else: + if last_try.state == BuildState.PENDING and last_build.state == BuildState.PENDING: + # Both are pending!? Oh my. Whichever is newer, then. + if last_try.started_at < last_build.started_at: + last_build.state = BuildState.TIMEDOUT + build_type = 'build' + else: + last_try.state = BuildState.TIMEDOUT + build_type = 'try' + elif last_try.state == BuildState.PENDING: + last_build.state = BuildState.TIMEDOUT + build_type = 'try' + elif last_build.state == BuildState.PENDING: + last_try.state = BuildState.TIMEDOUT + build_type = 'build' + else: + pass + + if build_type == 'build': + result.changed = True + self.status = 'failure' + self.build_state = BuildState.FAILURE + elif build_type == 'try': + result.changed = True + self.status = 'failure' + self.try_state = BuildState.FAILURE return result diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 1785e8a..463d0ff 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -11,7 +11,7 @@ BuildState, # ProcessEventResult, ) -from homu.pull_request_events import ( +from homu.github_v4 import ( PullRequestEvent ) @@ -326,6 +326,7 @@ def test_tried(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.PENDING + assert state.last_try.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -344,6 +345,7 @@ def test_tried(_): assert state.get_status() == 'success' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.SUCCESS + assert state.last_try.state == BuildState.SUCCESS @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -371,6 +373,7 @@ def test_try_failed(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.PENDING + assert state.last_try.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -389,6 +392,7 @@ def test_try_failed(_): assert state.get_status() == 'failure' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.FAILURE + assert state.last_try.state == BuildState.FAILURE @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -416,6 +420,7 @@ def test_try_timed_out(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.PENDING + assert state.last_try.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -434,6 +439,7 @@ def test_try_timed_out(_): assert state.get_status() == 'failure' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.FAILURE + assert state.last_try.state == BuildState.TIMEDOUT @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -461,6 +467,7 @@ def test_try_reset_by_push(_): assert state.try_ is True assert state.get_status() == 'pending' assert state.try_state == BuildState.PENDING + assert state.last_try.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -478,6 +485,7 @@ def test_try_reset_by_push(_): assert state.try_ is True assert state.get_status() == 'success' assert state.try_state == BuildState.SUCCESS + assert state.last_try.state == BuildState.SUCCESS result = state.process_event(create_event({ 'eventType': 'PullRequestCommit', @@ -490,6 +498,7 @@ def test_try_reset_by_push(_): assert state.try_ is False assert state.get_status() == '' assert state.try_state == BuildState.NONE + assert state.last_try.state == BuildState.SUCCESS @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -519,6 +528,7 @@ def test_build(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.PENDING assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -537,6 +547,7 @@ def test_build(_): assert state.get_status() == 'completed' assert state.build_state == BuildState.SUCCESS assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.SUCCESS @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -564,6 +575,7 @@ def test_build_failed(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.PENDING assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -582,6 +594,7 @@ def test_build_failed(_): assert state.get_status() == 'failure' assert state.build_state == BuildState.FAILURE assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.FAILURE @unittest.mock.patch('homu.pull_req_state.assert_authorized', @@ -610,6 +623,7 @@ def test_build_retry_cancels(_): assert state.get_status() == 'pending' assert state.build_state == BuildState.PENDING assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.PENDING result = state.process_event(create_event({ 'eventType': 'IssueComment', @@ -627,6 +641,7 @@ def test_build_retry_cancels(_): assert state.get_status() == '' assert state.build_state == BuildState.NONE assert state.try_state == BuildState.NONE + assert state.last_build.state == BuildState.CANCELLED # TODO: does issuing this retry emit label events? diff --git a/homu/tests/test_process_event_multiple.py b/homu/tests/test_process_event_multiple.py new file mode 100644 index 0000000..d8c0b75 --- /dev/null +++ b/homu/tests/test_process_event_multiple.py @@ -0,0 +1,479 @@ +import unittest.mock +import re +from homu.comments import Comment +from homu.consts import ( + LabelEvent, +) + +from homu.pull_req_state import ( + PullReqState, + ApprovalState, + BuildState, + # ProcessEventResult, +) +from homu.github_v4 import ( + PullRequestEvent +) + + +def new_state(num=1, head_sha='abcdef', status='', title='A change'): + repo = unittest.mock.Mock() + repo.treeclosed = False + repo.repo_label = 'test' + repo.owner = 'test-org' + repo.name = 'test-repo' + repo.gh = None + repo.github_repo = None + + state = PullReqState( + repository=repo, + num=num, + head_sha=head_sha, + status=status, + db=None, + mergeable_que=None, + author='ferris') + + state.title = title + state.cfg = {} + + return state + + +def render_comment(comment): + if isinstance(comment, Comment): + return comment.render() + else: + return comment + + +def assert_comment(pattern, comments): + for comment in comments: + if re.search(pattern, render_comment(comment)) is not None: + return True + + return False + + +global_cursor = 1 + + +def create_event(event): + global global_cursor + global_cursor += 1 + cursor = "{0:010d}".format(global_cursor) + return PullRequestEvent(cursor, event) + + +def return_true(a, b, c, d, e, f): + return True + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_tried_multiple_times(_): + """ + Test that a pull request that has been tried multiple times has a history + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.SUCCESS + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:02:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.SUCCESS + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:03:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.SUCCESS + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.SUCCESS + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_tried_multiple_times_failed_then_succeeded(_): + """ + Test that a pull request that has been tried multiple times has a history + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'failure' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.FAILURE + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:02:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:03:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.SUCCESS + + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_tried_multiple_times_failed_then_succeeded(_): + """ + Test that a pull request that has been tried shows up as tried + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'failure' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.FAILURE + assert len(state.try_history) == 1 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:02:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:03:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert len(state.try_history) == 2 + assert state.try_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.try_history[0].state == BuildState.FAILURE + assert state.try_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.try_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.try_history[1].state == BuildState.SUCCESS + +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_built_multiple_times(_): + """ + Test that a pull request that has been built multiple times has a history + """ + + state = new_state() + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'pending' + assert state.build_state == BuildState.PENDING + assert state.try_state == BuildState.NONE + assert len(state.build_history) == 1 + assert state.build_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.build_history[0].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'failure' + assert state.build_state == BuildState.FAILURE + assert state.try_state == BuildState.NONE + assert len(state.build_history) == 1 + assert state.build_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.build_history[0].state == BuildState.FAILURE + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:02:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'pending' + assert state.build_state == BuildState.PENDING + assert state.try_state == BuildState.NONE + assert len(state.build_history) == 2 + assert state.build_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.build_history[0].state == BuildState.FAILURE + assert state.build_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.build_history[1].state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:03:00Z', + })) + + assert result.changed is True + assert state.try_ is False + assert state.get_status() == 'failure' + assert state.build_state == BuildState.FAILURE + assert state.try_state == BuildState.NONE + assert len(state.build_history) == 2 + assert state.build_history[0].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[0].merge_sha == "330c85d9270b32d7703ebefc337eb37ae959f741" # noqa + assert state.build_history[0].state == BuildState.FAILURE + assert state.build_history[1].head_sha == "065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe" # noqa + assert state.build_history[1].merge_sha == "dba7673010f19a94af4345453005933fd511bea9" # noqa + assert state.build_history[1].state == BuildState.FAILURE From a15e1da7555971f63e25b11b30b62e226b11a9c8 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Wed, 21 Aug 2019 11:58:22 -0500 Subject: [PATCH 17/17] Calculate `status`, `try_` and others from history Previously, we discretely set `status`, `try`, `build_state`, and `try_state` on each event. With the addition of the run histories, we can now glean all of this information from those histories instead of tracking them independently. The results appear to all be identical on the current Homu queue except for one edge case: a pull request was tried, approved, then unapproved. - Using the previous method, the status would be '', because approval changes the status from 'success (try)' to '', and unapproval doesn't change the status. - Using the current method, the status would be 'success (try)', because a successful try has occured for the relevant commit hash and the PR isn't approved. --- homu/pull_req_state.py | 193 ++++++++++++++-------- homu/tests/test_process_event.py | 105 ++++++++++-- homu/tests/test_process_event_multiple.py | 6 +- 3 files changed, 221 insertions(+), 83 deletions(-) diff --git a/homu/pull_req_state.py b/homu/pull_req_state.py index 3cc3d16..1a854fa 100644 --- a/homu/pull_req_state.py +++ b/homu/pull_req_state.py @@ -91,8 +91,6 @@ class PullReqState: assignee = '' delegate = '' last_github_cursor = None - build_state = BuildState.NONE - try_state = BuildState.NONE github_pr_state = GitHubPullRequestState.OPEN def __init__(self, repository, num, head_sha, status, db, mergeable_que, author): @@ -101,7 +99,6 @@ def __init__(self, repository, num, head_sha, status, db, mergeable_que, author) self.repository = repository self.num = num self.head_sha = head_sha - self.status = status self.db = db self.mergeable_que = mergeable_que self.author = author @@ -130,8 +127,45 @@ def gh(self): def label_events(self): return self.repository.cfg.get('labels', {}) + @property + def build_state(self): + """ + The current build state. This closely matches the `status` when `try_` is false. + """ + current_build = self.current_build + + if current_build is None: + return BuildState.NONE + + if current_build.state == BuildState.TIMEDOUT: + return BuildState.FAILURE + if current_build.state == BuildState.CANCELLED: + return BuildState.NONE + + return current_build.state + + @property + def try_state(self): + """ + The current try state. This closely matches the `status` when `try_` is true. + """ + current_try = self.current_try + + if current_try is None: + return BuildState.NONE + + if current_try.state == BuildState.TIMEDOUT: + return BuildState.FAILURE + if current_try.state == BuildState.CANCELLED: + return BuildState.NONE + + return current_try.state + @property def last_build(self): + """ + The most recent build run, or None if there have been no attempts to run a build. + """ if len(self.build_history) == 0: return None @@ -139,18 +173,103 @@ def last_build(self): @property def last_try(self): + """ + The most recent try run, or None if there have been no attempts to run a try. + """ if len(self.try_history) == 0: return None return self.try_history[-1] + @property + def current_build(self): + """ + The most recent build run that corresponds to the current head sha. + + This will either be the same as last_build, or None if last_build applied to a previous commit hash. + """ + last_build = self.last_build + if last_build is None: + return None + + if last_build.head_sha == self.head_sha: + return last_build + + return None + + @property + def current_try(self): + """ + The most recent try run that corresponds to the current head sha. + + This will either be the same as last_try, or None if last_try applied to a previous commit hash. + """ + last_try = self.last_try + if last_try is None: + return None + + if last_try.head_sha == self.head_sha: + return last_try + + return None + + @property + def status(self): + [status, try_] = self.get_current_status_and_try() + return status + + @status.setter + def status(self, value): + print("setting status on {} to '{}' (except not really)".format(self.num, value)) + + @property + def try_(self): + [status, try_] = self.get_current_status_and_try() + return try_ + + @try_.setter + def try_(self, value): + pass + + def get_current_status_and_try(self): + current_build = self.current_build + current_try = self.current_try + + if current_build is not None: + if current_build.state == BuildState.SUCCESS: + return ['completed', False] + return [self.state_to_status(current_build.state), False] + + if self.approval_state == ApprovalState.APPROVED: + return ['approved', False] + + if current_try is not None: + return [self.state_to_status(current_try.state), True] + + return ['', False] + + def state_to_status(self, build_status): + if build_status == BuildState.NONE: + return '' + if build_status == BuildState.PENDING: + return 'pending' + if build_status == BuildState.SUCCESS: + return 'success' + if build_status == BuildState.FAILURE: + return 'failure' + if build_status == BuildState.ERROR: + return 'error' + if build_status == BuildState.TIMEDOUT: + return 'failure' + if build_status == BuildState.CANCELLED: + return '' + return '' + def head_advanced(self, head_sha, *, use_db=True): self.head_sha = head_sha self.approved_by = '' - self.status = '' self.merge_sha = '' self.build_res = {} - self.try_ = False self.mergeable = None if use_db: @@ -428,15 +547,6 @@ def process_event(self, event): # New commits come in: no longer approved result.changed = result.changed or self.approved_by != '' self.approved_by = '' - result.changed = result.changed or self.try_ != False - self.try_ = False - # TODO: Do we *always* reset the state? - result.changed = result.changed or self.status != '' - self.status = '' - result.changed = result.changed or self.try_state != BuildState.NONE - self.try_state = BuildState.NONE - result.changed = result.changed or self.build_state != BuildState.NONE - self.build_state = BuildState.NONE elif event.event_type == 'HeadRefForcePushedEvent': result.changed = self.head_sha != event['afterCommit']['oid'] @@ -523,26 +633,6 @@ def process_event(self, event): return result -# def process_issue_comment(self, event, command): -# result = ProcessEventResult() -# if command.action == 'homu-state': -# return self.process_homu_state(event, command) -# -# if command.action == 'approve': -# # TODO: Something with states -# result.changed = self.approved_by != command.actor -# self.approved_by = command.actor -# -# if command.action == 'unapprove': -# # TODO: Something with states -# result.changed = self.approved_by != '' -# self.approved_by = None -# -# # if command.action == 'try': -# # changed = True -# # self.tries.append(PullRequestTry(1, self.head_sha, None)) -# return result - def process_issue_comment(self, event, command): # TODO: Don't hardcode botname botname = 'bors' @@ -634,8 +724,6 @@ def process_issue_comment(self, event, command): else: self.approved_by = approver - self.try_ = False - self.status = '' result.changed = True result.label_events.append(LabelEvent.APPROVED) @@ -719,16 +807,13 @@ def process_issue_comment(self, event, command): elif command.action == 'retry': _assert_try_auth_verified() - self.status = '' if self.try_: event = LabelEvent.TRY - self.try_state = BuildState.NONE if self.last_try is not None: self.last_try.state = BuildState.CANCELLED # self.ended_at = else: event = LabelEvent.APPROVED - self.build_state = BuildState.NONE if self.last_build is not None: self.last_build.state = BuildState.CANCELLED @@ -873,10 +958,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'BuildStarted': result.changed = True - self.try_ = False - self.status = 'pending' - self.build_state = BuildState.PENDING - self.build_history.append( BuildHistoryItem( head_sha=state['head_sha'], @@ -887,10 +968,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'BuildCompleted': result.changed = True - self.try_ = False - self.status = 'completed' - self.build_state = BuildState.SUCCESS - item = next((item for item in self.build_history if item.state == BuildState.PENDING and item.merge_sha == state['merge_sha']), None) @@ -901,10 +978,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'BuildFailed': result.changed = True - self.try_ = False - self.status = 'failure' - self.build_state = BuildState.FAILURE - item = None if 'merge_sha' in state: # Sweet! We can find it by sha and we're good. @@ -923,9 +996,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'TryBuildStarted': result.changed = True - self.try_ = True - self.status = 'pending' - self.try_state = BuildState.PENDING self.try_history.append( BuildHistoryItem( head_sha=state['head_sha'], @@ -936,9 +1006,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'TryBuildCompleted': result.changed = True - self.status = 'success' - self.try_state = BuildState.SUCCESS - item = next((item for item in self.try_history if item.state == BuildState.PENDING and item.merge_sha == state['merge_sha']), None) @@ -949,9 +1016,6 @@ def process_homu_state(self, event, command): elif state['type'] == 'TryBuildFailed': result.changed = True - self.status = 'failure' - self.try_state = BuildState.FAILURE - item = None if 'merge_sha' in state: # Sweet! We can find it by sha and we're good. @@ -1009,13 +1073,6 @@ def process_homu_state(self, event, command): else: pass - if build_type == 'build': - result.changed = True - self.status = 'failure' - self.build_state = BuildState.FAILURE - elif build_type == 'try': - result.changed = True - self.status = 'failure' - self.try_state = BuildState.FAILURE + result.changed = True return result diff --git a/homu/tests/test_process_event.py b/homu/tests/test_process_event.py index 463d0ff..da9646e 100644 --- a/homu/tests/test_process_event.py +++ b/homu/tests/test_process_event.py @@ -308,7 +308,7 @@ def test_tried(_): Test that a pull request that has been tried shows up as tried """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -355,7 +355,7 @@ def test_try_failed(_): Test that a pull request that has been tried shows up as tried """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -402,7 +402,7 @@ def test_try_timed_out(_): Test that a pull request that has been tried shows up as tried """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -450,7 +450,7 @@ def test_try_reset_by_push(_): not show up as tried """ - state = new_state() + state = new_state(head_sha="065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe") result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -510,7 +510,20 @@ def test_build(_): be merged and removed. """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.NONE + result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -557,7 +570,20 @@ def test_build_failed(_): Test that a pull request that has been built and failed shows up that way. """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.NONE + result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -605,7 +631,7 @@ def test_build_retry_cancels(_): command cancels the build. """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -645,11 +671,66 @@ def test_build_retry_cancels(_): # TODO: does issuing this retry emit label events? -#def test_tried_and_approved(): -# """ -# Test that a pull request that has been approved AND tried shows up as -# approved AND tried -# """ +@unittest.mock.patch('homu.pull_req_state.assert_authorized', + side_effect=return_true) +def test_tried_and_approved(_): + """ + Test that a pull request that has been approved AND tried shows up as + approved AND tried + """ + + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :hourglass: Trying commit 065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe with merge 330c85d9270b32d7703ebefc337eb37ae959f741... + + ''', # noqa + 'publishedAt': '1985-04-21T00:00:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'pending' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.PENDING + assert state.last_try.state == BuildState.PENDING + + result = state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'bors', + }, + 'body': ''' + :sunny: Try build successful - [checks-travis](https://travis-ci.com/rust-lang/rust/builds/115542062) Build commit: 330c85d9270b32d7703ebefc337eb37ae959f741 + + ''', # noqa + 'publishedAt': '1985-04-21T00:01:00Z', + })) + + assert result.changed is True + assert state.try_ is True + assert state.get_status() == 'success' + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert state.last_try.state == BuildState.SUCCESS + + state.process_event(create_event({ + 'eventType': 'IssueComment', + 'author': { + 'login': 'ferris', + }, + 'body': '@bors r+', + 'publishedAt': '1985-04-21T00:00:00Z', + })) + assert state.get_status() == 'approved' + assert state.approval_state == ApprovalState.APPROVED + assert state.build_state == BuildState.NONE + assert state.try_state == BuildState.SUCCESS + assert state.last_try.state == BuildState.SUCCESS @unittest.mock.patch('homu.pull_req_state.assert_authorized', diff --git a/homu/tests/test_process_event_multiple.py b/homu/tests/test_process_event_multiple.py index d8c0b75..5fa507e 100644 --- a/homu/tests/test_process_event_multiple.py +++ b/homu/tests/test_process_event_multiple.py @@ -76,7 +76,7 @@ def test_tried_multiple_times(_): Test that a pull request that has been tried multiple times has a history """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -281,7 +281,7 @@ def test_tried_multiple_times_failed_then_succeeded(_): Test that a pull request that has been tried shows up as tried """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': { @@ -383,7 +383,7 @@ def test_built_multiple_times(_): Test that a pull request that has been built multiple times has a history """ - state = new_state() + state = new_state(head_sha='065151f8b2c31d9e4ddd34aaf8d3263a997f5cfe') result = state.process_event(create_event({ 'eventType': 'IssueComment', 'author': {