Skip to content

feat(github): render multi-deployment apply comment from presentation model#319

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/md-10b-multi-deployment-comment-render
Jun 14, 2026
Merged

feat(github): render multi-deployment apply comment from presentation model#319
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/md-10b-multi-deployment-comment-render

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

What and Why ?

Adds the GitHub markdown renderer for multi-deployment apply comments, consuming the surface-agnostic presentation.Apply model (from #315). This is the rendering half of MD-10 — it turns the derived presentation into the comment an operator sees on the PR, and seeds the multi-deployment scenarios into the preview pipeline so the UX is reviewable in TEMPLATES.md.

Changes

  • New renderer turns presentation.Apply into a multi-deployment apply comment:
    • aggregate header + state counts + next-action line
    • per-deployment summary list
    • a <details> block per deployment whose body reuses the existing single-deployment renderer, so per-deployment table fidelity is identical to today's single-deployment comment
  • Seeds three multi-deployment apply preview scenarios — barrier in-progress, halt-on-failure, and completed rollout — through the preview pipeline so the aggregate + per-deployment UX auto-generates in TEMPLATES.md.
  • This layer is pure rendering — no storage or dispatch wiring yet (that lands in the follow-up).

Design-note compliance

Verified the seeded scenarios against the agreed UX contract in schemabot/design/schemabot-multi-deployment-ux.md:

Design-note element Covered? Where
Aggregate first, details second All 3 seeds lead with aggregate header + Deployments: … counts, then per-deployment <details>
Display order = deployment_order (eu→us→au→ca) Rows and details emitted in that order in all 3
Open/collapsed defaults (running/waiting/failed open; completed/pending collapsed) Barrier: eu+us <details open>, au/ca collapsed; Failure: us <details open>, eu/au/ca collapsed; Completed: all collapsed
Failure surfaced at aggregate (named failed dep + halted-because) Halt-on-failure seed: ❌ us — failed, ⏸ au — halted — us failed in the aggregate rows
Single next action One fenced command per scenario (cutover / retry)
No internal terminology "deployment", not apply_operation
Invariant #9 — no parallel copy implied Barrier seed has exactly one copy running (us); eu is parked at the cutover barrier — valid barrier semantics, not concurrent copies
Future parallel-copy "multiple ready for cutover" example ⛔️ deliberately omitted The note marks that as a not-yet-built future policy

Known divergence (follow-up): the failure scenario's next-action renders schemabot apply -e production, whereas the design note's Failure-UX example prescribes a per-deployment schemabot retry … --deployment <dep> (with revert reserved for the revert-window only). That's existing renderer behavior, not seed data — tracked as a follow-up.

Stacking

Part of the MD-10 per-deployment presentation series:

Copilot AI review requested due to automatic review settings June 12, 2026 12:30
@Kiran01bm Kiran01bm marked this pull request as ready for review June 12, 2026 12:38
@Kiran01bm Kiran01bm requested review from aparajon and morgo as code owners June 12, 2026 12:38
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a new GitHub-markdown renderer for multi-deployment apply comments by consuming the surface-agnostic presentation.Apply model (introduced in #315). This enables an aggregate “rollup” header plus per-deployment summaries and expandable per-deployment details, while reusing the existing single-deployment renderer for each deployment’s detailed body.

Changes:

  • Added RenderMultiDeploymentApplyComment and supporting helpers to render aggregate + per-deployment sections from presentation.Apply.
  • Added unit tests covering in-progress barrier rollouts, halted failures, reuse of the single-deployment renderer, unknown states, and “no next action” behavior.

Reviewed changes

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

File Description
pkg/webhook/templates/multi_apply.go New renderer for multi-deployment apply PR comments (aggregate header + per-deployment summaries/details).
pkg/webhook/templates/multi_apply_test.go New test coverage validating multi-deployment comment output and structure.

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

Comment thread pkg/webhook/templates/multi_apply.go Outdated
Comment thread pkg/webhook/templates/multi_apply.go Outdated
Comment thread pkg/webhook/templates/multi_apply.go
Comment thread pkg/webhook/templates/multi_apply_test.go Outdated
Comment thread pkg/webhook/templates/multi_apply.go Outdated
Comment thread pkg/webhook/templates/multi_apply.go Outdated
Comment thread pkg/webhook/templates/multi_apply.go Outdated
@aparajon

Copy link
Copy Markdown
Collaborator

The rendering matches the intended model well — aggregate header, per-status counts, single next-action, ordered summary, per-deployment <details> reusing the single-deployment renderer. Two correctness issues in the next-action commands need fixing, plus two smaller items:

  1. Rendered commands aren't executable by today's CLI (pkg/webhook/templates/multi_apply.go:117-129) — cutover/start/revert all require an apply ID, and none of them have a --deployment flag yet. The single-deployment footer renders schemabot cutover <apply-id>. Suggest rendering the executable apply-ID form now, and switching to deployment-targeted commands in the PR that adds that CLI surface — the comment should never instruct an operator to run something that errors.

  2. revert is the wrong verb for a failed deployment — revert targets a deployment inside its post-cutover revert window; a failed deployment never cut over. The existing failed-state footer suggests schemabot apply -e <env> to retry; the aggregate next-action should match that until a per-deployment retry command exists.

  3. <summary> interpolates the deployment name/label as raw HTML without escaping — names come from config, so worth escaping defensively.

  4. A deployment missing from Details renders an empty <details> body; a "no detail available" placeholder would read better.

There are also several existing inline review comments on these same lines that don't appear addressed on the current head — worth resolving those together with the above.


🤖 This review was generated by Claude Code (claude-fable-5) with maintainer approval.

@Kiran01bm

Copy link
Copy Markdown
Collaborator Author
  • Rendered commands aren't executable by today's CLI (pkg/webhook/templates/multi_apply.go:117-129) — cutover/start/revert all require an apply ID, and none of them have a --deployment flag yet. The single-deployment footer renders schemabot cutover <apply-id>. Suggest rendering the executable apply-ID form now, and switching to deployment-targeted commands in the PR that adds that CLI surface — the comment should never instruct an operator to run something that errors.
  • revert is the wrong verb for a failed deployment — revert targets a deployment inside its post-cutover revert window; a failed deployment never cut over. The existing failed-state footer suggests schemabot apply -e <env> to retry; the aggregate next-action should match that until a per-deployment retry command exists.
  • <summary> interpolates the deployment name/label as raw HTML without escaping — names come from config, so worth escaping defensively.
  • A deployment missing from Details renders an empty <details> body; a "no detail available" placeholder would read better.

thank you - all 4 concerns are addressed in 51d578c

… model

MD-10b: aggregate header (state, per-status counts, single next-action),
an at-a-glance per-deployment summary, and a <details> per deployment that
reuses the single-deployment renderer for full per-table fidelity. Dormant
until callers dispatch on operation-row count; single-deployment UX unchanged.
…deployment summaries

Address review on the multi-deployment apply comment: suggest only the
apply-id command forms today's CLI accepts (cutover/start) and the retry
form for a failed apply, since revert applies only to a deployment in its
post-cutover window and no --deployment flag exists yet. HTML-escape
config-derived deployment names/labels, and render a placeholder for a
deployment with no detail. Document that ApplyID/Environment are always
set so the renderer need not guard them.
@Kiran01bm Kiran01bm force-pushed the kiran01bm/md-10b-multi-deployment-comment-render branch from 51d578c to 9db9b9f Compare June 14, 2026 03:08
…S.md

Seed in-progress (barrier), halt-on-failure, and completed rollout scenarios
through the preview pipeline so the aggregate + per-deployment UX renders in
TEMPLATES.md. Also fix PR-only section headings escaping the snapshot redirect.
@Kiran01bm Kiran01bm merged commit a9dd832 into main Jun 14, 2026
27 checks passed
@Kiran01bm Kiran01bm deleted the kiran01bm/md-10b-multi-deployment-comment-render branch June 14, 2026 22:35
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