diff --git a/bert_e/git_host/base.py b/bert_e/git_host/base.py index a38ab926..8251f652 100644 --- a/bert_e/git_host/base.py +++ b/bert_e/git_host/base.py @@ -253,6 +253,10 @@ def key(self) -> str: class AbstractComment(metaclass=ABCMeta): """Abstract class defining the interface of a pull requests's comment.""" + @abstractmethod + def edit(self, text: str) -> None: + """Replace the text of the comment.""" + @abstractmethod def delete(self) -> None: """Delete the comment.""" diff --git a/bert_e/git_host/bitbucket/__init__.py b/bert_e/git_host/bitbucket/__init__.py index 6e24460c..18d86f7f 100644 --- a/bert_e/git_host/bitbucket/__init__.py +++ b/bert_e/git_host/bitbucket/__init__.py @@ -489,6 +489,16 @@ def delete(self): pull_request_id=self.data['pullrequest']['id'], comment_id=self.id) + def edit(self, text): + # Bitbucket Cloud only supports PUT to update a comment + url = self.GET_URL.format( + full_name=self.full_name(), + pull_request_id=self.data['pullrequest']['id'], + comment_id=self.id) + response = self.client.put( + url, data=json.dumps({'content': {'raw': text}})) + self.data = self.load(response).data + @classmethod def create(cls, client, data, **kwargs): return super().create(client, {'content': {'raw': data}}, **kwargs) diff --git a/bert_e/git_host/github/__init__.py b/bert_e/git_host/github/__init__.py index 59515b1c..99dee324 100644 --- a/bert_e/git_host/github/__init__.py +++ b/bert_e/git_host/github/__init__.py @@ -1015,6 +1015,10 @@ def text(self) -> str: def id(self) -> int: return self.data['id'] + def edit(self, text: str) -> None: + self.data = self.client.patch( + self.data['url'], data=json.dumps({'body': text})) + def delete(self) -> None: self.client.delete(self.data['url']) diff --git a/bert_e/git_host/mock.py b/bert_e/git_host/mock.py index 02138b94..e5553895 100644 --- a/bert_e/git_host/mock.py +++ b/bert_e/git_host/mock.py @@ -406,6 +406,11 @@ def comments(self): class CommentController(Controller, base.AbstractComment): + def edit(self, text): + self.controlled.content = { + "raw": text, "markup": "markdown", "html": text} + self.controlled.updated_on = datetime.now() + def delete(self): self.controlled.delete() diff --git a/bert_e/templates/pr_status_comment.md b/bert_e/templates/pr_status_comment.md new file mode 100644 index 00000000..297f6a4b --- /dev/null +++ b/bert_e/templates/pr_status_comment.md @@ -0,0 +1,18 @@ +{{ marker }} +## Bert-E status: {{ state }} +{% if status %} +Result: **{{ status }}** +{% endif %} +{% if integration_prs %} + +Integration pull requests: +{% for pr in integration_prs -%} +* #{{ pr.id }}: `{{ pr.src }}` → `{{ pr.dst }}` +{% endfor %} +{% endif %} +{% if active_options %} + +*Options set:* **{{ active_options|join(', ') }}** +{% endif %} + +*This comment is updated at each step; see the comments below for details.* diff --git a/bert_e/tests/test_bert_e.py b/bert_e/tests/test_bert_e.py index 8b6126c5..9f96e4b5 100644 --- a/bert_e/tests/test_bert_e.py +++ b/bert_e/tests/test_bert_e.py @@ -1387,7 +1387,7 @@ def test_request_integration_branch_creation(self): with self.assertRaises(exns.RequestIntegrationBranches): self.handle( pr.id, settings=settings, options=options, backtrace=True) - self.assertEqual(len(list(pr.get_comments())), 2) + self.assertEqual(len(list(pr.get_comments())), 3) self.assertIn( 'Request integration branches', self.get_last_pr_comment(pr)) self.assertIn( @@ -1397,7 +1397,7 @@ def test_request_integration_branch_creation(self): with self.assertRaises(exns.BuildNotStarted): self.handle( pr.id, settings=settings, options=options, backtrace=True) - self.assertEqual(len(list(pr.get_comments())), 4) + self.assertEqual(len(list(pr.get_comments())), 5) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) self.assertIn( 'create_integration_branches', self.get_last_pr_comment(pr)) @@ -1444,7 +1444,7 @@ def test_request_integration_branch_by_creating_pull_requests(self): with self.assertRaises(exns.BuildNotStarted): self.handle( pr.id, settings=settings, options=options, backtrace=True) - self.assertEqual(len(list(pr.get_comments())), 4) + self.assertEqual(len(list(pr.get_comments())), 5) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) options = self.bypass_all @@ -1493,10 +1493,10 @@ def test_creation_integration_branch_by_approve(self): with self.assertRaises(exns.ApprovalRequired): self.handle(pr.id, options=options, backtrace=True) - self.assertEqual(len(list(pr.get_comments())), 3) + self.assertEqual(len(list(pr.get_comments())), 4) self.assertIn( - 'Integration data created', list(pr.get_comments())[-2].text) + 'Integration data created', list(pr.get_comments())[-3].text) self.assertIn( 'Waiting for approval', self.get_last_pr_comment(pr)) @@ -3533,7 +3533,7 @@ def test_bypass_author_comment_check(self): """ # noqa pr = self.create_pr('feature/TEST-0042', 'development/10') self.handle(pr.id, settings=settings) - self.assertIs(len(list(pr.get_comments())), 2) + self.assertIs(len(list(pr.get_comments())), 3) self.assertIn('bypass_jira_check', self.get_last_pr_comment(pr)) settings = """ repository_owner: {owner} @@ -3555,7 +3555,7 @@ def test_bypass_author_comment_check(self): """ # noqa pr = self.create_pr('feature/TEST-0043', 'development/10') self.handle(pr.id, settings=settings) - self.assertIs(len(list(pr.get_comments())), 2) + self.assertIs(len(list(pr.get_comments())), 3) self.assertIn('bypass_author_approval', self.get_last_pr_comment(pr)) settings = """ @@ -3578,7 +3578,7 @@ def test_bypass_author_comment_check(self): """ # noqa pr = self.create_pr('feature/TEST-0044', 'development/10') self.handle(pr.id, settings=settings) - self.assertIs(len(list(pr.get_comments())), 2) + self.assertIs(len(list(pr.get_comments())), 3) self.assertIn('bypass_peer_approval', self.get_last_pr_comment(pr)) settings = """ @@ -3601,7 +3601,7 @@ def test_bypass_author_comment_check(self): """ # noqa pr = self.create_pr('feature/TEST-0045', 'development/10') self.handle(pr.id, settings=settings) - self.assertIs(len(list(pr.get_comments())), 2) + self.assertIs(len(list(pr.get_comments())), 3) self.assertIn('bypass_build_status', self.get_last_pr_comment(pr)) def test_bypass_author_jira(self): diff --git a/bert_e/tests/test_pr_status_comment.py b/bert_e/tests/test_pr_status_comment.py new file mode 100644 index 00000000..a5cff0d6 --- /dev/null +++ b/bert_e/tests/test_pr_status_comment.py @@ -0,0 +1,93 @@ +"""Tests of the pinned status comment maintained by notify_user.""" +from types import SimpleNamespace + +from bert_e import exceptions +from bert_e.workflow.pr_utils import ( + STATUS_COMMENT_MARKER, find_comment, find_status_comment, notify_user, + update_status_comment, +) + + +class FakeComment: + def __init__(self, author, text): + self.author = author + self.text = text + + def edit(self, text): + self.text = text + + +class FakePR: + def __init__(self): + self.comments = [] + + def add_comment(self, msg): + self.comments.append(FakeComment('bert-e', msg)) + + def set_bot_status(self, *args, **kwargs): + pass + + +SETTINGS = SimpleNamespace(robot='bert-e', no_comment=False, + interactive=False, send_bot_status=False) + + +def _conflict(): + src = SimpleNamespace(name='x') + dst = SimpleNamespace(name='development/1.1', allow_prefixes=['feature']) + return exceptions.IncompatibleSourceBranchPrefix( + active_options=[], source=src, destination=dst) + + +def test_status_comment_created_once_and_updated(): + pr = FakePR() + err = _conflict() + notify_user(SETTINGS, pr, err) + assert len(pr.comments) == 2 + # the pinned status comment comes first + assert pr.comments[0] is find_status_comment(pr, 'bert-e') + assert 'IncompatibleSourceBranchPrefix' in pr.comments[0].text + + other = exceptions.MissingJiraId( + active_options=[], source_branch='x', dest_branch='b') + update_status_comment(SETTINGS, pr, other) + assert len(pr.comments) == 2 + assert 'MissingJiraId' in pr.comments[0].text + + +def test_informational_message_leaves_status_untouched(): + pr = FakePR() + update_status_comment(SETTINGS, pr, _conflict()) + before = pr.comments[0].text + update_status_comment(SETTINGS, pr, exceptions.HelpMessage( + options={}, commands={}, active_options=[])) + assert pr.comments[0].text == before + + +def test_find_comment_skips_status_comment(): + pr = FakePR() + pr.add_comment('hello') + pr.add_comment(STATUS_COMMENT_MARKER + ' status') + assert find_comment(pr, 'bert-e', 'hello', -1) is pr.comments[0] + + +def test_integration_prs_kept_between_updates(): + pr = FakePR() + update_status_comment(SETTINGS, pr, _conflict()) + child = SimpleNamespace(id=7, src_branch='w/1.1/x', name='w/1.1/x', + dst_branch='development/1.1') + info = exceptions.IntegrationDataCreated( + active_options=[], child_prs=[child, child], wbranches=[child, child], + ignored=[], githost='mock', owner='o', slug='r', bert_e='bert-e') + update_status_comment(SETTINGS, pr, info) + assert '#7' in pr.comments[0].text + assert 'IncompatibleSourceBranchPrefix' in pr.comments[0].text + update_status_comment(SETTINGS, pr, _conflict()) + assert '#7' in pr.comments[0].text + + +def test_no_comment_setting_skips(): + pr = FakePR() + settings = SimpleNamespace(**{**vars(SETTINGS), 'no_comment': True}) + update_status_comment(settings, pr, _conflict()) + assert pr.comments == [] diff --git a/bert_e/tests/unit/test_status_template.py b/bert_e/tests/unit/test_status_template.py new file mode 100644 index 00000000..8d6ec6a8 --- /dev/null +++ b/bert_e/tests/unit/test_status_template.py @@ -0,0 +1,31 @@ +"""Unit tests for the pinned pull request status comment template.""" +from bert_e.lib.template_loader import render +from bert_e.workflow.pr_utils import STATUS_COMMENT_MARKER, STATUS_PR_RE + + +def _render(**kwargs): + params = dict(marker=STATUS_COMMENT_MARKER, state='Conflict', code=1, + status=None, integration_prs=[], active_options=None) + params.update(kwargs) + return render('pr_status_comment.md', **params) + + +def test_starts_with_marker_and_shows_state(): + text = _render() + assert text.startswith(STATUS_COMMENT_MARKER) + assert 'Conflict' in text + + +def test_lists_integration_prs_parseable(): + text = _render(integration_prs=[ + {'id': 12, 'src': 'w/1.1/feature/x', 'dst': 'development/1.1'}, + {'id': 13, 'src': 'w/2.0/feature/x', 'dst': 'development/2.0'}]) + found = [m.groupdict() for m in STATUS_PR_RE.finditer(text)] + assert [f['id'] for f in found] == ['12', '13'] + assert found[1]['dst'] == 'development/2.0' + + +def test_result_and_options(): + text = _render(status='failure', active_options=['after_pull_request']) + assert 'failure' in text + assert 'after_pull_request' in text diff --git a/bert_e/workflow/pr_utils.py b/bert_e/workflow/pr_utils.py index a09f3991..5732c6aa 100644 --- a/bert_e/workflow/pr_utils.py +++ b/bert_e/workflow/pr_utils.py @@ -14,13 +14,27 @@ """Pull Requests messaging utility functions.""" import itertools import logging +import re + +import requests from bert_e import exceptions +from bert_e.git_host import base as git_host_base from bert_e.git_host.base import AbstractComment, AbstractPullRequest from bert_e.lib.cli import confirm +from bert_e.lib.template_loader import render LOG = logging.getLogger(__name__) +STATUS_COMMENT_MARKER = '' +STATUS_STATE_RE = re.compile(r'^## Bert-E status: (?P.*)$', + re.MULTILINE) +STATUS_RESULT_RE = re.compile(r'^Result: \*\*(?P.*)\*\*$', + re.MULTILINE) +STATUS_PR_RE = re.compile( + r'^\* #(?P\d+): `(?P[^`]+)` \u2192 `(?P[^`]+)`', + re.MULTILINE) + def find_comment(pull_request: AbstractPullRequest, username=None, startswith=None, max_history=None) -> AbstractComment: @@ -43,6 +57,8 @@ def find_comment(pull_request: AbstractPullRequest, username=None, for comment in comments: if comment.author != username: continue + if comment.text.startswith(STATUS_COMMENT_MARKER): + continue if startswith and not comment.text.startswith(startswith): if max_history == -1: return @@ -98,10 +114,81 @@ def _send_bot_status(settings, pull_request: AbstractPullRequest, ) +def find_status_comment(pull_request: AbstractPullRequest, username): + """Return the pinned status comment posted by the bot, if any.""" + for comment in pull_request.comments: + if comment.author == username and \ + comment.text.startswith(STATUS_COMMENT_MARKER): + return comment + return None + + +def _integration_pull_requests(comment, previous_text=None): + """List the integration pull requests known for a status comment. + + Use those carried by the message when available, otherwise keep the + ones listed in the previous status comment. + """ + child_prs = comment.kwargs.get('child_prs') + if child_prs: + return [{'id': pr.id, 'dst': pr.dst_branch, 'src': pr.src_branch} + for pr in child_prs] + if previous_text: + return [ + {'id': m.group('id'), 'dst': m.group('dst'), + 'src': m.group('src')} + for m in STATUS_PR_RE.finditer(previous_text) + ] + return [] + + +def update_status_comment(settings, pull_request: AbstractPullRequest, + comment: exceptions.TemplateException): + """Create or update the single comment showing the latest bot state. + + The comment is edited in place so that the current status is always at + the same (early) position in the pull request conversation, and the + description of the pull request is left untouched. + """ + if settings.no_comment or settings.interactive: + return + status = getattr(comment, 'status', None) + child_prs = comment.kwargs.get('child_prs') + if status is None and not child_prs: + # informational messages (greetings, help...) do not change the + # state of the pull request + return + existing = find_status_comment(pull_request, settings.robot) + state, code = comment.title, comment.code + if status is None: + # only the integration pull requests changed: keep the state + if existing is None: + return + m = STATUS_STATE_RE.search(existing.text) + state = m.group('state') if m else state + m = STATUS_RESULT_RE.search(existing.text) + status = m.group('status') if m else None + code = None + prs = _integration_pull_requests( + comment, existing.text if existing else None) + text = render('pr_status_comment.md', marker=STATUS_COMMENT_MARKER, + state=state, code=code, status=status, + integration_prs=prs, + active_options=comment.kwargs.get('active_options')) + if existing is None: + pull_request.add_comment(text) + elif existing.text.strip() != text.strip(): + existing.edit(text) + + def notify_user(settings, pull_request: AbstractPullRequest, comment: exceptions.TemplateException): """Notify user by sending a comment or a build status in a pull request.""" try: + try: + update_status_comment(settings, pull_request, comment) + except (requests.HTTPError, git_host_base.Error): + LOG.warning("Could not update the status comment", exc_info=True) _send_bot_status(settings, pull_request, comment) _send_comment(settings, pull_request, str(comment), comment.dont_repeat_if_in_history)