diff --git a/CHANGES.rst b/CHANGES.rst index 8c19b87..2c8b30f 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,6 +3,9 @@ * New API to post checks on GitHub. [#45] +* Remove support for ``post_pr_comment`` - instead we now support exclusively + status checks. [#49] + 0.2 (2018-11-22) ---------------- diff --git a/baldrick/plugins/github_pull_requests.py b/baldrick/plugins/github_pull_requests.py index fd65291..c6e27ef 100644 --- a/baldrick/plugins/github_pull_requests.py +++ b/baldrick/plugins/github_pull_requests.py @@ -75,9 +75,6 @@ def process_pull_request(repository, number, installation): if not pr_config.get("enabled", False): return "Skipping PR checks, disabled in config." - post_comment = pr_config.get("post_pr_comment", False) - pull_request_substring = pr_config.get('pull_request_substring', '') - # Disable if the config is not present if pr_config is None: return @@ -89,21 +86,6 @@ def process_pull_request(repository, number, installation): repo_handler = RepoHandler(pr_handler.head_repo_name, pr_handler.head_branch, installation) - def is_previous_comment(message): - if len(pull_request_substring) > 0: - return pull_request_substring in message - else: - return True - - # Find previous comments by this app - comment_ids = pr_handler.find_comments(f'{current_app.bot_username}[bot]', - filter_keep=is_previous_comment) - - if len(comment_ids) == 0: - comment_id = None - else: - comment_id = comment_ids[-1] - # First check whether there are labels that indicate the checks should be # skipped @@ -112,12 +94,6 @@ def is_previous_comment(message): for label in pr_handler.labels: if label in skip_labels: - if post_comment: - skip_message = pr_config.get("skip_message", "Pull request checks have " - "been skipped as this pull request has been " - f"labelled as **{label}**") - skip_message = skip_message.format(pr_handler=pr_handler, repo_handler=repo_handler) - pr_handler.submit_comment(skip_message, comment_id=comment_id) if skip_fails: pr_handler.set_status('failure', "Skipping checks due to {0} label".format(label), current_app.bot_username) return @@ -129,81 +105,38 @@ def is_previous_comment(message): if result is not None: results.update(result) - failures = [details['description'] for details in results.values() if details['state'] in ('error', 'failure')] - - if post_comment: - - # Post all failures in a comment, and have a single status check - - if failures: - - pull_request_prologue = pr_config.get('fail_prologue', '') - pull_request_epilogue = pr_config.get('fail_epilogue', '') - - fail_status = pr_config.get('fail_status', 'Failed some checks') - - message = pull_request_prologue.format(pr_handler=pr_handler, repo_handler=repo_handler) - for failure in failures: - message += f'* {failure}\n' - message += pull_request_epilogue.format(pr_handler=pr_handler, repo_handler=repo_handler) - comment_url = pr_handler.submit_comment(message, comment_id=comment_id, return_url=True) + # Post each failure as a status - pr_handler.set_status('failure', fail_status, current_app.bot_username, target_url=comment_url) + existing_statuses = pr_handler.list_statuses() - else: + for context, details in sorted(results.items()): - pass_status = pr_config.get('pass_status', 'Passed all checks') + full_context = current_app.bot_username + ':' + context - all_passed_message = pr_config.get('all_passed_message', '') - all_passed_message = all_passed_message.format(pr_handler=pr_handler, repo_handler=repo_handler) + # NOTE: we could in principle check if the status has been posted + # before, and if so not post it again, but we had this in the past + # and there were some strange caching issues where GitHub would + # return old status messages, so we avoid doing that. - if all_passed_message: - pr_handler.submit_comment(all_passed_message, comment_id=comment_id) + pr_handler.set_status(details['state'], details['description'], + full_context, + target_url=details.get('target_url')) - pr_handler.set_status('success', pass_status, current_app.bot_username) + # For statuses that have been skipped this time but existed before, set + # status to pass and set message to say skipped - else: - - # Post each failure as a status - - existing_statuses = pr_handler.list_statuses() - - for context, details in sorted(results.items()): - - full_context = current_app.bot_username + ':' + context - - # NOTE: we could in principle check if the status has been posted - # before, and if so not post it again, but we had this in the past - # and there were some strange caching issues where GitHub would - # return old status messages, so we avoid doing that. + for full_context in existing_statuses: - pr_handler.set_status(details['state'], details['description'], - full_context, - target_url=details.get('target_url')) - - # For statuses that have been skipped this time but existed before, set - # status to pass and set message to say skipped - - for full_context in existing_statuses: - - if full_context.startswith(current_app.bot_username + ':'): - context = full_context[len(current_app.bot_username) + 1:] - if context not in results: - pr_handler.set_status('success', 'This check has been skipped', - current_app.bot_username + ':' + context) - - # Also set the general 'single' status check as a skipped check if it - # is present - if full_context == current_app.bot_username: + if full_context.startswith(current_app.bot_username + ':'): + context = full_context[len(current_app.bot_username) + 1:] + if context not in results: pr_handler.set_status('success', 'This check has been skipped', - current_app.bot_username) - - # If a comment has been posted before, and to be careful only if it is - # a comment that matches the specified substring, we edit the comment. - if len(pull_request_substring) > 0 and len(comment_ids) > 0: - for comment_id in comment_ids: - pr_handler.submit_comment(f'Check results are now reported in the ' - 'status checks at the bottom of this page.', - comment_id=comment_id) + current_app.bot_username + ':' + context) + + # Also set the general 'single' status check as a skipped check if it + # is present + if full_context == current_app.bot_username: + pr_handler.set_status('success', 'This check has been skipped', + current_app.bot_username) return 'Finished pull requests checks' diff --git a/baldrick/plugins/tests/test_github_pull_requests.py b/baldrick/plugins/tests/test_github_pull_requests.py index f918e97..82cc795 100644 --- a/baldrick/plugins/tests/test_github_pull_requests.py +++ b/baldrick/plugins/tests/test_github_pull_requests.py @@ -12,10 +12,6 @@ [ tool.testbot ] [ tool.testbot.pull_requests ] enabled = true -post_pr_comment = {post_pr_comment} -all_passed_message = '{all_passed_message}' -fail_prologue = '{fail_prologue}' -fail_epilogue = '{fail_epilogue}' """ @@ -98,42 +94,11 @@ def test_empty_default(self, app, client): # registered handlers don't return any checks test_hook.return_value = {} - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message="''", - fail_prologue='', fail_epilogue='') + self.get_file_contents.return_value = CONFIG_TEMPLATE self.send_event(client) - assert self.requests_post.call_count == 1 - - args, kwargs = self.requests_post.call_args - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json'] == {'state': 'success', - 'description': 'Passed all checks', - 'context': 'testbot'} - - def test_empty_with_message(self, app, client): - - # As above, but a default message is given - - test_hook.return_value = {} - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') - - self.send_event(client) - - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/1234/comments' - assert kwargs['json'] == {'body': 'All checks passed'} - - args, kwargs = self.requests_post.call_args_list[1] - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json'] == {'state': 'success', - 'description': 'Passed all checks', - 'context': 'testbot'} + assert self.requests_post.call_count == 0 def test_all_passed(self, app, client): @@ -142,9 +107,7 @@ def test_all_passed(self, app, client): test_hook.return_value = {'test1': {'description': 'No problem', 'state': 'success'}, 'test2': {'description': 'All good here', 'state': 'success'}} - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='false', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') + self.get_file_contents.return_value = CONFIG_TEMPLATE self.send_event(client) @@ -162,31 +125,6 @@ def test_all_passed(self, app, client): 'description': 'All good here', 'context': 'testbot:test2'} - def test_all_passed_with_comment(self, app, client): - - # As above, but a default message is given - - test_hook.return_value = {'test1': {'description': 'No problem', 'state': 'success'}, - 'test2': {'description': 'All good here', 'state': 'success'}} - - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') - - self.send_event(client) - - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/1234/comments' - assert kwargs['json'] == {'body': 'All checks passed'} - - args, kwargs = self.requests_post.call_args_list[1] - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json'] == {'state': 'success', - 'description': 'Passed all checks', - 'context': 'testbot'} - def test_one_failure(self, app, client): # As above, but a default message is given @@ -194,9 +132,7 @@ def test_one_failure(self, app, client): test_hook.return_value = {'test1': {'description': 'Problems here', 'state': 'failure'}, 'test2': {'description': 'All good here', 'state': 'success'}} - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='false', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') + self.get_file_contents.return_value = CONFIG_TEMPLATE self.send_event(client) @@ -214,57 +150,6 @@ def test_one_failure(self, app, client): 'description': 'All good here', 'context': 'testbot:test2'} - def test_one_failure_with_comment(self, app, client): - - # As above, but a default message is given - - test_hook.return_value = {'test1': {'description': 'Problems here', 'state': 'failure'}, - 'test2': {'description': 'All good here', 'state': 'success'}} - - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') - - self.send_event(client) - - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/1234/comments' - assert kwargs['json'] == {'body': '* Problems here\n'} - - args, kwargs = self.requests_post.call_args_list[1] - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json']['state'] == 'failure' - assert kwargs['json']['description'] == 'Failed some checks' - assert kwargs['json']['context'] == 'testbot' - - def test_one_failure_with_comment_prologue_epilogue(self, app, client): - - # As above, but a default message is given - - test_hook.return_value = {'test1': {'description': 'Problems here', 'state': 'failure'}, - 'test2': {'description': 'All good here', 'state': 'success'}} - - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='All checks passed', - fail_prologue='The prologue - ', - fail_epilogue=' - The epilogue') - - self.send_event(client) - - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/1234/comments' - assert kwargs['json'] == {'body': 'The prologue - * Problems here\n - The epilogue'} - - args, kwargs = self.requests_post.call_args_list[1] - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json']['state'] == 'failure' - assert kwargs['json']['description'] == 'Failed some checks' - assert kwargs['json']['context'] == 'testbot' - # The following test is not relevant currently since we don't skip posting # statuses, due to strange caching issues with GitHub. But if we ever add # back this functionality, the test below could come in handy. @@ -276,9 +161,7 @@ def test_one_failure_with_comment_prologue_epilogue(self, app, client): # test_hook.return_value = {'test1': {'description': 'Problems here', 'state': 'failure'}, # 'test2': {'description': 'All good here', 'state': 'success'}} # - # self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='false', - # all_passed_message='All checks passed', - # fail_prologue='', fail_epilogue='') + # self.get_file_contents.return_value = CONFIG_TEMPLATE # # self.existing_statuses = [{'context': 'testbot:test1', # 'description': 'Problems here', @@ -301,9 +184,7 @@ def test_no_skip_existing_different_statuses(self, app, client): test_hook.return_value = {'test1': {'description': 'Problems here', 'state': 'failure'}, 'test2': {'description': 'All good here', 'state': 'success'}} - self.get_file_contents.return_value = CONFIG_TEMPLATE.format(post_pr_comment='false', - all_passed_message='All checks passed', - fail_prologue='', fail_epilogue='') + self.get_file_contents.return_value = CONFIG_TEMPLATE self.existing_statuses = [{'context': 'testbot:test1', 'description': 'Problems here', @@ -337,56 +218,21 @@ def test_skip_on_labels(self, app, client): # registered handlers don't return any checks test_hook.return_value = {} - self.get_file_contents.return_value = (CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='', - fail_prologue='', - fail_epilogue='') + - 'skip_labels = [ "Experimental" ]\n' + - "skip_message = 'All checks have been skipped'\n") + self.get_file_contents.return_value = (CONFIG_TEMPLATE + + 'skip_labels = [ "Experimental" ]\n') self.labels.return_value = ['Experimental'] self.send_event(client) - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/1234/comments' - assert kwargs['json'] == {'body': 'All checks have been skipped'} + assert self.requests_post.call_count == 1 - args, kwargs = self.requests_post.call_args_list[1] + args, kwargs = self.requests_post.call_args assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' assert kwargs['json'] == {'state': 'failure', 'description': 'Skipping checks due to Experimental label', 'context': 'testbot'} - def test_update_existing_comment(self, app, client): - - # There is already a comment that should be updated - - self.pr_comments = [{'id': 14431, 'body': 'I like apples', 'user': {'login': 'testbot[bot]'}}] - - test_hook.return_value = {} - self.get_file_contents.return_value = (CONFIG_TEMPLATE.format(post_pr_comment='true', - all_passed_message='All checks passed', - fail_prologue='', - fail_epilogue='') + - 'pull_request_substring = "apples"\n') - - self.send_event(client) - - assert self.requests_post.call_count == 2 - - args, kwargs = self.requests_post.call_args_list[0] - assert args[0] == 'https://api.github.com/repos/test-repo/issues/comments/14431' - assert kwargs['json'] == {'body': 'All checks passed'} - - args, kwargs = self.requests_post.call_args_list[1] - assert args[0] == 'https://api.github.com/repos/test-repo/statuses/abc464aa' - assert kwargs['json'] == {'state': 'success', - 'description': 'Passed all checks', - 'context': 'testbot'} - def test_check_returns_none(self, app, client): """ Test that a check can return None to skip itself. diff --git a/doc/plugins.rst b/doc/plugins.rst index bb62157..756a2ba 100644 --- a/doc/plugins.rst +++ b/doc/plugins.rst @@ -32,9 +32,8 @@ Pull request handlers --------------------- We provide a plugin that will perform checks on a pull request and report the -results back to the pull request, either as a comment and a single status check, -or individual status checks. Which checks are done are themselves plugins and -will be described in subsequent sections. +results back to the pull request using status checks. Which checks are done are +themselves plugins and will be described in subsequent sections. To enable pull request handlers, include the following in your ``pyproject.toml`` file:: @@ -45,11 +44,6 @@ To enable pull request handlers, include the following in your In addition, you can use the following configuration items if you wish to change the default behavior: -* ``post_pr_comment = false/true``: if ``true``, the results of the checks will - be summarized in a comment, and a single overall status check will be - reported. If ``false``, each check will be reported as a separate status - check. The default is ``false``. - * ``skip_labels = []``: this can be set to a list of GitHub labels which, if present, will cause the checks to be skipped. Note that labels are case-sensitive. The default is an empty list. @@ -58,26 +52,6 @@ the default behavior: ``skip_labels``, then a failed status check will be posted to the pull request. If ``false``, the checks will be silently skipped. The default is ``true``. -By default, the comment/statuses posted by the bot should be informative, but -if you wish to change the wording of these messages, you can override them with -the following parameters - note that all these only apply when -``post_pr_comment`` is ``true``: - -* ``skip_message = "..."``: the message to display in a comment if the checks are - skipped due to ``skip_labels`` - -* ``fail_prologue = "..."`` and ``fail_epilogue = "..."``: the text to include - before and after the results of the checks in the comment. - -* ``fail_status = "..."`` and ``pass_status = "..."``: the message to show in - the overall status check. - -* ``all_passed_message = "..."``: the message to show in a comment if all checks passed. - -* ``pull_request_substring``: a string that can be used to identify previous - comments posted by the bot. This should be a string common to - ``all_passed_message``, and ``fail_prologue`` or ``fail_epilogue``. - GitHub milestone checker ^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/template/pyproject.toml b/template/pyproject.toml index 656d9b1..1a0342e 100644 --- a/template/pyproject.toml +++ b/template/pyproject.toml @@ -17,16 +17,8 @@ [ tool..pull_requests ] # enabled = true - # post_pr_comment = true - # pull_request_substring = "" # skip_labels = [] # skip_fails = true - # skip_message = "" - # fail_prologue = "" - # fail_epilogue = "" - # fail_status = "" - # pass_status = "" - # all_passed_message = "" [ tool..towncrier_changelog ]