From 38a739a62d2109ae6e0a8ca0fe9e071f25e23459 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 2 Oct 2026 15:57:16 +0000 Subject: [PATCH 1/7] fix: resolve issue #299 Maintain a single status comment per pull request, edited in place, showing the latest state, the integration branches / pull requests and their build status. --- bert_e/docs/USER_DOC.md | 11 + bert_e/exceptions.py | 18 + bert_e/git_host/base.py | 11 + bert_e/git_host/bitbucket/__init__.py | 17 +- bert_e/git_host/github/__init__.py | 9 +- bert_e/git_host/mock.py | 5 + bert_e/settings.py | 3 + bert_e/templates/pr_status.md | 27 ++ bert_e/tests/test_bert_e.py | 127 +++++- bert_e/tests/test_git_host.py | 16 + .../test_github_bitbucket_edit_comment.py | 113 ++++++ bert_e/tests/unit/test_pr_status_comment.py | 360 ++++++++++++++++++ bert_e/tests/unit/test_status_template.py | 85 +++++ bert_e/workflow/gitwaterflow/__init__.py | 14 +- bert_e/workflow/gitwaterflow/queueing.py | 39 +- .../workflow/gitwaterflow/status_comment.py | 182 +++++++++ bert_e/workflow/pr_utils.py | 44 ++- openwiki/architecture/gitwaterflow.md | 14 + settings.sample.yml | 9 + 19 files changed, 1064 insertions(+), 40 deletions(-) create mode 100644 bert_e/templates/pr_status.md create mode 100644 bert_e/tests/unit/test_github_bitbucket_edit_comment.py create mode 100644 bert_e/tests/unit/test_pr_status_comment.py create mode 100644 bert_e/tests/unit/test_status_template.py create mode 100644 bert_e/workflow/gitwaterflow/status_comment.py diff --git a/bert_e/docs/USER_DOC.md b/bert_e/docs/USER_DOC.md index 664a6124..fdbaa822 100644 --- a/bert_e/docs/USER_DOC.md +++ b/bert_e/docs/USER_DOC.md @@ -34,6 +34,17 @@ and associated tickets. __Bert-E__ helps the participants in a pull request correct the items that do not follow the rules, by issuing a status report and specific messages. +__Bert-E__ also maintains a single **status comment** in each pull request, +titled `Bert-E status`. It is posted right after the first greetings, edited in +place (the description of the pull request is never modified) and always shows +the latest state of the pull request (for instance `Conflict`, `Queued` or +`Merged`), the integration branches with their open integration pull requests +and the status of their builds, and the checklist of the `status` command. +Information messages (greetings, help, status report...) do not change the +state shown. Pull requests which were opened before the status comment was +introduced get it at the next run of __Bert-E__ on them, below the existing +comments. The feature can be disabled with the `pr_status_comment` setting. + * There are different stages in the merge of a pull request * verification that the minimum information required for the process is diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index f9654dad..50571d1c 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -31,6 +31,14 @@ class TemplateException(BertE_Exception): template = None # whether to re-publish if the message is already in the history dont_repeat_if_in_history = -1 + # whether this message describes the current state of the pull request + # (and thus must be reflected in the pull request's status comment), as + # opposed to a one-shot information or the answer to a command. + reports_state = True + # human readable name of the state, defaults to the class name + state_label = None + # status displayed with the state, defaults to `status` + state_status = None def __init__(self, **kwargs): self.kwargs = kwargs @@ -59,6 +67,7 @@ class SilentException(BertE_Exception): # template for informative exceptions class InformationException(TemplateException): dont_repeat_if_in_history = NEVER_REPEAT + reports_state = False # template exceptions @@ -71,24 +80,28 @@ class HelpMessage(TemplateException): code = 101 template = 'help.md' dont_repeat_if_in_history = 0 # allow repeating if requested by user + reports_state = False class SuccessMessage(TemplateException): code = 102 template = 'successful_merge.md' status = "success" + state_label = "Merged" class CommandNotImplemented(TemplateException): code = 103 template = 'not_implemented.md' dont_repeat_if_in_history = 0 # allow repeating if requested by user + reports_state = False class StatusReport(TemplateException): code = 104 template = 'status.md' dont_repeat_if_in_history = 0 # allow repeating if requested by user + reports_state = False class IncompatibleSourceBranchPrefix(TemplateException): @@ -161,6 +174,7 @@ class AfterPullRequest(TemplateException): code = 120 template = 'after_pull_request.md' status = "queued" + state_label = "Waiting for another pull request" class IntegrationDataCreated(InformationException): @@ -215,6 +229,7 @@ class PartialMerge(TemplateException): template = 'partial_merge.md' dont_repeat_if_in_history = 0 # allow repeating as many times as it occurs status = "success" + state_label = "Partially merged" class QueueOutOfOrder(TemplateException): @@ -232,6 +247,7 @@ class LossyResetWarning(TemplateException): code = 129 template = "lossy_reset.md" status = "failure" + reports_state = False class IncorrectCommandSyntax(TemplateException): @@ -274,6 +290,8 @@ class RequestIntegrationBranches(TemplateException): class QueueBuildFailedMessage(TemplateException): code = 136 template = "queue_build_failed.md" + state_label = "Queue build failed" + state_status = "failure" # no bot status check is sent for this one class ForeignCommitsInSourceBranch(TemplateException): diff --git a/bert_e/git_host/base.py b/bert_e/git_host/base.py index a38ab926..fd559848 100644 --- a/bert_e/git_host/base.py +++ b/bert_e/git_host/base.py @@ -257,6 +257,17 @@ class AbstractComment(metaclass=ABCMeta): def delete(self) -> None: """Delete the comment.""" + @abstractmethod + def edit(self, msg: str) -> None: + """Replace the comment's contents with `msg`. + + The comment keeps its place in the pull request's history. + + Args: + - msg: the new raw plaintext of the comment. + + """ + @property @abstractmethod def author(self) -> str: diff --git a/bert_e/git_host/bitbucket/__init__.py b/bert_e/git_host/bitbucket/__init__.py index 6e24460c..71bfaad9 100644 --- a/bert_e/git_host/bitbucket/__init__.py +++ b/bert_e/git_host/bitbucket/__init__.py @@ -348,12 +348,16 @@ def full_name(self): return self['destination']['repository']['full_name'] def add_comment(self, msg): - return Comment.create( + comment = Comment.create( self.client, data=msg, full_name=self.full_name(), pull_request_id=self['id'] ) + # keep the cache of comments (see `comments`) consistent + if getattr(self, '_comments', None): + self._comments.append(comment) + return comment def set_bot_status(self, status: str | None, title: str, summary: str): raise NotImplementedError('"set_bot_status" feature ' @@ -489,6 +493,17 @@ def delete(self): pull_request_id=self.data['pullrequest']['id'], comment_id=self.id) + def edit(self, msg): + 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': msg}})) + response.raise_for_status() + # the pull request caches its comments: keep them up to date + self.data['content']['raw'] = msg + @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..8570994a 100644 --- a/bert_e/git_host/github/__init__.py +++ b/bert_e/git_host/github/__init__.py @@ -269,7 +269,7 @@ def patch(self, url, data, **kwargs): """ url = self._patch_url(url) - response = self.session.post(url, data=data, **kwargs) + response = self.session.patch(url, data=data, **kwargs) response.raise_for_status() return json.loads(response.text) @@ -1015,6 +1015,13 @@ def text(self) -> str: def id(self) -> int: return self.data['id'] + def edit(self, msg: str) -> None: + self.client.patch( + self.data['url'], + data=json.dumps({'body': msg}) + ) + self.data['body'] = msg + 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..a94deedd 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, msg): + self.controlled.content = {"raw": msg, "markup": "markdown", + "html": msg} + self.controlled.updated_on = datetime.now() + def delete(self): self.controlled.delete() diff --git a/bert_e/settings.py b/bert_e/settings.py index 66b68434..4c4ab6c1 100644 --- a/bert_e/settings.py +++ b/bert_e/settings.py @@ -135,6 +135,9 @@ class Meta: frontend_url = fields.Str(required=False, load_default='') + # Maintain an always up-to-date status comment in the pull requests + pr_status_comment = fields.Bool(required=False, load_default=True) + repository_owner = fields.Str(required=True) repository_slug = fields.Str(required=False, load_default=None) diff --git a/bert_e/templates/pr_status.md b/bert_e/templates/pr_status.md new file mode 100644 index 00000000..e12579da --- /dev/null +++ b/bert_e/templates/pr_status.md @@ -0,0 +1,27 @@ +{% extends "message.md" %} + +{% block title -%} +Bert-E status +{% endblock %} + +{% block message %} +{{ icon }} **{{ label }}**{% if code %} (message {{ code }}){% endif %} + +{% if integration is not none %} +{% if integration %} +integration branch | pull request | build +-------------------|--------------|------ +{% for item in integration -%} +`{{ item.branch }}` | {% if item.pr_id %}#{{ item.pr_id }}{% else %}-{% endif %} | {% if item.build %}{{ item.build }}{% else %}-{% endif %} +{% endfor %} +{% else %} +*No integration branch.* +{% endif %} +{% endif %} + +{% if status %} +{% include 'status_report.md' %} +{% endif %} + +*This comment is kept up to date by Bert-E.* +{% endblock %} diff --git a/bert_e/tests/test_bert_e.py b/bert_e/tests/test_bert_e.py index 8b6126c5..55f6ea1d 100644 --- a/bert_e/tests/test_bert_e.py +++ b/bert_e/tests/test_bert_e.py @@ -50,6 +50,7 @@ from bert_e.lib.git import Repository as GitRepository from bert_e.lib.git import Branch, MergeFailedException from bert_e.lib.retry import RetryHandler +from bert_e.workflow.pr_utils import is_status_comment from bert_e.lib.simplecmd import CommandError, cmd from bert_e.settings import setup_settings from bert_e.workflow import gitwaterflow as gwf @@ -938,8 +939,18 @@ class RepositoryTests(unittest.TestCase): 'bypass_leader_approval' ] + def count_comments(self, pr): + """Count the comments of a pull request, except Bert-E's status + comment: it is edited in place and is not part of the conversation. + """ + return len([c for c in pr.get_comments() + if not is_status_comment(c)]) + def get_last_pr_comment(self, pr): - return list(pr.get_comments())[-1].text + """Text of the last message of a pull request (the status comment, + edited in place, is not a message).""" + return [c for c in pr.get_comments() + if not is_status_comment(c)][-1].text def bypass_all_but(self, exceptions): self.assertIsInstance(exceptions, list) @@ -1351,7 +1362,7 @@ def test_comments_without_integration_pull_requests(self): options = self.bypass_all_but(['bypass_build_status']) pr = self.create_pr('feature/TEST-0042', 'development/10') self.handle(pr.id, settings=settings, options=options) - self.assertIs(len(list(pr.get_comments())), 1) + self.assertIs(self.count_comments(pr), 1) self.assertIn('Hello %s' % self.args.contributor_username, self.get_last_pr_comment(pr)) @@ -1387,7 +1398,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(self.count_comments(pr), 2) self.assertIn( 'Request integration branches', self.get_last_pr_comment(pr)) self.assertIn( @@ -1397,7 +1408,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(self.count_comments(pr), 4) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) self.assertIn( 'create_integration_branches', self.get_last_pr_comment(pr)) @@ -1444,7 +1455,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(self.count_comments(pr), 4) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) options = self.bypass_all @@ -1493,7 +1504,7 @@ 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(self.count_comments(pr), 3) self.assertIn( 'Integration data created', list(pr.get_comments())[-2].text) @@ -1548,7 +1559,7 @@ def test_integration_branch_creation_latest_branch(self): options = self.bypass_all_but(['bypass_build_status']) pr = self.create_pr('feature/TEST-0069', 'development/10') self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 1) + self.assertEqual(self.count_comments(pr), 1) with self.assertRaises(exns.BuildNotStarted): self.handle( @@ -1587,7 +1598,7 @@ def test_comments_for_manual_integration_pr_creation(self): options = self.bypass_all_but(['bypass_build_status']) pr = self.create_pr('feature/TEST-0069', 'development/4.3') self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 2) + self.assertEqual(self.count_comments(pr), 2) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) self.assertIn('You can set option', self.get_last_pr_comment(pr)) self.assertNotIn('if you would like to be', @@ -1595,22 +1606,22 @@ def test_comments_for_manual_integration_pr_creation(self): pr.add_comment('Ok ok') self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 3) + self.assertEqual(self.count_comments(pr), 3) comment = pr.add_comment('@%s create_pull_requests' % self.args.robot_username) self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 5) + self.assertEqual(self.count_comments(pr), 5) self.assertIn('Integration data created', self.get_last_pr_comment(pr)) self.assertNotIn('You can set option', self.get_last_pr_comment(pr)) self.assertIn('if you would like to be', self.get_last_pr_comment(pr)) self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 5) + self.assertEqual(self.count_comments(pr), 5) comment.delete() self.handle(pr.id, settings=settings, options=options) - self.assertEqual(len(list(pr.get_comments())), 4) + self.assertEqual(self.count_comments(pr), 4) def test_merge_without_integration_prs(self): """Test a normal Bert-E workflow with no integration PR. @@ -3533,7 +3544,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(self.count_comments(pr), 2) self.assertIn('bypass_jira_check', self.get_last_pr_comment(pr)) settings = """ repository_owner: {owner} @@ -3555,7 +3566,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(self.count_comments(pr), 2) self.assertIn('bypass_author_approval', self.get_last_pr_comment(pr)) settings = """ @@ -3578,7 +3589,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(self.count_comments(pr), 2) self.assertIn('bypass_peer_approval', self.get_last_pr_comment(pr)) settings = """ @@ -3601,7 +3612,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(self.count_comments(pr), 2) self.assertIn('bypass_build_status', self.get_last_pr_comment(pr)) def test_bypass_author_jira(self): @@ -4542,6 +4553,90 @@ def test_main_pr_declined(self): options=self.bypass_all_but(['bypass_build_status']), backtrace=True) + def get_status_comments(self, pr): + return [c for c in pr.get_comments() + if is_status_comment(c, self.args.robot_username)] + + def test_status_comment(self): + """The status comment follows the real flow of a pull request. + + 1. it is created next to the greetings, with the integration branches, + their pull requests and their build status; + 2. it is edited in place (never duplicated) when the state changes: + conflict, reset, merge; + 3. it does not replace the messages and is not part of the + description. + + """ + pr1 = self.create_pr('bugfix/TEST-0006', 'development/10.0', + file_='toto.txt') + pr2 = self.create_pr('bugfix/TEST-0006-other', 'development/10.0', + file_='toto.txt') + description = pr2.description + wbranch = 'w/10/bugfix/TEST-0006-other' + + # Integration branch & pull request are created, build not started + with self.assertRaises(exns.BuildNotStarted): + self.handle( + pr2.id, backtrace=True, + options=self.bypass_all_but(['bypass_build_status'])) + status, = self.get_status_comments(pr2) + comments = list(pr2.get_comments()) + self.assertIn('Hello', comments[0].text) + self.assertEqual(comments[1].id, status.id) + integration_pr, = self.contributor_bb.get_pull_requests( + src_branch=wbranch) + self.assertIn('Waiting for the builds to start', status.text) + self.assertIn( + '`%s` | #%s | NOTSTARTED' % (wbranch, integration_pr.id), + status.text) + + # Same state: the comment is left untouched + with self.assertRaises(exns.BuildNotStarted): + self.handle( + pr2.id, backtrace=True, + options=self.bypass_all_but(['bypass_build_status'])) + self.assertEqual( + [c.text for c in self.get_status_comments(pr2)], [status.text]) + + # The first pull request is merged + with self.assertRaises(exns.SuccessMessage): + self.handle(pr1.id, options=self.bypass_all, backtrace=True) + status1, = self.get_status_comments(pr1) + self.assertIn('Merged', status1.text) + self.assertNotIn('Waiting', status1.text) + + # The second one now conflicts: same comment, new state + with self.assertRaises(exns.Conflict): + self.handle(pr2.id, options=self.bypass_all, backtrace=True) + status, = self.get_status_comments(pr2) + self.assertIn('Conflict', status.text) + self.assertNotIn('Waiting', status.text) + self.assertIn('`%s` | #%s |' % (wbranch, integration_pr.id), + status.text) + self.assertIn('Conflict', self.get_last_pr_comment(pr2)) + + # Reset: the integration data disappears from the status + pr2.add_comment('@%s reset' % self.args.robot_username) + with self.assertRaises(exns.ResetComplete): + self.handle(pr2.id, options=self.bypass_all, backtrace=True) + status, = self.get_status_comments(pr2) + self.assertIn('Reset complete', status.text) + self.assertIn('No integration branch', status.text) + self.assertNotIn(wbranch, status.text) + + # Information and answers to commands do not change the state + pr2.add_comment('@%s help' % self.args.robot_username) + with self.assertRaises(exns.HelpMessage): + self.handle(pr2.id, options=self.bypass_all, backtrace=True) + status, = self.get_status_comments(pr2) + self.assertIn('Reset complete', status.text) + + # The description is never modified + self.assertEqual( + self.contributor_bb.get_pull_request(pr2.id).description, + description) + def test_integration_pr_declined(self): pr = self.create_pr('bugfix/TEST-0001', 'development/4.3') self.gitrepo.cmd('git fetch --all') diff --git a/bert_e/tests/test_git_host.py b/bert_e/tests/test_git_host.py index b82084d4..26f593fa 100644 --- a/bert_e/tests/test_git_host.py +++ b/bert_e/tests/test_git_host.py @@ -244,6 +244,22 @@ def test_pull_request_comments(self, workspace): assert cmt1.text == 'First comment' assert cmt2.text == 'Last comment' + def test_pull_request_edit_comment(self, workspace): + pull_request = make_pull_request( + workspace, 'test_pull_request_edit_comment', 'master' + ) + first = pull_request.add_comment('First comment') + last = pull_request.add_comment('Last comment') + + first.edit('Edited comment') + assert first.text == 'Edited comment' + + # The comment is edited in place: no new comment, same position. + comments = list(pull_request.get_comments()) + assert [c.text for c in comments] == ['Edited comment', + 'Last comment'] + assert [c.id for c in comments] == [first.id, last.id] + def test_build_status(self, workspace): pull_request = make_pull_request(workspace, 'test_build_status', 'master') diff --git a/bert_e/tests/unit/test_github_bitbucket_edit_comment.py b/bert_e/tests/unit/test_github_bitbucket_edit_comment.py new file mode 100644 index 00000000..b89d6699 --- /dev/null +++ b/bert_e/tests/unit/test_github_bitbucket_edit_comment.py @@ -0,0 +1,113 @@ +"""Unit tests of the `edit` method of the pull request comments.""" +import json + +import pytest +import requests +import requests_mock + +from bert_e.git_host import bitbucket, github, mock +from bert_e.git_host.base import AbstractComment + +GITHUB_URL = 'https://api.github.com/repos/octo/repo/issues/comments/42' +BITBUCKET_URL = ('https://api.bitbucket.org/2.0/repositories/octo/repo/' + 'pullrequests/7/comments/42') + + +def test_edit_is_part_of_the_abstract_interface(): + assert 'edit' in AbstractComment.__abstractmethods__ + + +def make_github_comment(): + client = github.Client('login', 'password', 'login@example.com') + return github.Comment( + client=client, _validate=False, id=42, body='old', url=GITHUB_URL, + user={'login': 'Robot'}) + + +def make_bitbucket_comment(): + client = bitbucket.Client('login', 'password', 'login@example.com') + return bitbucket.Comment( + client=client, _validate=False, deleted=False, + content={'raw': 'old'}, pullrequest={'id': 7}, + links={'self': {'href': BITBUCKET_URL}}, + user={'account_id': 'robot'}) + + +def test_github_edit_patches_the_comment(): + comment = make_github_comment() + with requests_mock.Mocker() as mocker: + mocker.patch(GITHUB_URL, json={'id': 42, 'body': 'new'}) + comment.edit('new') + + assert mocker.call_count == 1 + request = mocker.request_history[0] + assert request.method == 'PATCH' + assert request.url == GITHUB_URL + assert json.loads(request.text) == {'body': 'new'} + assert comment.text == 'new' + + +def test_github_edit_http_error_is_raised_and_text_kept(): + comment = make_github_comment() + with requests_mock.Mocker() as mocker: + mocker.patch(GITHUB_URL, status_code=404, json={}) + with pytest.raises(requests.HTTPError): + comment.edit('new') + assert comment.text == 'old' + + +def test_bitbucket_edit_puts_the_comment(): + comment = make_bitbucket_comment() + assert comment.id == '42' + with requests_mock.Mocker() as mocker: + mocker.put(BITBUCKET_URL, json={}) + comment.edit('new') + + assert mocker.call_count == 1 + request = mocker.request_history[0] + assert request.method == 'PUT' + assert request.url == BITBUCKET_URL + assert json.loads(request.text) == {'content': {'raw': 'new'}} + assert comment.text == 'new' + + +def test_bitbucket_edit_http_error_is_raised_and_text_kept(): + comment = make_bitbucket_comment() + with requests_mock.Mocker() as mocker: + mocker.put(BITBUCKET_URL, status_code=403, json={}) + with pytest.raises(requests.HTTPError): + comment.edit('new') + assert comment.text == 'old' + + +def test_bitbucket_pull_request_keeps_its_cache_of_comments_up_to_date(): + client = bitbucket.Client('login', 'password', 'login@example.com') + pull_request = bitbucket.PullRequest( + client, id=7, + destination={'repository': {'full_name': 'octo/repo'}}) + existing = make_bitbucket_comment() + pull_request._comments = [existing] + created = {'content': {'raw': 'hello'}, 'deleted': False, + 'pullrequest': {'id': 7}, 'links': {'self': { + 'href': BITBUCKET_URL.replace('42', '43')}}, + 'user': {'account_id': 'robot'}, + 'created_on': '2024-01-01T00:00:00+00:00'} + with requests_mock.Mocker() as mocker: + mocker.post(BITBUCKET_URL.rsplit('/', 1)[0], json=created) + comment = pull_request.add_comment('hello') + + assert pull_request.comments == [existing, comment] + + +def test_mock_edit_replaces_the_text_in_place(): + client = mock.Client('robot', 'password', 'robot@example.com') + stored = mock.Comment( + client, content='old', pull_request_id=1, full_name='o/r').create() + try: + comment = mock.CommentController(client, stored) + comment.edit('new') + assert comment.text == 'new' + listed = mock.Comment.get_list(client, 'o/r', 1) + assert [c.content['raw'] for c in listed] == ['new'] + finally: + mock.Comment.items = [] diff --git a/bert_e/tests/unit/test_pr_status_comment.py b/bert_e/tests/unit/test_pr_status_comment.py new file mode 100644 index 00000000..3b04a446 --- /dev/null +++ b/bert_e/tests/unit/test_pr_status_comment.py @@ -0,0 +1,360 @@ +"""Unit tests for the always up-to-date status comment of pull requests.""" +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +import requests + +from bert_e import exceptions +from bert_e.workflow import pr_utils +from bert_e.workflow.gitwaterflow import ( + handle_comments, handle_pull_request, status_comment +) +from bert_e.workflow.gitwaterflow.status_comment import ( + State, ensure_status_comment, publish_status, render_status +) + +ROBOT = 'robot' +STATUS = pr_utils.STATUS_COMMENT_HEADER + '\n\nsome status' + + +class FakeComment: + def __init__(self, author, text): + self.author = author + self.text = text + + def edit(self, msg): + self.text = msg + + +class FakePullRequest: + id = 12 + + def __init__(self, comments=()): + self.comments = list(comments) + self.added = 0 + + def add_comment(self, msg): + self.added += 1 + comment = FakeComment(ROBOT, msg) + self.comments.append(comment) + return comment + + +def make_job(pull_request=None, **settings): + settings = dict(dict(robot=ROBOT, no_comment=False, interactive=False, + pr_status_comment=True, build_key='pre-merge'), + **settings) + return SimpleNamespace( + settings=SimpleNamespace(**settings), + pull_request=pull_request or FakePullRequest(), + active_options=['bypass_jira_check'], + git=SimpleNamespace(src_branch=None, dst_branch=None, cascade=None), + ) + + +def instance(exc_class): + """Instance of an exception, without rendering its template: only the + class level description of the state matters here.""" + return exc_class.__new__(exc_class) + + +# --- which messages are states ------------------------------------------- + +@pytest.mark.parametrize('exc_class, label, status', [ + (exceptions.Conflict, 'Conflict', 'failure'), + (exceptions.BuildFailed, 'Build failed', 'failure'), + (exceptions.ApprovalRequired, 'Approval required', 'queued'), + (exceptions.AfterPullRequest, 'Waiting for another pull request', + 'queued'), + (exceptions.Queued, 'Queued', 'in_progress'), + (exceptions.SuccessMessage, 'Merged', 'success'), + (exceptions.PartialMerge, 'Partially merged', 'success'), + (exceptions.QueueBuildFailedMessage, 'Queue build failed', 'failure'), + (exceptions.ResetComplete, 'Reset complete', None), + (exceptions.WrongDestination, 'Wrong destination', None), + (exceptions.BuildInProgress, 'Waiting for the builds to complete', + 'in_progress'), + (exceptions.BuildNotStarted, 'Waiting for the builds to start', None), + (exceptions.PullRequestDeclined, 'Declined', None), +]) +def test_states(exc_class, label, status): + state = State.from_exception(instance(exc_class)) + assert (state.label, state.status) == (label, status) + + +@pytest.mark.parametrize('exc_class', [ + exceptions.InitMessage, exceptions.HelpMessage, exceptions.StatusReport, + exceptions.CommandNotImplemented, exceptions.LossyResetWarning, + exceptions.IntegrationDataCreated, + exceptions.PendingHotfixVersionReminder, + exceptions.NothingToDo, exceptions.NotMyJob, + exceptions.CommentAlreadyExists, exceptions.JobSuccess, +]) +def test_information_is_not_a_state(exc_class): + assert State.from_exception(instance(exc_class)) is None + + +def test_every_information_exception_is_not_a_state(): + for exc_class in exceptions.InformationException.__subclasses__(): + assert State.from_exception(instance(exc_class)) is None + + +def test_final_states(): + final = [exceptions.SuccessMessage, exceptions.PartialMerge, + exceptions.PullRequestDeclined] + assert all(State.from_exception(instance(e)).final for e in final) + assert not State.from_exception(instance(exceptions.Queued)).final + assert not State.from_exception(instance(exceptions.Conflict)).final + + +# --- status comment recognition ------------------------------------------ + +def test_find_comment_ignores_the_status_comment(): + message = FakeComment(ROBOT, 'Hello') + pull_request = FakePullRequest([ + message, FakeComment(ROBOT, STATUS), + FakeComment('someone', 'thanks')]) + # Last message of the robot, and not its status comment + assert pr_utils.find_comment(pull_request, ROBOT) is message + assert pr_utils.find_comment( + pull_request, ROBOT, 'Hello', max_history=-1) is message + assert pr_utils.find_comment( + FakePullRequest([FakeComment(ROBOT, STATUS)]), ROBOT) is None + + +def test_status_comment_must_be_authored_by_the_robot(): + pull_request = FakePullRequest([FakeComment('someone', STATUS)]) + assert pr_utils.find_status_comment(pull_request, ROBOT) is None + assert not pr_utils.is_status_comment(pull_request.comments[0], ROBOT) + + +# --- publishing ----------------------------------------------------------- + +def test_publish_creates_then_edits_in_place(): + job = make_job() + publish_status(job, instance(exceptions.BuildNotStarted), with_git=False) + comment, = job.pull_request.comments + assert comment.text.startswith(pr_utils.STATUS_COMMENT_HEADER) + assert 'Waiting for the builds to start' in comment.text + + # same state: nothing to do + publish_status(job, instance(exceptions.BuildNotStarted), with_git=False) + assert job.pull_request.comments == [comment] + + publish_status(job, instance(exceptions.Conflict), with_git=False) + assert job.pull_request.comments == [comment] + assert 'Conflict' in comment.text + assert 'Waiting' not in comment.text + assert job.pull_request.added == 1 + + +def test_publish_keeps_the_state_on_information(): + job = make_job() + publish_status(job, instance(exceptions.Conflict), with_git=False) + before = job.pull_request.comments[0].text + publish_status(job, instance(exceptions.HelpMessage), with_git=False) + assert job.pull_request.comments[0].text == before + + +@pytest.mark.parametrize('settings', [ + {'pr_status_comment': False}, {'no_comment': True}, + {'interactive': True}, +]) +def test_publish_can_be_disabled(settings): + job = make_job(**settings) + publish_status(job, instance(exceptions.Conflict), with_git=False) + assert job.pull_request.comments == [] + + +def test_publish_never_fails_on_expected_errors(): + job = make_job() + job.pull_request.add_comment = MagicMock( + side_effect=requests.HTTPError('boom')) + publish_status(job, instance(exceptions.Conflict), with_git=False) + + job.pull_request.add_comment = MagicMock( + side_effect=exceptions.FlakyGitHost( + git_host='github', active_options=[])) + publish_status(job, instance(exceptions.Conflict), with_git=False) + + +def test_publish_does_not_hide_unexpected_errors(): + job = make_job() + job.pull_request.add_comment = MagicMock(side_effect=ValueError('bug')) + with pytest.raises(ValueError): + publish_status(job, instance(exceptions.Conflict), with_git=False) + + +def test_ensure_creates_a_placeholder_only_once(): + job = make_job() + ensure_status_comment(job) + comment, = job.pull_request.comments + assert 'Being analyzed' in comment.text + assert 'integration branch' not in comment.text + + publish_status(job, instance(exceptions.Conflict), with_git=False) + ensure_status_comment(job) + assert job.pull_request.comments == [comment] + assert 'Conflict' in comment.text + + +# --- integration pull requests --------------------------------------------- + +def make_branch(name, sha): + return SimpleNamespace(name=name, get_latest_commit=lambda: sha) + + +def make_pr(src_branch, pr_id, status='OPEN'): + return SimpleNamespace(src_branch=src_branch, id=pr_id, status=status) + + +def make_git_job(branches, prs, builds): + job = make_job() + job.git.src_branch = job.git.dst_branch = object() + job.project_repo = MagicMock() + job.project_repo.get_pull_requests.return_value = prs + job.project_repo.get_build_status.side_effect = \ + lambda sha, key: builds[sha] + patcher = patch.object(status_comment, 'get_integration_branches', + return_value=iter(branches)) + return job, patcher + + +def test_integration_rows_follow_the_host(): + branches = [make_branch('w/5/feature/x', 'a'), + make_branch('w/6/feature/x', 'b'), + make_branch('w/7/feature/x', 'c')] + # No pull request for w/7 (never created, or declined by a reset) + prs = [make_pr('w/5/feature/x', 10), make_pr('w/6/feature/x', 11), + make_pr('w/7/feature/x', 12, status='DECLINED')] + builds = {'a': 'SUCCESSFUL', 'b': 'FAILED', 'c': 'INPROGRESS'} + job, patcher = make_git_job(branches, prs, builds) + with patcher: + rows = status_comment._integration_rows(job) + + assert rows == [ + {'branch': 'w/5/feature/x', 'pr_id': 10, 'build': 'SUCCESSFUL'}, + {'branch': 'w/6/feature/x', 'pr_id': 11, 'build': 'FAILED'}, + {'branch': 'w/7/feature/x', 'pr_id': None, 'build': 'INPROGRESS'}, + ] + job.project_repo.get_pull_requests.assert_called_once_with( + src_branch=['w/5/feature/x', 'w/6/feature/x', 'w/7/feature/x']) + + +def test_integration_rows_without_build_key_nor_branches(): + job, patcher = make_git_job([make_branch('w/5/feature/x', 'a')], [], {}) + job.settings.build_key = '' + with patcher: + rows = status_comment._integration_rows(job) + assert rows == [{'branch': 'w/5/feature/x', 'pr_id': None, + 'build': None}] + job.project_repo.get_build_status.assert_not_called() + + job, patcher = make_git_job([], [], {}) + with patcher: + assert status_comment._integration_rows(job) == [] + job.project_repo.get_pull_requests.assert_not_called() + + +def test_render_status_with_git_data(): + job, patcher = make_git_job( + [make_branch('w/5/feature/x', 'a')], [make_pr('w/5/feature/x', 10)], + {'a': 'INPROGRESS'}) + report = {'approvals': SimpleNamespace(display_name='Approvals', + details=['1 peer approval(s) ' + 'missing'], **{'pass': 0})} + with patcher, patch.object(status_comment, 'clone_git_repo'), \ + patch.object(status_comment, '_build_status_report', + return_value=report): + msg = render_status(job, State.from_exception( + instance(exceptions.ApprovalRequired))) + + assert '`w/5/feature/x` | #10 | INPROGRESS' in msg + assert '1 peer approval(s) missing' in msg + + +def test_final_state_has_no_integration_data(): + job, patcher = make_git_job( + [make_branch('w/5/feature/x', 'a')], [], {'a': 'SUCCESSFUL'}) + with patcher as get_branches, \ + patch.object(status_comment, 'clone_git_repo') as clone, \ + patch.object(status_comment, '_build_status_report') as report: + msg = render_status(job, State.from_exception( + instance(exceptions.SuccessMessage))) + + assert 'Merged' in msg + assert 'integration branch' not in msg + assert 'check | status' not in msg + get_branches.assert_not_called() + clone.assert_not_called() + report.assert_not_called() + + +# --- integration in the workflow ------------------------------------------- + +def test_status_is_published_before_the_message(): + job = make_job() + job.pull_request.author = 'someone' + job.pull_request.src_branch = 'feature/x' + error = exceptions.ResetComplete(couldnt_decline=[], active_options=[]) + calls = MagicMock() + with patch('bert_e.workflow.gitwaterflow._handle_pull_request', + side_effect=error), \ + patch('bert_e.workflow.gitwaterflow.publish_status', + calls.publish), \ + patch('bert_e.workflow.gitwaterflow.notify_user', calls.notify): + with pytest.raises(exceptions.ResetComplete): + handle_pull_request(job) + + assert [c[0] for c in calls.mock_calls] == ['publish', 'notify'] + calls.publish.assert_called_once_with(job, error) + + +@pytest.mark.parametrize('exc_class', [ + exceptions.BuildInProgress, exceptions.BuildNotStarted, + exceptions.PullRequestDeclined]) +def test_silent_states_are_published(exc_class): + job = make_job() + job.pull_request.author = 'someone' + job.pull_request.src_branch = 'feature/x' + with patch('bert_e.workflow.gitwaterflow._handle_pull_request', + side_effect=exc_class()), \ + patch('bert_e.workflow.gitwaterflow.publish_status') as publish, \ + patch('bert_e.workflow.gitwaterflow.notify_user') as notify: + with pytest.raises(exc_class): + handle_pull_request(job) + publish.assert_called_once() + notify.assert_not_called() + + +def test_nothing_to_do_is_not_published(): + job = make_job() + job.pull_request.author = 'someone' + job.pull_request.src_branch = 'feature/x' + with patch('bert_e.workflow.gitwaterflow._handle_pull_request', + side_effect=exceptions.NothingToDo()), \ + patch('bert_e.workflow.gitwaterflow.publish_status') as publish: + with pytest.raises(exceptions.NothingToDo): + handle_pull_request(job) + publish.assert_not_called() + + +def test_commands_are_looked_for_past_the_status_comment(): + """The status comment is not a message: a command posted before it, but + after the last message, must still be handled, and the scan must stop at + the last message.""" + pull_request = FakePullRequest([ + FakeComment('someone', '@robot old_command'), + FakeComment(ROBOT, 'a message'), + FakeComment('someone', '@robot status'), + FakeComment(ROBOT, STATUS), + ]) + pull_request.author = 'someone' + job = make_job(pull_request, admins=[]) + with patch('bert_e.workflow.gitwaterflow.Reactor') as reactor_class: + handle_comments(job) + + reactor = reactor_class.return_value + reactor.handle_commands.assert_called_once_with( + job, '@robot status', '@robot', False) 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..ac437b52 --- /dev/null +++ b/bert_e/tests/unit/test_status_template.py @@ -0,0 +1,85 @@ +"""Unit tests for the template of the pull request status comment.""" +from types import SimpleNamespace + +import pytest +from jinja2.exceptions import UndefinedError + +from bert_e.lib.template_loader import render +from bert_e.workflow.pr_utils import STATUS_COMMENT_HEADER + +ROWS = [ + {'branch': 'w/5/feature/x', 'pr_id': 10, 'build': 'SUCCESSFUL'}, + {'branch': 'w/6/feature/x', 'pr_id': None, 'build': None}, +] + + +def render_status(**kwargs): + context = dict(icon=':x:', label='Conflict', code=114, integration=None, + status={}, active_options=[]) + context.update(kwargs) + return render('pr_status.md', **context) + + +def test_starts_with_the_header_which_identifies_it(): + assert render_status().startswith(STATUS_COMMENT_HEADER) + + +def test_state(): + msg = render_status() + assert ':x: **Conflict** (message 114)' in msg + + +def test_state_without_code(): + msg = render_status(code=None, label='Declined') + assert '**Declined**' in msg + assert 'message' not in msg + + +def test_integration_table(): + msg = render_status(integration=ROWS) + assert 'integration branch | pull request | build' in msg + assert '`w/5/feature/x` | #10 | SUCCESSFUL' in msg + assert '`w/6/feature/x` | - | -' in msg + + +def test_no_integration_branch(): + msg = render_status(integration=[]) + assert '*No integration branch.*' in msg + assert 'integration branch |' not in msg + + +def test_unknown_integration_data_is_not_shown(): + msg = render_status(integration=None) + assert 'integration branch' not in msg + assert 'No integration branch' not in msg + + +def test_checklist(): + item = SimpleNamespace(display_name='Approvals', + details=['1 peer approval(s) missing'], + **{'pass': False}) + msg = render_status(status={'approvals': item}) + assert 'check | status' in msg + assert '**Approvals** | :exclamation: 1 peer approval(s) missing' in msg + assert 'check | status' not in render_status(status={}) + + +def test_active_options_are_displayed(): + msg = render_status(active_options=['wait', 'approve']) + assert '**wait, approve**' in msg + + +def test_never_talks_to_the_bot(): + """The robot reacts to what is written after its name: the status comment + must not contain any mention of it.""" + msg = render_status(integration=ROWS, active_options=['wait']) + assert '@' not in msg + + +@pytest.mark.parametrize('missing', ['icon', 'label', 'integration']) +def test_strict_context(missing): + context = dict(icon=':x:', label='Conflict', code=1, integration=None, + status={}, active_options=[]) + del context[missing] + with pytest.raises(UndefinedError): + render('pr_status.md', **context) diff --git a/bert_e/workflow/gitwaterflow/__init__.py b/bert_e/workflow/gitwaterflow/__init__.py index 3ad5c435..0937e81e 100644 --- a/bert_e/workflow/gitwaterflow/__init__.py +++ b/bert_e/workflow/gitwaterflow/__init__.py @@ -25,7 +25,7 @@ from bert_e.lib.simplecmd import CommandError from bert_e.reactor import Reactor, NotFound, NotPrivileged, NotAuthored from ..git_utils import push, clone_git_repo -from ..pr_utils import find_comment, notify_user +from ..pr_utils import find_comment, is_status_comment, notify_user from .branches import ( branch_factory, build_branch_cascade, is_cascade_consumer, is_cascade_producer, BranchCascade, QueueBranch, IntegrationBranch @@ -43,6 +43,7 @@ notify_integration_data, update_integration_branches) from .jira import jira_checks +from .status_comment import ensure_status_comment, publish_status from . import queueing @@ -58,8 +59,15 @@ def handle_pull_request(job: PullRequestJob): try: _handle_pull_request(job) except messages.TemplateException as err: + # The status comment goes first: the message must stay the most + # recent comment of the pull request. + publish_status(job, err) notify_user(job.settings, job.pull_request, err) raise + except (messages.BuildInProgress, messages.BuildNotStarted, + messages.PullRequestDeclined) as err: + publish_status(job, err) + raise @handler(CommitJob) @@ -127,6 +135,7 @@ def _handle_pull_request(job: PullRequestJob): early_checks(job) send_greetings(job) + ensure_status_comment(job) src = job.git.src_branch = branch_factory(job.git.repo, job.pull_request.src_branch) dst = job.git.dst_branch = branch_factory(job.git.repo, @@ -343,6 +352,9 @@ def handle_comments(job): # Look for commands in comments posted after BertE's last message. for comment in reversed(job.pull_request.comments): author = comment.author + if is_status_comment(comment, job.settings.robot): + # not a message: it is edited in place + continue if author == job.settings.robot: return privileged = author in admins and author != pr_author diff --git a/bert_e/workflow/gitwaterflow/queueing.py b/bert_e/workflow/gitwaterflow/queueing.py index 13d05e97..ff60ba97 100644 --- a/bert_e/workflow/gitwaterflow/queueing.py +++ b/bert_e/workflow/gitwaterflow/queueing.py @@ -28,6 +28,7 @@ QueueCollection, QueueIntegrationBranch, branch_factory, build_queue_collection) from .integration import get_integration_branches +from .status_comment import publish_status from typing import List @@ -46,11 +47,11 @@ def notify_queue_build_failed(failed_prs: List[int], job: QueuesJob): # only through build status checks. for pr_id in failed_prs: pull_request = job.project_repo.get_pull_request(pr_id) - notify_user( - job.settings, pull_request, exceptions.QueueBuildFailedMessage( - active_options=job.active_options, - frontend_url=job.bert_e.settings.frontend_url) - ) + message = exceptions.QueueBuildFailedMessage( + active_options=job.active_options, + frontend_url=job.bert_e.settings.frontend_url) + publish_status(job, message, pull_request, with_git=False) + notify_user(job.settings, pull_request, message) @job_handler(QueuesJob) @@ -213,15 +214,15 @@ def close_queued_pull_request(job, pr_id, cascade): if dst.includes_commit(src.get_latest_commit()): # Everything went fine, send a success message - notify_user( - job.settings, pull_request, exceptions.SuccessMessage( - branches=target_branches, - ignored=job.git.cascade.ignored_branches, - pending_hotfixes=job.git.cascade.pending_hotfix_branches, - issue=src.jira_issue_key, - author=pull_request.author_display_name, - active_options=[]) - ) + message = exceptions.SuccessMessage( + branches=target_branches, + ignored=job.git.cascade.ignored_branches, + pending_hotfixes=job.git.cascade.pending_hotfix_branches, + issue=src.jira_issue_key, + author=pull_request.author_display_name, + active_options=[]) + publish_status(job, message, pull_request, with_git=False) + notify_user(job.settings, pull_request, message) else: # Frown at the author for adding posterior changes. This @@ -229,11 +230,11 @@ def close_queued_pull_request(job, pr_id, cascade): # have disappeared, so the normal pre-queuing workflow will restart # naturally. commits = list(src.get_commit_diff(dst)) - notify_user( - job.settings, pull_request, exceptions.PartialMerge( - commits=commits, branches=job.git.cascade.dst_branches, - active_options=[]) - ) + message = exceptions.PartialMerge( + commits=commits, branches=job.git.cascade.dst_branches, + active_options=[]) + publish_status(job, message, pull_request, with_git=False) + notify_user(job.settings, pull_request, message) # Remove integration branches (potentially let Bert-E rebuild them if # the merge was partial) diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py new file mode 100644 index 00000000..ab9da583 --- /dev/null +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -0,0 +1,182 @@ +# Copyright 2016-2018 Scality +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Always up-to-date status comment of a pull request. + +Bert-E posts a new comment at each step, which makes the current state of a +pull request hard to find. In addition, it maintains a single comment, edited +in place, which gives the latest state of the pull request, its integration +branches / pull requests and the status of their builds. + +The comment is never part of the pull request description. +""" +import logging +import re + +import requests + +from bert_e import exceptions +from bert_e.git_host.base import Error as GitHostError +from bert_e.lib import git +from bert_e.lib.schema import SchemaError +from bert_e.lib.simplecmd import CommandError +from bert_e.lib.template_loader import render +from ..git_utils import clone_git_repo +from ..pr_utils import (STATUS_COMMENT_HEADER, find_status_comment, + upsert_status_comment) +from .commands import _build_status_report +from .integration import get_integration_branches + +LOG = logging.getLogger(__name__) + +# Errors that must not prevent Bert-E from going on when it cannot refresh the +# status comment: the git host or the git repository are unreachable, or the +# repository is in a state we do not understand (reported by the main flow). +EXPECTED_ERRORS = ( + requests.exceptions.RequestException, GitHostError, SchemaError, + exceptions.FlakyGitHost, exceptions.InternalException, + git.GitException, CommandError, +) + +ICONS = { + 'success': ':white_check_mark:', + 'failure': ':x:', + 'in_progress': ':hourglass_flowing_sand:', + 'queued': ':hourglass:', + None: ':information_source:', +} + +# Silent exceptions which nevertheless describe a state of the pull request. +SILENT_STATES = { + exceptions.BuildInProgress: 'Waiting for the builds to complete', + exceptions.BuildNotStarted: 'Waiting for the builds to start', + exceptions.PullRequestDeclined: 'Declined', +} + +# States after which the integration data and the checklist (approvals, +# builds...) are meaningless. +FINAL_STATES = (exceptions.SuccessMessage, exceptions.PartialMerge, + exceptions.PullRequestDeclined) + + +class State: + """The state of a pull request, as reported by Bert-E.""" + + def __init__(self, label, status=None, code=None, final=False): + self.label = label + self.status = status + self.code = code + self.final = final + + @property + def icon(self): + return ICONS.get(self.status, ICONS[None]) + + @classmethod + def initial(cls): + return cls('Being analyzed', 'in_progress') + + @classmethod + def from_exception(cls, exc): + """Return the state described by an exception, or None if it does + not describe any (information, answer to a command...).""" + if isinstance(exc, exceptions.TemplateException): + if not exc.reports_state: + return None + label = exc.state_label or re.sub( + r'(?<=[a-z])(?=[A-Z])', ' ', type(exc).__name__).capitalize() + return cls(label, exc.state_status or exc.status, exc.code, + final=isinstance(exc, FINAL_STATES)) + label = SILENT_STATES.get(type(exc)) + if label is None: + return None + return cls(label, exc.status, final=isinstance(exc, FINAL_STATES)) + + +def _integration_rows(job): + """List the existing integration branches, their open pull requests and + their build status. + + This is read from the repository and the git host each time, so that it + follows resets, declined pull requests and recreated branches. + """ + wbranches = list(get_integration_branches(job)) + names = [b.name for b in wbranches] + prs = {} + if names: + prs = {pr.src_branch: pr for pr in + job.project_repo.get_pull_requests(src_branch=names) + if pr.status == 'OPEN'} + key = job.settings.build_key + rows = [] + for branch in wbranches: + pr = prs.get(branch.name) + build = None + if key: + build = job.project_repo.get_build_status( + branch.get_latest_commit(), key) + rows.append({'branch': branch.name, + 'pr_id': pr.id if pr else None, + 'build': build}) + return rows + + +def render_status(job, state, with_git=True): + """Render the status comment. + + Args: + with_git: whether to look at the git repository and the integration + pull requests. Not possible outside of a pull request job. + + """ + integration = None + report = {} + if (with_git and not state.final and + job.git.src_branch and job.git.dst_branch): + clone_git_repo(job) + integration = _integration_rows(job) + report = _build_status_report(job) + msg = render('pr_status.md', icon=state.icon, label=state.label, + code=state.code, integration=integration, status=report, + active_options=job.active_options) + assert msg.startswith(STATUS_COMMENT_HEADER) + return msg + + +def publish_status(job, outcome, pull_request=None, with_git=True): + """Update the pull request's status comment according to the outcome of + a run (an exception, or a State). + + Failing to do so is never fatal. + """ + if not job.settings.pr_status_comment: + return + state = outcome if isinstance(outcome, State) else \ + State.from_exception(outcome) + if state is None: + return + try: + upsert_status_comment( + job.settings, pull_request or job.pull_request, + render_status(job, state, with_git)) + except EXPECTED_ERRORS: + LOG.warning("Could not update the status comment of pull request %s", + (pull_request or job.pull_request).id, exc_info=True) + + +def ensure_status_comment(job): + """Make sure the pull request has a status comment, as early as possible + so that it stays close to the description.""" + if (job.settings.pr_status_comment and + find_status_comment(job.pull_request, job.settings.robot) is None): + publish_status(job, State.initial(), with_git=False) diff --git a/bert_e/workflow/pr_utils.py b/bert_e/workflow/pr_utils.py index a09f3991..e2297399 100644 --- a/bert_e/workflow/pr_utils.py +++ b/bert_e/workflow/pr_utils.py @@ -21,6 +21,21 @@ LOG = logging.getLogger(__name__) +# Every status comment starts with this line: this is how we recognize it. +STATUS_COMMENT_HEADER = '# Bert-E status' + + +def is_status_comment(comment: AbstractComment, username=None) -> bool: + """Tell whether the comment is the bot's pull request status comment. + + Args: + username: if given, the comment must also be authored by this user. + + """ + if username is not None and comment.author != username: + return False + return comment.text.startswith(STATUS_COMMENT_HEADER) + def find_comment(pull_request: AbstractPullRequest, username=None, startswith=None, max_history=None) -> AbstractComment: @@ -36,8 +51,10 @@ def find_comment(pull_request: AbstractPullRequest, username=None, The latest comment if it was found. None otherwise. """ - # check last commits - comments = reversed(pull_request.comments) + # The status comment is edited in place and is not part of the + # conversation: it must neither count as a message nor hide one. + comments = (c for c in reversed(pull_request.comments) + if not is_status_comment(c, username)) if max_history not in (None, -1): comments = itertools.islice(comments, 0, max_history) for comment in comments: @@ -50,6 +67,29 @@ def find_comment(pull_request: AbstractPullRequest, username=None, return comment +def find_status_comment(pull_request: AbstractPullRequest, username + ) -> AbstractComment: + """Return the status comment of the pull request, if any.""" + for comment in pull_request.comments: + if is_status_comment(comment, username): + return comment + + +def upsert_status_comment(settings, pull_request: AbstractPullRequest, + msg: str) -> None: + """Create the status comment, or edit it if its contents changed.""" + if settings.no_comment or settings.interactive: + LOG.debug('Not sending the status comment.') + return + comment = find_status_comment(pull_request, settings.robot) + if comment is None: + LOG.debug('CREATING STATUS COMMENT %s', msg) + pull_request.add_comment(msg) + elif comment.text != msg: + LOG.debug('UPDATING STATUS COMMENT %s', msg) + comment.edit(msg) + + def _send_comment(settings, pull_request: AbstractPullRequest, msg: str, dont_repeat_if_in_history=10) -> None: """Comment a pull request. diff --git a/openwiki/architecture/gitwaterflow.md b/openwiki/architecture/gitwaterflow.md index 2ce92a28..be287f1c 100644 --- a/openwiki/architecture/gitwaterflow.md +++ b/openwiki/architecture/gitwaterflow.md @@ -108,6 +108,20 @@ system**; raising the right exception *is* how you send a message or stop processing. When adding a new check, add a matching exception + template pair here, and reference the message-code table in `USER_DOC.md`. +### The status comment + +`gitwaterflow/status_comment.py` keeps one robot comment per pull request +(header `# Bert-E status`, see `pr_utils.STATUS_COMMENT_HEADER`) up to date. +It is created right after the greetings (`ensure_status_comment`) and +edited in place (`AbstractComment.edit`) by `handle_pull_request` and the +queue handlers (`publish_status`) from the exception that ended the run. +`TemplateException.reports_state` tells states (`Conflict`, `Queued`...) from +information/answers to commands (`InitMessage`, `HelpMessage`...), which leave +the state untouched. Integration branches, their open pull requests and +builds are read from the repository and the git host at each update, and the +checklist reuses `commands._build_status_report`. `find_comment` and the +command scan of `handle_comments` ignore this comment: it is not a message. + ## The merge queue (`bert_e/workflow/gitwaterflow/queueing.py`) When `use_queue` is enabled (the default, toggled by `disable_queues` diff --git a/settings.sample.yml b/settings.sample.yml index 55ec0476..1dd11fca 100644 --- a/settings.sample.yml +++ b/settings.sample.yml @@ -170,6 +170,15 @@ admins: max_commit_diff: 100 +# pr_status_comment [OPTIONAL]: +# Bert-E maintains a comment, edited in place, which shows the latest +# state of the pull request, its integration branches / pull requests and +# their build status. Set this setting to false to disable it. +# +# default value: true +pr_status_comment: true + + # always_create_integration_pull_requests [OPTIONAL]: # Bert-E will create pull requests on integration branches by default. # You can set this setting to false if you don't wan't any integration From 1f2562b286200362cf43f6b0a1226b75b6841178 Mon Sep 17 00:00:00 2001 From: bot Date: Fri, 2 Oct 2026 16:26:49 +0000 Subject: [PATCH 2/7] fix: address review comments on PR #301 --- bert_e/exceptions.py | 6 ++++ bert_e/tests/unit/test_pr_status_comment.py | 18 +++++++++-- .../workflow/gitwaterflow/status_comment.py | 30 +++++++++++++++++-- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index 50571d1c..363f7737 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -184,12 +184,14 @@ class IntegrationDataCreated(InformationException): class UnknownCommand(TemplateException): code = 122 + reports_state = False template = 'unknown_command.md' status = "failure" class NotEnoughCredentials(TemplateException): code = 123 + reports_state = False template = "not_enough_credentials.md" status = "failure" @@ -252,12 +254,14 @@ class LossyResetWarning(TemplateException): class IncorrectCommandSyntax(TemplateException): code = 130 + reports_state = False template = "incorrect_command_syntax.md" status = "failure" class IncorrectPullRequestNumber(TemplateException): code = 131 + reports_state = False template = "incorrect_pull_request_number.md" status = "failure" @@ -270,12 +274,14 @@ class SourceBranchTooOld(TemplateException): class FlakyGitHost(TemplateException): code = 133 + reports_state = False template = "flaky_git_host.md" status = "failure" class NotAuthor(TemplateException): code = 134 + reports_state = False template = "not_author.md" status = "failure" diff --git a/bert_e/tests/unit/test_pr_status_comment.py b/bert_e/tests/unit/test_pr_status_comment.py index 3b04a446..f1a4cfe4 100644 --- a/bert_e/tests/unit/test_pr_status_comment.py +++ b/bert_e/tests/unit/test_pr_status_comment.py @@ -179,11 +179,23 @@ def test_publish_never_fails_on_expected_errors(): publish_status(job, instance(exceptions.Conflict), with_git=False) -def test_publish_does_not_hide_unexpected_errors(): +def test_publish_never_raises_on_unexpected_errors(): job = make_job() job.pull_request.add_comment = MagicMock(side_effect=ValueError('bug')) - with pytest.raises(ValueError): - publish_status(job, instance(exceptions.Conflict), with_git=False) + publish_status(job, instance(exceptions.Conflict), with_git=False) + + +def test_publish_ignores_command_answers(): + job = make_job() + publish_status(job, instance(exceptions.UnknownCommand), with_git=False) + assert job.pull_request.comments == [] + + +def test_publish_does_not_comment_old_declined_pull_requests(): + job = make_job() + publish_status(job, instance(exceptions.PullRequestDeclined), + with_git=False) + assert job.pull_request.comments == [] def test_ensure_creates_a_placeholder_only_once(): diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index ab9da583..0dae0a87 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -144,12 +144,27 @@ def render_status(job, state, with_git=True): if (with_git and not state.final and job.git.src_branch and job.git.dst_branch): clone_git_repo(job) - integration = _integration_rows(job) - report = _build_status_report(job) + repo = job.project_repo + original = repo.get_build_status + cache = {} + + def cached_build_status(revision, key): + if (revision, key) not in cache: + cache[(revision, key)] = original(revision, key) + return cache[(revision, key)] + + # share the build statuses between the table and the checklist + repo.get_build_status = cached_build_status + try: + integration = _integration_rows(job) + report = _build_status_report(job) + finally: + del repo.get_build_status msg = render('pr_status.md', icon=state.icon, label=state.label, code=state.code, integration=integration, status=report, active_options=job.active_options) - assert msg.startswith(STATUS_COMMENT_HEADER) + if not msg.startswith(STATUS_COMMENT_HEADER): + LOG.error("The status comment lacks its header") return msg @@ -165,6 +180,11 @@ def publish_status(job, outcome, pull_request=None, with_git=True): State.from_exception(outcome) if state is None: return + if (isinstance(outcome, exceptions.PullRequestDeclined) and + find_status_comment(pull_request or job.pull_request, + job.settings.robot) is None): + # do not add comments on closed pull requests which never had one + return try: upsert_status_comment( job.settings, pull_request or job.pull_request, @@ -172,6 +192,10 @@ def publish_status(job, outcome, pull_request=None, with_git=True): except EXPECTED_ERRORS: LOG.warning("Could not update the status comment of pull request %s", (pull_request or job.pull_request).id, exc_info=True) + except Exception: + LOG.exception("Unexpected error while updating the status comment " + "of pull request %s", + (pull_request or job.pull_request).id) def ensure_status_comment(job): From 66e75e8352a2954f575245ff2a5d6851c0817d79 Mon Sep 17 00:00:00 2001 From: bot Date: Fri, 2 Oct 2026 16:42:28 +0000 Subject: [PATCH 3/7] fix: address review comments on PR #301 --- bert_e/server/webhook.py | 6 +++++ bert_e/tests/unit/test_pr_status_comment.py | 7 +++--- bert_e/workflow/gitwaterflow/__init__.py | 2 +- .../workflow/gitwaterflow/status_comment.py | 23 ++++++++++++------- 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/bert_e/server/webhook.py b/bert_e/server/webhook.py index e46ae676..f328e55c 100644 --- a/bert_e/server/webhook.py +++ b/bert_e/server/webhook.py @@ -78,6 +78,12 @@ def handle_github_pr_event(bert_e, json_data): def handle_github_issue_comment(bert_e, json_data): """Handle a GitHub webhook sent on an issue comment event.""" + sender = (json_data.get('comment') or {}).get('user') or {} + if sender.get('login') == bert_e.settings.robot: + # Bert-E's own comments (including its status comment) must not + # trigger new runs + LOG.debug('Comment written by the robot, ignoring event') + return event = github.IssueCommentEvent(client=bert_e.client, **json_data) pr = event.pull_request if pr: diff --git a/bert_e/tests/unit/test_pr_status_comment.py b/bert_e/tests/unit/test_pr_status_comment.py index f1a4cfe4..63213927 100644 --- a/bert_e/tests/unit/test_pr_status_comment.py +++ b/bert_e/tests/unit/test_pr_status_comment.py @@ -77,6 +77,7 @@ class level description of the state matters here.""" 'in_progress'), (exceptions.BuildNotStarted, 'Waiting for the builds to start', None), (exceptions.PullRequestDeclined, 'Declined', None), + (exceptions.NothingToDo, 'Nothing to do', None), ]) def test_states(exc_class, label, status): state = State.from_exception(instance(exc_class)) @@ -88,7 +89,7 @@ def test_states(exc_class, label, status): exceptions.CommandNotImplemented, exceptions.LossyResetWarning, exceptions.IntegrationDataCreated, exceptions.PendingHotfixVersionReminder, - exceptions.NothingToDo, exceptions.NotMyJob, + exceptions.NotMyJob, exceptions.CommentAlreadyExists, exceptions.JobSuccess, ]) def test_information_is_not_a_state(exc_class): @@ -340,7 +341,7 @@ def test_silent_states_are_published(exc_class): notify.assert_not_called() -def test_nothing_to_do_is_not_published(): +def test_nothing_to_do_updates_the_status_comment(): job = make_job() job.pull_request.author = 'someone' job.pull_request.src_branch = 'feature/x' @@ -349,7 +350,7 @@ def test_nothing_to_do_is_not_published(): patch('bert_e.workflow.gitwaterflow.publish_status') as publish: with pytest.raises(exceptions.NothingToDo): handle_pull_request(job) - publish.assert_not_called() + publish.assert_called_once() def test_commands_are_looked_for_past_the_status_comment(): diff --git a/bert_e/workflow/gitwaterflow/__init__.py b/bert_e/workflow/gitwaterflow/__init__.py index 0937e81e..73db7ec5 100644 --- a/bert_e/workflow/gitwaterflow/__init__.py +++ b/bert_e/workflow/gitwaterflow/__init__.py @@ -65,7 +65,7 @@ def handle_pull_request(job: PullRequestJob): notify_user(job.settings, job.pull_request, err) raise except (messages.BuildInProgress, messages.BuildNotStarted, - messages.PullRequestDeclined) as err: + messages.PullRequestDeclined, messages.NothingToDo) as err: publish_status(job, err) raise diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index 0dae0a87..209a4b96 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -61,12 +61,19 @@ exceptions.BuildInProgress: 'Waiting for the builds to complete', exceptions.BuildNotStarted: 'Waiting for the builds to start', exceptions.PullRequestDeclined: 'Declined', + exceptions.NothingToDo: 'Nothing to do', } -# States after which the integration data and the checklist (approvals, -# builds...) are meaningless. +# Silent states which are only published when a status comment already exists +# (never create a comment on closed or already merged pull requests). +UPDATE_ONLY = (exceptions.PullRequestDeclined, exceptions.NothingToDo) + +# States for which the integration data and the checklist (approvals, +# builds...) are meaningless, or not worth the git host API calls (build +# states are very frequent and the API is rate limited). FINAL_STATES = (exceptions.SuccessMessage, exceptions.PartialMerge, - exceptions.PullRequestDeclined) + exceptions.PullRequestDeclined, exceptions.NothingToDo, + exceptions.BuildInProgress, exceptions.BuildNotStarted) class State: @@ -180,12 +187,12 @@ def publish_status(job, outcome, pull_request=None, with_git=True): State.from_exception(outcome) if state is None: return - if (isinstance(outcome, exceptions.PullRequestDeclined) and - find_status_comment(pull_request or job.pull_request, - job.settings.robot) is None): - # do not add comments on closed pull requests which never had one - return try: + if (isinstance(outcome, UPDATE_ONLY) and + find_status_comment(pull_request or job.pull_request, + job.settings.robot) is None): + # do not add comments on closed pull requests which never had one + return upsert_status_comment( job.settings, pull_request or job.pull_request, render_status(job, state, with_git)) From 27ca8f7d062320dd9e053efe12f3225c237eacf7 Mon Sep 17 00:00:00 2001 From: bot Date: Fri, 2 Oct 2026 16:55:24 +0000 Subject: [PATCH 4/7] fix: address review comments on PR #301 --- bert_e/server/webhook.py | 8 ++++++++ bert_e/tests/test_server.py | 1 + .../workflow/gitwaterflow/status_comment.py | 20 +++++++++++-------- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/bert_e/server/webhook.py b/bert_e/server/webhook.py index f328e55c..2a2e038f 100644 --- a/bert_e/server/webhook.py +++ b/bert_e/server/webhook.py @@ -60,6 +60,14 @@ def handle_bitbucket_repo_event(bert_e, event, json_data): def handle_bitbucket_pr_event(bert_e, event, json_data): """Handle a Bitbucket webhook sent on a pull request event.""" + commenter = (json_data.get('comment') or {}).get('user') or {} + if event.startswith('comment') and bert_e.settings.robot in ( + commenter.get('nickname'), commenter.get('username'), + commenter.get('account_id')): + # Bert-E's own comments (including its status comment) must not + # trigger new runs + LOG.debug('Comment written by the robot, ignoring event') + return pr_id = json_data['pullrequest']['id'] pr = PullRequest(bert_e.client, **json_data['pullrequest']) LOG.info('The pull request <%s> has been updated', pr_id) diff --git a/bert_e/tests/test_server.py b/bert_e/tests/test_server.py index f7c261d6..37e25100 100644 --- a/bert_e/tests/test_server.py +++ b/bert_e/tests/test_server.py @@ -61,6 +61,7 @@ def __init__(self, *args, **kwargs): self.settings.repository_owner = 'owner' self.settings.repository_slug = 'slug' self.settings.build_key = 'pre-merge' + self.settings.robot = 'robot' self.settings.pull_request_base_url = \ 'https://bitbucket.org/foo/bar/pull-requests/{pr_id}' self.settings.commit_base_url = \ diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index 209a4b96..3782572c 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -69,11 +69,9 @@ UPDATE_ONLY = (exceptions.PullRequestDeclined, exceptions.NothingToDo) # States for which the integration data and the checklist (approvals, -# builds...) are meaningless, or not worth the git host API calls (build -# states are very frequent and the API is rate limited). +# builds...) are meaningless. FINAL_STATES = (exceptions.SuccessMessage, exceptions.PartialMerge, - exceptions.PullRequestDeclined, exceptions.NothingToDo, - exceptions.BuildInProgress, exceptions.BuildNotStarted) + exceptions.PullRequestDeclined, exceptions.NothingToDo) class State: @@ -188,11 +186,17 @@ def publish_status(job, outcome, pull_request=None, with_git=True): if state is None: return try: - if (isinstance(outcome, UPDATE_ONLY) and - find_status_comment(pull_request or job.pull_request, - job.settings.robot) is None): + if isinstance(outcome, UPDATE_ONLY): + existing = find_status_comment(pull_request or job.pull_request, + job.settings.robot) # do not add comments on closed pull requests which never had one - return + if existing is None: + return + # NothingToDo must not overwrite a meaningful state (queued, + # merged, declined, wait...): only replace the initial one + if (isinstance(outcome, exceptions.NothingToDo) and + State.initial().label not in existing.text): + return upsert_status_comment( job.settings, pull_request or job.pull_request, render_status(job, state, with_git)) From 2e5c42ffadd1dc63dbae74b19100253ff0d81535 Mon Sep 17 00:00:00 2001 From: bot Date: Fri, 2 Oct 2026 17:05:25 +0000 Subject: [PATCH 5/7] fix: address review comments on PR #301 --- bert_e/workflow/gitwaterflow/status_comment.py | 17 ++++++++++++++--- bert_e/workflow/pr_utils.py | 11 ++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index 3782572c..4f8c8a80 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -150,6 +150,7 @@ def render_status(job, state, with_git=True): job.git.src_branch and job.git.dst_branch): clone_git_repo(job) repo = job.project_repo + had_own = 'get_build_status' in vars(repo) original = repo.get_build_status cache = {} @@ -164,7 +165,10 @@ def cached_build_status(revision, key): integration = _integration_rows(job) report = _build_status_report(job) finally: - del repo.get_build_status + if had_own: + repo.get_build_status = original + else: + del repo.get_build_status msg = render('pr_status.md', icon=state.icon, label=state.label, code=state.code, integration=integration, status=report, active_options=job.active_options) @@ -185,8 +189,15 @@ def publish_status(job, outcome, pull_request=None, with_git=True): State.from_exception(outcome) if state is None: return + existing = None + waiting = (isinstance(outcome, exceptions.NothingToDo) and + 'wait option' in str(outcome)) + if waiting: + state = State('Waiting (wait option is set)', 'queued', final=True) try: - if isinstance(outcome, UPDATE_ONLY): + if waiting: + pass # the wait option is meaningful: replace any state + elif isinstance(outcome, UPDATE_ONLY): existing = find_status_comment(pull_request or job.pull_request, job.settings.robot) # do not add comments on closed pull requests which never had one @@ -199,7 +210,7 @@ def publish_status(job, outcome, pull_request=None, with_git=True): return upsert_status_comment( job.settings, pull_request or job.pull_request, - render_status(job, state, with_git)) + render_status(job, state, with_git), existing) except EXPECTED_ERRORS: LOG.warning("Could not update the status comment of pull request %s", (pull_request or job.pull_request).id, exc_info=True) diff --git a/bert_e/workflow/pr_utils.py b/bert_e/workflow/pr_utils.py index e2297399..0f7a85e8 100644 --- a/bert_e/workflow/pr_utils.py +++ b/bert_e/workflow/pr_utils.py @@ -76,12 +76,17 @@ def find_status_comment(pull_request: AbstractPullRequest, username def upsert_status_comment(settings, pull_request: AbstractPullRequest, - msg: str) -> None: - """Create the status comment, or edit it if its contents changed.""" + msg: str, comment=None) -> None: + """Create the status comment, or edit it if its contents changed. + + `comment` is the already known status comment, if any, which saves + listing the pull request comments again. + """ if settings.no_comment or settings.interactive: LOG.debug('Not sending the status comment.') return - comment = find_status_comment(pull_request, settings.robot) + if comment is None: + comment = find_status_comment(pull_request, settings.robot) if comment is None: LOG.debug('CREATING STATUS COMMENT %s', msg) pull_request.add_comment(msg) From d19d9ef7b61c09564bb143c3a2e0bf15db0430e0 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Fri, 2 Oct 2026 17:25:05 +0000 Subject: [PATCH 6/7] fix: address review comments on PR #301 --- bert_e/exceptions.py | 4 +++ bert_e/workflow/gitwaterflow/__init__.py | 2 +- .../workflow/gitwaterflow/status_comment.py | 27 ++++--------------- 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index 363f7737..46c954e9 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -596,6 +596,10 @@ class NothingToDo(SilentException): code = 302 +class WaitOptionSet(NothingToDo): + """Nothing is done on purpose because the wait option is set.""" + + class BuildInProgress(SilentException): code = 303 status = "in_progress" diff --git a/bert_e/workflow/gitwaterflow/__init__.py b/bert_e/workflow/gitwaterflow/__init__.py index 73db7ec5..ca959ccb 100644 --- a/bert_e/workflow/gitwaterflow/__init__.py +++ b/bert_e/workflow/gitwaterflow/__init__.py @@ -553,7 +553,7 @@ def check_dependencies(job): """ if job.settings.wait: - raise messages.NothingToDo('wait option is set') + raise messages.WaitOptionSet('wait option is set') after_prs = job.settings.after_pull_request diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index 4f8c8a80..9c6219f4 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -149,26 +149,10 @@ def render_status(job, state, with_git=True): if (with_git and not state.final and job.git.src_branch and job.git.dst_branch): clone_git_repo(job) - repo = job.project_repo - had_own = 'get_build_status' in vars(repo) - original = repo.get_build_status - cache = {} - - def cached_build_status(revision, key): - if (revision, key) not in cache: - cache[(revision, key)] = original(revision, key) - return cache[(revision, key)] - - # share the build statuses between the table and the checklist - repo.get_build_status = cached_build_status - try: - integration = _integration_rows(job) - report = _build_status_report(job) - finally: - if had_own: - repo.get_build_status = original - else: - del repo.get_build_status + # the git hosts cache build statuses, which are thus shared between + # the table and the checklist + integration = _integration_rows(job) + report = _build_status_report(job) msg = render('pr_status.md', icon=state.icon, label=state.label, code=state.code, integration=integration, status=report, active_options=job.active_options) @@ -190,8 +174,7 @@ def publish_status(job, outcome, pull_request=None, with_git=True): if state is None: return existing = None - waiting = (isinstance(outcome, exceptions.NothingToDo) and - 'wait option' in str(outcome)) + waiting = isinstance(outcome, exceptions.WaitOptionSet) if waiting: state = State('Waiting (wait option is set)', 'queued', final=True) try: From 246c2d24b256912f68d6c29d5519b92b146a6e09 Mon Sep 17 00:00:00 2001 From: bot Date: Fri, 2 Oct 2026 17:33:19 +0000 Subject: [PATCH 7/7] fix: address review comments on PR #301 --- bert_e/tests/unit/test_pr_status_comment.py | 10 ++++++++++ bert_e/workflow/gitwaterflow/status_comment.py | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/bert_e/tests/unit/test_pr_status_comment.py b/bert_e/tests/unit/test_pr_status_comment.py index 63213927..04972a95 100644 --- a/bert_e/tests/unit/test_pr_status_comment.py +++ b/bert_e/tests/unit/test_pr_status_comment.py @@ -371,3 +371,13 @@ def test_commands_are_looked_for_past_the_status_comment(): reactor = reactor_class.return_value reactor.handle_commands.assert_called_once_with( job, '@robot status', '@robot', False) + + +def test_wait_option_replaces_any_state(): + assert State.from_exception(instance(exceptions.WaitOptionSet)) is not None + job = make_job() + publish_status(job, instance(exceptions.Conflict), with_git=False) + publish_status(job, instance(exceptions.WaitOptionSet), with_git=False) + comment, = job.pull_request.comments + assert 'Waiting (wait option is set)' in comment.text + assert 'Conflict' not in comment.text diff --git a/bert_e/workflow/gitwaterflow/status_comment.py b/bert_e/workflow/gitwaterflow/status_comment.py index 9c6219f4..9aeb2d34 100644 --- a/bert_e/workflow/gitwaterflow/status_comment.py +++ b/bert_e/workflow/gitwaterflow/status_comment.py @@ -102,7 +102,8 @@ def from_exception(cls, exc): r'(?<=[a-z])(?=[A-Z])', ' ', type(exc).__name__).capitalize() return cls(label, exc.state_status or exc.status, exc.code, final=isinstance(exc, FINAL_STATES)) - label = SILENT_STATES.get(type(exc)) + label = next((v for k, v in SILENT_STATES.items() + if isinstance(exc, k)), None) if label is None: return None return cls(label, exc.status, final=isinstance(exc, FINAL_STATES))