fix(ci): finalize PR notification design (remove octo-pr-feed + fix result-notify)#98
Conversation
…esult-notify Final design (per product owner): - octo-pr-result-notify.yml: terminal state only (merged/closed), routes to global pr-feed via OCTO_PR_FEED_GROUP_ID org variable - octo-pr-review-feed.yml: review events → project group Changes: - Delete octo-pr-feed.yml (pr opened events are intentionally not monitored) - Update octo-pr-result-notify.yml: - Trigger narrowed to [closed] only (drop reopened) - Simplified event_kind expression (pr_merged | pr_closed) - feed_group_id: uses vars.OCTO_PR_FEED_GROUP_ID with safe fallback
Jerry-Xin
left a comment
There was a problem hiding this comment.
Clean follow-up to #97. Two files changed, both correct.
Deleted octo-pr-feed.yml: Removes the opened trigger workflow — per design, only terminal states (merged/closed) and review outcomes are monitored. No orphaned references remain.
octo-pr-result-notify.yml changes verified:
types: [closed, reopened]→[closed]: Correct.reopenedis not a terminal state; removing it aligns with the stated design.event_kindsimplified to${{ github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' }}: Correct. Since the only trigger isclosed, the ternary covers both merged and non-merged cases. The removedpr_reopenedbranch is dead code now.feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}added to bothpr-resultandreview-resultjobs: Addresses #97's blocking issue. The||fallback ensures the workflow works even without the org variable — matches the reusable workflow's own default value.
CI: yamllint ✅ actionlint ✅ kustomize ✅ (check-sprint fail is unrelated sprint tracking).
No blocking issues.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #98 (octo-deployment)
Verdict: APPROVED
A clean, well-scoped CI-only change. No application code touched. The two changes are correct and consistent with the stated final notification design. No P0/P1 issues found.
What changed
octo-pr-feed.ymldeleted — removed theopened-event "new PR" feed.octo-pr-result-notify.yml- Trigger
pull_request_targetnarrowed from[closed, reopened]→[closed]. event_kindsimplified from a 3-way (pr_merged | pr_closed | pr_reopened) to a 2-way (pr_merged | pr_closed).- Added
feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}to both thepr-resultandreview-resultjobs.
- Trigger
Verification
✅ event_kind logic is correct. The pr-result job is guarded by if: github.event_name == 'pull_request_target', and the only pull_request_target type is now closed. Therefore github.event.action is always closed in this job, so dropping the action checks is safe. The new expression github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' resolves correctly: merged → pr_merged, otherwise (closed-without-merge) → pr_closed. Removing the pr_reopened branch is consistent with dropping the reopened trigger.
✅ feed_group_id fallback is correct. vars.OCTO_PR_FEED_GROUP_ID evaluates to an empty (falsy) string when unset, so the || fallback '1c303c142e9840f2a9b46c10b0972e8d' engages. This matches the reusable workflow's own input default (Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main declares feed_group_id default '1c303c142e9840f2a9b46c10b0972e8d'), so behavior is identical whether the org variable is set or not.
✅ Reusable workflow accepts the new input. Confirmed the called reusable workflow declares feed_group_id (type string, optional) — no contract mismatch.
✅ No-overlap ownership holds. octo-pr-result-notify.yml handles terminal/review events (closed + review:submitted) → global feed; octo-pr-review-feed.yml handles [ready_for_review, review_requested] → project group. The event-type sets are disjoint, so no double-notification.
✅ Clean deletion. octo-pr-feed.yml is fully removed at HEAD; no other workflow uses:/calls it.
Findings
P2 (non-blocking) — stale comment reference. .github/workflows/labeler.yml:11 still reads:
# the pattern used by auto-add-to-project.yml and octo-pr-feed.yml.
octo-pr-feed.yml no longer exists after this PR. Purely cosmetic — does not affect any workflow execution — but worth tidying in a follow-up to avoid confusing future readers.
Observation (informational, not a defect)
This is an intentional behavior change per the PR description: opened/reopened PRs no longer generate a feed notification. Note that a non-draft PR opened directly (not via draft→ready) does not emit ready_for_review; such a PR will only surface in a feed once a reviewer is requested (review_requested) or on a terminal/review event. This matches the documented "only terminal states and review outcomes" design decision, so it is by design — flagging only so the team is aware of the coverage gap for directly-opened PRs.
Summary
Finalizes the PR notification design. Clean, no-overlap ownership:
octo-pr-result-notify.yml[closed]+pull_request_review: [submitted]vars.OCTO_PR_FEED_GROUP_IDwith fallback)octo-pr-review-feed.yml[ready_for_review, review_requested]Design decisions (per product owner)
opened/reopenedevents are not monitored — only terminal states (merged/closed) and review outcomesocto-pr-feed.ymlis deleted — it was added in a prior pass but is not needed under the final designChanges
octo-pr-feed.yml: deletedocto-pr-result-notify.yml:[closed, reopened]→[closed](terminal state only)event_kind: simplified topr_merged | pr_closedfeed_group_id:${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}(org variable with safe fallback)No application code modified.