Surface migration terminal failure cause - #19
Merged
Merged
Conversation
Operator saw "Migration failed" with no reason. Backend now reports a terminal_failure (code, summary, occurred_at) on both target_state and combined_state; render it. One shared formatter in internal/render so status output and the watch TUI cannot drift. Combined wins over target: combined is only set on FAILED, not TERMINATED (user abort), so preferring target would show a cause on migrations the server does not consider failed. Summary is seeded into renderedValues because elm-exporter now prefers the authored summary for combined display_message, which would otherwise print the same sentence twice. --json needed no change; it already passes the raw API document through. Pinned with a test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e558d009-3ea1-4455-b1fd-c2a180332509
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Target fallback can mislabel terminated migrations, and cutover status can suppress the failure explanation entirely.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds rendering for server-recorded terminal migration failure causes across status and watch views.
Changes:
- Models and decodes terminal failure metadata.
- Adds shared failure formatting with timestamps and deduplication.
- Extends tests and kitchen-sink fixtures.
| File | Description |
|---|---|
internal/render/terminal_failure.go |
Resolves and formats terminal failures. |
internal/render/terminal_failure_test.go |
Tests failure rendering and precedence. |
internal/render/migration.go |
Integrates failures into migration output. |
internal/elmapi/migrations.go |
Adds terminal failure API fields. |
internal/elmapi/migrations_test.go |
Tests decoding and raw JSON preservation. |
internal/cmd/migration/watch/view.go |
Displays failure causes in watch mode. |
internal/cmd/migration/watch/watch_test.go |
Tests watch failure details. |
cmd/kitchen-sink/main.go |
Adds a failed-migration preview fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AleBaccin
marked this pull request as draft
September 28, 2026 12:19
Addresses review on #19. Both bugs were places where the code contradicted a doc comment added in the same PR. Gate the target fallback. TerminalFailureFor fell back to the target cause unconditionally, so a terminated migration -- a deliberate abort -- rendered a Failure section whenever the target had separately reported a fault, misattributing the user's own cancellation as a failure. The target cause is now used only when combined state is absent, undecided, or itself failed. Needs its own predicate: statusGlyph/statusText deliberately lump failed in with terminated for glyph purposes, which is the exact distinction here. Render the failure in CutoverStatus. It passed the cause to renderCombinedState, which suppresses a display_message matching the summary, but never rendered the section -- so cutover status showed a bare Failed and deleted text that was visible before this PR. The dedup is now correct rather than destructive, and cutover gains Code and Occurred. Drop t.Helper() from pinNow: setup helper, no assertions. The existing cutover test asserted NotContains(summary), encoding the bug; it now asserts the cause renders exactly once, guarding the drop and the double-print together. Mutation-verified: reverting either fix fails exactly the new cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e558d009-3ea1-4455-b1fd-c2a180332509
AleBaccin
marked this pull request as ready for review
September 28, 2026 13:55
juruen
approved these changes
Sep 30, 2026
juruen
left a comment
juruen
left a comment
There was a problem hiding this comment.
I'm new to this code base, but this LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When a migration reaches a terminal state, ELM now records why. Nothing a customer touches rendered it — an operator saw
Migration failed, orFailed: 42 resources failed, which is a symptom and is often empty entirely (a target repository policy blocks repo creation before any resource is imported).This renders the cause.
Notes for review
--jsonneeded no change. Every command emits viarender.WriteRawJSON(out, resp.Raw)— the raw API document, not re-serialized structs.terminal_failureappears there the moment gh/gh serializes it. Pinned with a regression test rather than left as a happy accident.One shared formatter (
internal/render/terminal_failure.go) consumed by both the status renderer and the watch TUI, so the two surfaces can't drift. The mvnd half of this project shipped a bug where a struct was built at three sites and only two learned about a new field.internal/tuineeded no change —sourceDetailViewdelegates torender.MigrationStatusand inherits the section.Precedence: combined over target.
combined_state.terminal_failureis populated only when combined status isFAILED, deliberately notTERMINATED(a user abort). Target is a faithful passthrough that can be set while combined describes something else, so preferring it would surface a cause on migrations the server doesn't consider failed.The dedup is the fragile part. elm-exporter#872 makes
combined_state.display_messageprefer the authored summary forFAILED, andrenderCombinedStatealready printsDisplayMessage— the same sentence would render twice. The summary is seeded into the existingrenderedValuesslice so the existing normalization suppresses the echo. Covered by tests, and mutation-verified: removing the seeding fails exactly those two subtests.The
codeis rendered raw, not throughfriendlyValue(). It's a contract token; keeping it verbatim makes it greppable and quotable in a support escalation. Unrecognized codes render fine — the server'ssummaryis printed verbatim and nothing switches on the code to produce text, so this won't go stale as mvnd adds codes.Absent
occurred_atomits the line rather than printing—, which would wrongly imply a failure at an unknown time. The server returns null deliberately to distinguish "no time recorded" from "failed in 1970".Testing
make fmt vet lint testclean.make kitchen-sinkhas a new failed-migration fixture whosedisplay_messagedeliberately equals the summary, so the preview demonstrates the dedup.kitchen-sink output 🖌️