Skip to content

Defer locked terminal dashboard comments #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
trask wants to merge 1 commit into
base: main
Choose a base branch
from
+159 −3
Open

Defer locked terminal dashboard comments #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/scripts/pull-request-dashboard/pr_status_comment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@
DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard"


class StatusCommentDeferred(Exception):
pass


def author_nudge_episode_marker(episode_id: str) -> str:
return f"{AUTHOR_NUDGE_EPISODE_MARKER_PREFIX}{episode_id} -->"

Expand Down Expand Up @@ -408,10 +412,15 @@ def upsert_status_comment(
body: str,
*,
create: bool = True,
locked: bool = False,
) -> None:
comments = managed_status_comments(repo, pr_number)
if comments:
comment = comments[0]
if locked and (comment.get("body") != body or len(comments) > 1):
raise StatusCommentDeferred(
f"PR #{pr_number} is locked; deferring terminal status comment"
)
comment_id = comment["id"]
if comment.get("body") == body:
print(f"PR #{pr_number} status comment is unchanged", file=sys.stderr)
Expand Down Expand Up @@ -457,6 +466,7 @@ def publish_pr_status(repo: str, pr_number: int, dashboard_state: dict[str, Any]
pr_number,
render_status_comment(pr, result),
create=not is_terminal_pr(pr),
locked=is_terminal_pr(pr) and bool(pr.get("locked")),
)


Expand All @@ -472,6 +482,9 @@ def update_targeted_status_comment_from_state(repo: str, pr_number: int) -> list
return []
try:
publish_pr_status(repo, pr_number, dashboard_state)
except StatusCommentDeferred as e:
print(e, file=sys.stderr)
return []
except Exception as e:
return [f"PR #{pr_number}: {e}"]

Expand Down Expand Up @@ -524,6 +537,8 @@ def update_status_comments_from_state(
for number in rollout_pr_numbers:
try:
publish_pr_status(repo, number, dashboard_state)
except StatusCommentDeferred as e:
print(e, file=sys.stderr)
except Exception as e:
errors.append(f"PR #{number}: {e}")
else:
Expand Down
147 changes: 144 additions & 3 deletions .github/scripts/pull-request-dashboard/test_pr_status_comment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,20 @@ def test_does_not_create_comment_when_creation_is_disabled(

self.assertEqual([], self.commands)

@patch.object(pr_status_comment, "managed_status_comments", return_value=[])
def test_locked_pr_without_status_comment_has_nothing_to_defer(
self, _comments: object
) -> None:
pr_status_comment.upsert_status_comment(
"open-telemetry/example",
1,
"body",
create=False,
locked=True,
)

self.assertEqual([], self.commands)

@patch.object(
pr_status_comment,
"managed_status_comments",
Expand All @@ -683,8 +697,49 @@ def test_still_updates_existing_comment_when_creation_is_disabled(

self.assertEqual(["PATCH"], [command[3] for command in self.commands])

@patch.object(
pr_status_comment,
"managed_status_comments",
return_value=[{"id": 7, "body": "<!-- pull-request-dashboard-status --> old"}],
)
def test_locked_pr_defers_existing_comment_update(self, _comments: object) -> None:
with self.assertRaisesRegex(
pr_status_comment.StatusCommentDeferred,
"PR #1 is locked",
):
pr_status_comment.upsert_status_comment(
"open-telemetry/example",
1,
"body",
create=False,
locked=True,
)

self.assertEqual([], self.commands)


class PublishPrStatusTest(unittest.TestCase):
@patch.object(pr_status_comment, "upsert_status_comment")
@patch.object(
pr_status_comment,
"gh_api",
return_value={
"number": 1,
"state": "closed",
"merged": True,
"locked": True,
},
)
def test_locked_terminal_pr_disables_creation_and_marks_update_locked(
self, _gh_api: Mock, upsert: Mock
) -> None:
pr_status_comment.publish_pr_status(
"open-telemetry/example", 1, {"prs": {}}
)

self.assertFalse(upsert.call_args.kwargs["create"])
self.assertTrue(upsert.call_args.kwargs["locked"])

@patch.object(pr_status_comment, "upsert_status_comment")
@patch.object(pr_status_comment, "gh_api")
def test_terminal_pr_never_creates_a_status_comment(
Expand Down Expand Up @@ -747,9 +802,48 @@ def test_requires_dashboard_app_identity_and_marker(self, _gh_api: object) -> No


class RolloutStateTest(unittest.TestCase):
@patch.object(pr_status_comment, "save_status_comment_rollout_state")
@patch.object(
pr_status_comment,
"publish_pr_status",
side_effect=pr_status_comment.StatusCommentDeferred("PR #34 is locked"),
)
@patch.object(
pr_status_comment,
"load_dashboard_state_cache",
return_value={"prs": {}},
)
@patch.object(
pr_status_comment,
"load_status_comment_rollout_state",
return_value={
"target_revision": 12,
"completed_revision": 11,
"pending_pr_numbers": [34],
},
)
def test_targeted_update_retains_deferred_locked_pr(
self,
_load_rollout: object,
_load_dashboard: object,
_publish_pr_status: Mock,
save_rollout: Mock,
) -> None:
status = pr_status_comment.update_targeted_status_comment_from_state(
"open-telemetry/example",
34,
)

self.assertEqual([], status)
save_rollout.assert_not_called()

@patch.object(pr_status_comment, "save_status_comment_rollout_state")
@patch.object(pr_status_comment, "publish_pr_status")
@patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}})
@patch.object(
pr_status_comment,
"load_dashboard_state_cache",
return_value={"prs": {}},
)
@patch.object(
pr_status_comment,
"load_status_comment_rollout_state",
Expand Down Expand Up @@ -779,7 +873,11 @@ def test_targeted_update_only_drains_triggering_pr(

@patch.object(pr_status_comment, "save_status_comment_rollout_state")
@patch.object(pr_status_comment, "publish_pr_status")
@patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}})
@patch.object(
pr_status_comment,
"load_dashboard_state_cache",
return_value={"prs": {}},
)
@patch.object(
pr_status_comment,
"load_status_comment_rollout_state",
Expand Down Expand Up @@ -933,7 +1031,50 @@ def test_failed_comment_write_retains_only_failed_pr_and_continues(
)

self.assertEqual(["PR #12: failed"], errors)
self.assertEqual([12, 34], [call.args[1] for call in publish_pr_status.call_args_list])
self.assertEqual(
[12, 34],
[call.args[1] for call in publish_pr_status.call_args_list],
)
saved_state = save_rollout.call_args.args[0]
self.assertEqual([12], saved_state["pending_pr_numbers"])
self.assertEqual(0, saved_state["completed_revision"])

@patch.object(pr_status_comment, "save_status_comment_rollout_state")
@patch.object(
pr_status_comment,
"publish_pr_status",
side_effect=[
pr_status_comment.StatusCommentDeferred("PR #12 is locked"),
None,
],
)
@patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}})
@patch.object(
pr_status_comment,
"load_status_comment_rollout_state",
return_value={
"target_revision": 0,
"completed_revision": 0,
"pending_pr_numbers": [],
},
)
def test_deferred_locked_pr_stays_pending_without_delivery_error(
self,
_load_rollout: object,
_load_dashboard: object,
publish_pr_status: Mock,
save_rollout: Mock,
) -> None:
errors = pr_status_comment.update_status_comments_from_state(
"open-telemetry/example",
{12, 34},
)

self.assertEqual([], errors)
self.assertEqual(
[12, 34],
[call.args[1] for call in publish_pr_status.call_args_list],
)
saved_state = save_rollout.call_args.args[0]
self.assertEqual([12], saved_state["pending_pr_numbers"])
self.assertEqual(0, saved_state["completed_revision"])
Expand Down