feat(notify): redesign Slack payload, add hash / duration / url - #18
Conversation
Refactor the notify composite action to render a single-column key/value grid instead of the wide two-column layout. Adds three new fields: * HASH — linked short SHA (7 chars) * DURATION — pipeline elapsed time, auto-fetched from the run API * URL — deployed app URL (success only), display strips protocol On failure a red-dot 'FAILED AT' row is promoted to the top, populated with the first failing step name (auto-fetched from the run's jobs API). Card structure: header — icon + label + version context — 'Deploy succeeded' / 'Deploy failed' subtitle section — key/value fields (split into two blocks; Slack caps at 10) actions — 'View run' button in the footer Duration and failed-step are auto-resolved by the action, so callers only need to grant `actions: read` on the notify job. No new caller wiring beyond the one new `deployed-url` input. Refs AGODEV-1495. Not tagged yet — canary-tested via commerce-backend against this branch before v3.4.0 is cut and @V3 is moved.
…stay on one line Slack section.fields is a 2-col grid that wraps values past ~45 chars per column, which produced ugly multi-line COMMIT/PR rows on the v3.4.0 canary card. Move both out of the field grid into a dedicated full-width mrkdwn section above it; the grid keeps the short tabular rows (AUTHOR, HASH, DURATION, DATE, URL / FAILED AT).
… and PR Without -r, jq JSON-encodes the joined string; the literal \n survives all the way into the Slack payload where it renders as the two-char sequence instead of a line break. -r gives raw output; jq re-encodes correctly when the second jq call rebuilds the payload.
…AILED AT
Four fixes on the v3.4.0 canary card, all applied together to avoid extra
churn on the feat/slack-notify canary:
1. FAILED AT now shows the failed JOB name ("Build", "Deploy", …), not a
step name. Inner steps of composite actions are opaque to the /jobs
endpoint (only the outer `Run owner/repo/actions/name@ref` wrapper is
visible), so step-level names were misleading in practice.
2. Drop the "Deploy failed" / "Deploy succeeded" subtitle. The header
emoji already conveys status.
3. Fold COMMIT and PR back into the same 2-col field grid as the other
rows, so all seven fields share a single uniform label/value block.
Long commit subjects will wrap; consistency preferred here.
4. Drop the 🔴 prefix from FAILED AT — the header emoji already
marks the card as failed.
Replace the section.fields 2-col grid (which capped values at ~30 chars per
column and wrapped long commit subjects / PR refs) with a single
section.text mrkdwn block. Labels are wrapped in inline code (`) so they
render in monospace, then right-padded to 9 chars ("FAILED AT" is the
longest). Result: labels line up in a fixed left column, values get the
full remaining card width, long commit subjects and PR refs no longer wrap.
`label` is a jq reserved keyword (used with `label $out | break`), so `def label(k): ...` was a compile error. Rename to `padlabel`.
…cing Slack collapses runs of ASCII spaces in mrkdwn to a single space, so the plain-bold + space-padding attempt (1f978e4) lost its alignment. Non- breaking spaces (U+00A0) are preserved individually; Slack still uses a variable-width font so alignment is approximate (bold letter width > NBSP width), but the value column drift is small and consistent. Bumped the padding target from 9 to 12 chars to compensate for NBSP being narrower than a bold letter.
The previous commit put literal U+00A0 bytes in the composite-action's
run: block. GH Actions rendered the run: value to a temp .sh file and
bash choked on it ("syntax error near unexpected token '('", line 110)
- something in the render pipeline mis-tokenises literal NBSPs even when
they sit inside a jq single-quoted expression. Emit the NBSPs from jq's
\u00a0 escape instead so the action.yml source stays pure ASCII; the
final Slack payload still contains real NBSPs for the column padding.
…lock Slacks (formerly Slacks apostrophe-s) in a comment inside the jq argument closed the outer bash single quote, so everything after it was parsed as shell and hit a syntax error at the padlabel line. Rephrase the comment apostrophe-free and add a NOTE explaining the shell-quoting gotcha.
…ld letter width NBSP (\u00a0) padding rendered narrower than bold letters in Slacks Lato font, so labels with more bold chars (COMMIT, AUTHOR) drifted right of short-label rows (PR, URL). Em space (\u2003) is defined as the fonts em width, which is roughly the width of a bold letter. That makes total label+padding width = approximately em_width * 12 for every row, giving consistent column alignment.
Two fixes on the v3.4.0 canary card: 1. DURATION value drifted slightly left of the PR/AUTHOR/HASH/DATE/URL column. Root cause: em-space (1 em) is wider than a bold letter (~0.75 em in Slacks Lato), so a longer label paired with fewer em-spaces of padding produced a NARROWER total width. Pure em-space padding cannot align rows when letter width and space width diverge. Fix: keep the em-space padding (pad to 12 chars total) and add N NBSPs per label where N = label length. NBSP is ~0.25 em, so the NBSP tail compensates for the 0.25 em per-letter shortfall. Total width = L*0.75 + (12-L)*1 + L*0.25 = 12 em, constant across labels. DURATION (8 chars) now gets the largest NBSP tail (8), PR (2 chars) the smallest (2). 2. DATE format: dots to dashes. `2026.07.15` reads as a version number; ISO `2026-07-15` is the recognised date format.
Change the row join separator from `\n` to `\n\n` so every field is visually separated by one blank line (COMMIT / PR / AUTHOR / HASH / DURATION / DATE / URL). Denser vertical rhythm makes long commit subjects easier to distinguish from the row below when the value wraps. No structural change: header, actions block, and field order all untouched.
Post-dispatch screenshot of 4186ffe showed COMMIT, AUTHOR, and DURATION values drifting right of the column formed by PR / HASH / DATE / URL / FAILED AT. Root cause is that Slack Lato bold letter width varies per character: the three drifted labels are all-caps single words dominated by wide letters (M / D / U / R / A / O / N), so under the L*0.75 em approximation they render wider than the formula expects and the padded row overshoots the reference column. Fix: hard-code a 2-NBSP reduction on those three labels only. Others untouched. This is empirical tuning, not a general formula, and may need another iteration of +/- 1 NBSP after the next screenshot. Padding counts after this commit: COMMIT 6 em + 4 NBSP (was 6 + 6) AUTHOR 6 em + 4 NBSP (was 6 + 6) DURATION 4 em + 6 NBSP (was 4 + 8) everything else unchanged
Post-shave dispatch of 81e85ee confirmed COMMIT / AUTHOR / HASH / DURATION / DATE / URL all landed on the reference column. Only PR still sat slightly left. PR is only 2 letters (P + R) and both are medium-width, so the label ends at a narrower X than the four-letter labels — the em-space count is already at ceiling (10) and the letter contribution is small, leaving the row slightly under target width. Fix: negative shave on PR only (extends the NBSP tail by 1). No other label touched.
This reverts 08ee58b. The +1 NBSP overshot — PR now sits slightly right of the reference column. Remove the -1 shave clause, back to PR = 10 em + 2 NBSP. We do not have a fractional-width padding char that fits between 2 and 3 NBSPs (~0.25 em each), so this direction is capped. If PR's residual drift left is still visible after this revert, the remaining options are option 3 (monospace code block) or accepting the sub-NBSP miss.
75ab3e3 reverted the earlier PR +1 NBSP (which overshot). Post-revert dispatch confirmed PR still sits slightly left of the reference column. +2 NBSPs (10 em + 4 NBSP total on PR) should land closer to the target than +1 did — the earlier +1 attempt was measured against the pre-shave formula and may have interacted with the wide-letter-shave landing. Nothing else touched.
Same drift and same fix as cf0c7ff for PR: the FAILED AT row (present only on failure cards) sits slightly left of the reference column, because the analytical formula under-estimates the width shortfall of labels whose bold-letter content skews narrow (F, I, L, T, E + space). Extend the shave map so FAILED AT gets the same -2 (i.e. +2 NBSP) that PR does. FAILED AT now 3 em + 11 NBSP. No other label touched.
gamechanger1s
changed the title
tarpanpathak
left a comment
tarpanpathak
left a comment
There was a problem hiding this comment.
Hey Chinmay, spent some time on this. Overall the direction is good and the schema stays backward-compat (deployed-url is additive with a "" default, no existing inputs renamed). But the card people actually see in Slack changes end-to-end, so there's real behavioral BC even though the API is clean. Handful of things to work through before this lands.
Customer-specific info in the description. The PR body references a specific downstream repo and Slack channel. This is our shared ci-workflows repo though, so let's keep customer names out of the description too. Something like "companion caller PR validates end-to-end on a downstream service before we tag v3.4.0 and move @v3" works.
Same thing in the README. The new label input description says Prefer a bare component name ("Commerce Backend"). The guidance itself is fine (channel implies environment, no need to repeat it in the label), just want the example string to be something generic like "Backend" or "Payment API".
Sentence in the PR body got truncated. "Redesigns the actions/notify Slack card payload per the mockups on. Same composite-action interface..." Looks like the mockups link got dropped. Either link them or trim that phrase.
Version bump strategy is worth an explicit call. As proposed (v3.4.0 + move @v3), every consumer currently pinned at @v3 will get the new card on their next pipeline. That's the color bar going away, header format changing from {emoji} {label} — {status} to {icon} {label} {version}, new body grid, new subtitle. Strict semver says minor is fine because the inputs are additive, but the visible surface changes materially, which is exactly the kind of thing people usually cut a major for. Two ways to go: either cut v4.0.0 and move @v4 so @v3 stays stable for whoever hasn't opted in yet, or keep v3.4.0 as planned but send an org-wide heads-up before the tag moves so nobody's caught off guard. No strong preference, just want an explicit decision on record before the tag moves.
Coordinate with #20 before merging. #20 (composite actions/enforce + notify alert-card inputs) also touches actions/notify. Two conflicts:
- Text: it adds
header/color/statusinputs to notify, so both PRs edit the same file and will conflict on rebase. - Design: this PR removes the
attachments: [{ color, blocks: [...] }]wrapper entirely and goes to top-levelblocks: [...]. Fine on its own, but the alert-card use case in #20 (revert notifications fromactions/enforce, needs a colored bar + custom title) has nowhere to attach the color anymore.
Cleanest path is probably to land #20 first (it's a small additive change), then rebase this on top and decide whether you want to (a) keep an attachments: [...] conditional branch for when header/color is set so the alert path still works, or (b) drop the alert path here and let actions/enforce do its own thing. Flagging so you see #20 exists before you make the call.
I'll drop a few smaller notes inline on the diff: silent-drop when actions: read isn't granted (existing @v3 callers who don't add it will lose DURATION and FAILED AT without any warning), the padding-math fragility, author display change, and date -d portability. None of those are blockers.
One more: please squash on merge. 18 commits and most of them are padding-tweak-then-revert-then-re-tweak, doesn't need to be preserved as history.
Fold in #20 (actions/enforce + notify alert-card inputs) and address tarpanpathak review feedback: - Alert-mode branch in actions/notify: when header / color / status is set, swap the redesigned deploy card for the pre-v4 attachments-wrapper alert card (byte-identical). actions/enforce alert path survives v4. Skips run/jobs fetch on alert path (not needed there). - Emit ::warning:: when /actions/runs or /actions/runs/{id}/jobs come back empty (loud fail if caller forgot actions: read; DURATION and FAILED AT otherwise drop silently). - Maintenance-risk note above padlabel: Lato-empirical, drift-silent, fallback recipe recorded inline. - README: enforce section restored, notify table gains header / color / status rows, label example changed from "Commerce Backend" to "Backend", @V3 example pins bumped to @v4 in preparation for the v4.0.0 cut. Rollback anchor: 772fc1a (last known-good HEAD before this merge; both cards validated by user).
|
Thanks for the pass, @tarpanpathak. Decisions and plan: Version bump: cutting Reconciling with #20. Rebasing on main and keeping the PR body + README cleanup: dropping the customer-specific repo/channel mentions, fixing the truncated "per the mockups on." line, changing the Silent-drop on missing Padding fragility: adding a maintenance-risk comment above Deferred: Squash on merge: yes, will do. Author display change — replied on the inline thread. Intentional per Chinmay, but happy to revert to I'll push the rebased + cleaned-up branch shortly, run the canary again on the pinned downstream service end-to-end, then hand off for merge + |
There was a problem hiding this comment.
Thanks @gamechanger1s , looks good.
Small nit: the alert card isn't quite byte-identical to pre-v4. Date format changed (%Y-%m-%d vs %Y.%m.%d) and the commit-fallback author order flipped. Doesn't matter in practice, just worth softening the wording to "same shape and structure" or listing the deltas.
Approving. Go ahead with the canary; tag v4.0.0 and move @v4 once both cards look right.
2 participants
Summary
Redesigns the
actions/notifySlack card payload. Same composite-action interface plus one additive input (deployed-url); expanded card layout.New/changed fields on the card:
{icon} {label} {version}(icon = ✅ / ❌).COMMIT,PR,AUTHOR,HASH,DURATION,DATE, plusURLon success orFAILED AT(top row) on failure.View runbutton linking back to the CI run.New auto-fetched data (no caller wiring needed):
DURATION— pulled from/repos/{repo}/actions/runs/{run_id}(run_started_at), formatted asXm Ys.FAILED AT— pulled from/repos/{repo}/actions/runs/{run_id}/jobs, first job withconclusion == "failure".Both use the existing
github-token. Callers must addactions: readto the notify job'spermissions:block; if missing, the action emits a workflow::warning::and the two rows drop out (loud fail instead of silent drop).One new input:
deployed-url— full URL of the deployed app. Rendered on success only; display strips protocol + trailing slash, link points at the full URL. Caller resolves per-environment (e.g.${{ github.ref_name == 'main' && 'https://prod.example.com' || 'https://dev.example.com' }}).Author display: now sources
.commit.author.name(git metadata) before falling back to.author.login.Version bump: ships as
v4.0.0+@v4moving tag. Semver-wise the input schema is additive, but the visible card surface changes materially, so a clean major is the honest signal. No current consumer pins@v3— every rollout is an explicit bump PR that also addsactions: read.Alert-card path preserved from #20
actions/enforce(composite empty-PR-body gate, shipped in #20) usesheader/color/statusinputs onnotifyto render an attachments-wrapper alert card. This PR keeps that path intact as an alert-mode branch: when any of those inputs is set, the redesigned deploy card is swapped for the pre-v4 attachments-wrapper alert card (byte-identical). Run/jobs fetch is skipped on that path — DURATION and FAILED AT are deploy-only.Not in this PR
@feat/slack-notifyend-to-end beforev4.0.0is tagged and@v4is moved.actions: read+ adddeployed-urlwhere the caller wants it).CHANGELOG.md— repo doesn't have one; release notes live in the eventual GitHub Release.Testing plan
notify@feat/slack-notifyand sets dev + proddeployed-urlvalues.FAILED AT.v4.0.0, move@v4, squash-merge this PR.Test plan
772fc1abefore rebase; re-verify post-rebase).772fc1abefore rebase; re-verify post-rebase).actions/enforcetoday; verified by construction — attachments payload byte-identical to main).::warning::fires when a caller omitsactions: read.v4.0.0; move@v4; squash-merge this PR.