Skip to content

feat(github): dispatch apply comment on deployment count#320

Merged
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/md-10c-comment-dispatch
Jun 14, 2026
Merged

feat(github): dispatch apply comment on deployment count#320
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/md-10c-comment-dispatch

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

What and Why ?

Wires the apply-comment path end to end: storage → presentation → render. Chooses the single- or multi-deployment renderer based on how many apply operations the apply owns, so single-deployment PRs are byte-for-byte unchanged and multi-deployment PRs get the new aggregated comment.

Changes

  • pkg/webhook/apply.go: extract shared tableProgressFromTasks(...) helper used by the existing single-deployment comment builder.
  • New pkg/webhook/multi_apply.go: dispatcher that
    • with <= 1 operation, uses the existing single-deployment renderer unchanged;
    • with >= 2 operations, maps []storage.ApplyOperation to presentation.Operation, groups tasks by ApplyOperationID, derives the presentation model, and renders the multi-deployment comment.

Stacking

Final piece of the MD-10 per-deployment presentation series, on top of #315 (model) and the renderer PR.

@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.

@aparajon

Copy link
Copy Markdown
Collaborator

The dispatch threshold (<=1 op → existing renderer byte-for-byte, >=2 → aggregate) and task routing by ApplyOperationID are right, and the policy resolution at the storage→presentation boundary is the correct seam. Three items:

  1. formatApplyStatusComment has no production callerscomment_observer.go and rollback.go still call formatProgressComment directly, and no call site fetches operation rows, so the dispatcher is dead code as merged. If the wiring is intentionally a follow-up, the description should say so (it currently claims end-to-end wiring). If it lands here, note the fail-closed question: a ListByApply error must not silently render the single-deployment comment for an apply that has siblings — that would hide a sibling failure from the PR.

  2. Build conflict with feat(config): add on_failure enum for rollout continuation #317 — this reads op.HaltOnFailure (*bool), which feat(config): add on_failure enum for rollout continuation #317 replaces with the on_failure enum; the two PRs can't merge independently as written. Suggest mapping from the enum here (and carrying the enum into presentation.Operation so a future pause doesn't need a re-plumb).

  3. Inner footers compete with the aggregate next-action — each deployment's <details> body inherits the single-deployment footer, so a deployment that isn't next in order can still render its own cutover command with the parent apply ID. Consider suppressing the inner footer in multi-deployment sections, or scoping it to the deployment the aggregate names. Related: buildDeploymentDetail shows the parent apply's database and timestamps in every section rather than op.Target and the operation's own timestamps.

Since this PR already has an approval, suggest new commits rather than a force-push.


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

… 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
MD-10c: map apply_operations to the presentation model and route the
status comment to the single- or multi-deployment renderer by operation
count. Dormant until the observer passes operations; single-deployment
and legacy applies render byte-for-byte as today.
#317 replaced the apply_operations halt_on_failure *bool with the
on_failure string enum. Resolve the presentation HaltOnFailure flag from
on_failure at the storage boundary, failing closed to halting for any
value other than "continue".
@Kiran01bm Kiran01bm force-pushed the kiran01bm/md-10c-comment-dispatch branch from 0211826 to 898bda6 Compare June 14, 2026 03:35
Base automatically changed from kiran01bm/md-10b-multi-deployment-comment-render to main June 14, 2026 22:35
@Kiran01bm Kiran01bm merged commit 6c9c582 into main Jun 14, 2026
27 checks passed
@Kiran01bm Kiran01bm deleted the kiran01bm/md-10c-comment-dispatch 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.

2 participants