Skip to content

Scope PR dashboard Slack notifications#20

Merged
trask merged 1 commit into
open-telemetry:mainfrom
trask:pr-dashboard-slack-notification-scope
Jun 26, 2026
Merged

Scope PR dashboard Slack notifications#20
trask merged 1 commit into
open-telemetry:mainfrom
trask:pr-dashboard-slack-notification-scope

Conversation

@trask

@trask trask commented Jun 26, 2026

Copy link
Copy Markdown
Member

Scope Slack notification processing so targeted PR refreshes only send initial notifications for the triggering PR, while scheduled runs handle due follow-up reminders across open PRs. This avoids webhook-driven refreshes sweeping unrelated Slack side effects while preserving the hourly reminder pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR scopes Slack notification processing in the PR dashboard workflow so that side effects match the trigger. Targeted (webhook/workflow_dispatch) refreshes now send only the triggering PR's initial notification, while scheduled runs send only due follow-up reminders across all open PRs. This prevents a single PR's webhook refresh from sweeping unrelated Slack reminders, while keeping the hourly scheduled reminder pass intact. The change plumbs a new --pr-number argument and a notification_kinds filter through notify_slack.pynotifications.py, and gates/keys the notify-slack workflow job accordingly.

Changes:

  • Gate the notify-slack job to run only on schedule or when a pr_number is present, and key its concurrency group per-PR (…-${pr_number || 'full'}), mirroring the existing update-dashboard job pattern.
  • Add a --pr-number CLI arg to notify_slack.py; when present, restrict to that PR with notification_kinds={"initial"}, otherwise use notification_kinds={"follow-up"} across all PRs.
  • Apply a notification_kinds filter in next_notification_state to suppress out-of-scope notification kinds, and document the new behavior in RATIONALE.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
.github/workflows/pull-request-dashboard-repo.yml Gates notify-slack to schedule/PR-targeted runs, scopes concurrency per-PR, and passes --pr-number when set.
.github/scripts/pull-request-dashboard/notify_slack.py Adds --pr-number arg and derives notification_numbers/notification_kinds, threading them through notify_slack/notify_slack_from_state.
.github/scripts/pull-request-dashboard/notifications.py Adds notification_kinds parameter to next_notification_state to suppress kinds outside the requested scope.
.github/scripts/pull-request-dashboard/RATIONALE.md Documents that scheduled runs send follow-ups and targeted refreshes send the triggering PR's initial notification.

I reviewed the argument plumbing (all callers updated to the new signatures), the kind-string coupling ("initial"/"follow-up" match pending_notification_kind's return values), the workflow gating and per-PR concurrency (which aligns with the existing update-dashboard group and is handled safely by the --force-with-lease CAS retry loop), and the RATIONALE wording. I did not find concrete defects. Note that these Python scripts have no automated test suite, and the change alters Slack notification delivery semantics and job concurrency for a side-effect-producing system, which warrants human verification.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@trask trask marked this pull request as ready for review June 26, 2026 17:03
@trask trask requested a review from a team as a code owner June 26, 2026 17:03
@trask trask merged commit 8cf7313 into open-telemetry:main Jun 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants