-
Notifications
You must be signed in to change notification settings - Fork 52
fix(deploy): surface CloudFormation rollback events in the deploy TUI (#1610) #1662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
7e97f90
6c5d416
3fdae14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,13 @@ type ResourceStatus = | |
| | 'UPDATE_FAILED' | ||
| | 'DELETE_IN_PROGRESS' | ||
| | 'DELETE_COMPLETE' | ||
| | 'DELETE_FAILED'; | ||
| | 'DELETE_FAILED' | ||
| | 'ROLLBACK_IN_PROGRESS' | ||
| | 'ROLLBACK_COMPLETE' | ||
| | 'ROLLBACK_FAILED' | ||
| | 'UPDATE_ROLLBACK_IN_PROGRESS' | ||
| | 'UPDATE_ROLLBACK_COMPLETE' | ||
| | 'UPDATE_ROLLBACK_FAILED'; | ||
|
|
||
| interface ParsedResource { | ||
| resourceType: string; | ||
|
|
@@ -75,8 +81,16 @@ interface ParsedResource { | |
|
|
||
| /** | ||
| * Get color for a resource status. | ||
| * | ||
| * Rollback states are checked first: in CloudFormation a ROLLBACK indicates a | ||
| * failed deploy whose changes are being reverted, so even a "clean" finish | ||
| * (ROLLBACK_COMPLETE / UPDATE_ROLLBACK_COMPLETE) is a failure, not a success. | ||
| * Coloring those green would mislead the user (see #1610). A failed rollback is | ||
| * the worst case (red); an in-progress or completed rollback signals | ||
| * "recovering from failure" (yellow). | ||
| */ | ||
| function getStatusColor(status: ResourceStatus): string | undefined { | ||
| if (status.includes('ROLLBACK')) return status.endsWith('_FAILED') ? 'red' : 'yellow'; | ||
| if (status.endsWith('_COMPLETE')) return 'green'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean that |
||
| if (status.endsWith('_FAILED')) return 'red'; | ||
| if (status.endsWith('_IN_PROGRESS')) return 'cyan'; | ||
|
|
@@ -103,7 +117,7 @@ function parseResourceMessage(msg: DeployMessage): ParsedResource | null { | |
| // Format: "StackName | STATUS | AWS::Service::Resource | LogicalId" | ||
| const resourceMatch = /(AWS::\S+)/.exec(text); | ||
| const statusMatch = | ||
| /(CREATE_IN_PROGRESS|CREATE_COMPLETE|CREATE_FAILED|UPDATE_IN_PROGRESS|UPDATE_COMPLETE|UPDATE_FAILED|DELETE_IN_PROGRESS|DELETE_COMPLETE|DELETE_FAILED)/.exec( | ||
| /(UPDATE_ROLLBACK_IN_PROGRESS|UPDATE_ROLLBACK_COMPLETE|UPDATE_ROLLBACK_FAILED|ROLLBACK_IN_PROGRESS|ROLLBACK_COMPLETE|ROLLBACK_FAILED|CREATE_IN_PROGRESS|CREATE_COMPLETE|CREATE_FAILED|UPDATE_IN_PROGRESS|UPDATE_COMPLETE|UPDATE_FAILED|DELETE_IN_PROGRESS|DELETE_COMPLETE|DELETE_FAILED)/.exec( | ||
| text | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,17 @@ import { DeployStatus } from '../DeployStatus.js'; | |
| import { render } from 'ink-testing-library'; | ||
| import React from 'react'; | ||
| import stripAnsi from 'strip-ansi'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| // Force ink/chalk to emit ANSI color codes so the status color-coding tests are | ||
| // deterministic regardless of TTY/CI. vi.hoisted is lifted above the ink import | ||
| // by vitest, so FORCE_COLOR is set before ink evaluates its color support. | ||
| vi.hoisted(() => { | ||
| process.env.FORCE_COLOR = '1'; | ||
| }); | ||
|
|
||
| // ink/chalk ANSI foreground color codes. | ||
| const ANSI = { green: '\x1b[32m', red: '\x1b[31m', yellow: '\x1b[33m', cyan: '\x1b[36m' } as const; | ||
|
|
||
| function makeMsg( | ||
| message: string, | ||
|
|
@@ -95,6 +105,21 @@ describe('DeployStatus', () => { | |
| expect(lastFrame()).not.toContain('Some general info'); | ||
| }); | ||
|
|
||
| it('renders ROLLBACK statuses instead of dropping the events', () => { | ||
| const messages = [ | ||
| makeResourceMsg('CloudFormation::Stack', 'ROLLBACK_IN_PROGRESS'), | ||
| makeResourceMsg('BedrockAgentCore::Gateway', 'UPDATE_ROLLBACK_IN_PROGRESS'), | ||
| makeResourceMsg('CloudFormation::Stack', 'ROLLBACK_FAILED'), | ||
| ]; | ||
|
|
||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
| const frame = lastFrame()!; | ||
|
|
||
| expect(frame).toContain('ROLLBACK_IN_PROGRESS'); | ||
| expect(frame).toContain('UPDATE_ROLLBACK_IN_PROGRESS'); | ||
| expect(frame).toContain('ROLLBACK_FAILED'); | ||
| }); | ||
|
|
||
| it('shows only last 8 resource events', () => { | ||
| const messages = Array.from({ length: 12 }, (_, i) => | ||
| makeResourceMsg(`Service::Resource${i}`, 'CREATE_COMPLETE') | ||
|
|
@@ -112,6 +137,60 @@ describe('DeployStatus', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('status color coding', () => { | ||
| // Returns the ANSI color code wrapping the line that contains the status, e.g. | ||
| // for "...\x1b[33mService::Resource ROLLBACK_COMPLETE\x1b[39m" -> "\x1b[33m". | ||
| // A single resource line is rendered as one colored Text node, so the opening | ||
| // code is the last ANSI escape before the status word on that line. | ||
| function colorOf(frame: string, status: string): string | undefined { | ||
| const line = frame.split('\n').find(l => l.includes(status)); | ||
| if (!line) return undefined; | ||
| const before = line.slice(0, line.indexOf(status)); | ||
| // eslint-disable-next-line no-control-regex | ||
| const codes = before.match(/\x1b\[\d+m/g); | ||
| return codes?.[codes.length - 1]; | ||
| } | ||
|
|
||
| it('renders CREATE_COMPLETE green (sanity check)', () => { | ||
| const messages = [makeResourceMsg('Lambda::Function', 'CREATE_COMPLETE')]; | ||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
|
|
||
| expect(colorOf(lastFrame()!, 'CREATE_COMPLETE')).toBe(ANSI.green); | ||
| }); | ||
|
|
||
| it('does NOT render ROLLBACK_COMPLETE green — it is a failed deploy, not a success (#1610)', () => { | ||
| const messages = [makeResourceMsg('CloudFormation::Stack', 'ROLLBACK_COMPLETE')]; | ||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
|
|
||
| const color = colorOf(lastFrame()!, 'ROLLBACK_COMPLETE'); | ||
| expect(color).not.toBe(ANSI.green); | ||
| expect(color).toBe(ANSI.yellow); | ||
| }); | ||
|
|
||
| it('does NOT render UPDATE_ROLLBACK_COMPLETE green', () => { | ||
| const messages = [makeResourceMsg('BedrockAgentCore::Gateway', 'UPDATE_ROLLBACK_COMPLETE')]; | ||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
|
|
||
| const color = colorOf(lastFrame()!, 'UPDATE_ROLLBACK_COMPLETE'); | ||
| expect(color).not.toBe(ANSI.green); | ||
| expect(color).toBe(ANSI.yellow); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't update rollback complete be red (since it indicates a failed deploy but a successful cleanup)? |
||
| }); | ||
|
|
||
| it('renders rollback in-progress states yellow (recovering from failure)', () => { | ||
| const messages = [makeResourceMsg('CloudFormation::Stack', 'ROLLBACK_IN_PROGRESS')]; | ||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
|
|
||
| expect(colorOf(lastFrame()!, 'ROLLBACK_IN_PROGRESS')).toBe(ANSI.yellow); | ||
| }); | ||
|
|
||
| it('renders ROLLBACK_FAILED red (worst case)', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think that any "rollback" which indicates that certain resources were not deployed should be red. because yellow seems more like a warning or just a statement. what do you think? |
||
| const messages = [makeResourceMsg('CloudFormation::Stack', 'ROLLBACK_FAILED')]; | ||
| const { lastFrame } = render(<DeployStatus messages={messages} isComplete={false} hasError={false} />); | ||
|
|
||
| expect(colorOf(lastFrame()!, 'ROLLBACK_FAILED')).toBe(ANSI.red); | ||
| }); | ||
| }); | ||
|
|
||
| describe('progress bar', () => { | ||
| it('renders progress bar with completed/total count', () => { | ||
| const messages = [makeMsg('deploying', 'CDK_TOOLKIT_I5502', { completed: 3, total: 10 })]; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads-up:
getStatusColoruses suffix matching, so the newROLLBACK_COMPLETEandUPDATE_ROLLBACK_COMPLETEstatuses will render green — same asCREATE_COMPLETE. That's misleading because in CFN those statuses indicate a failed deploy whose rollback finished cleanly, not success.A few options:
getStatusColorso that*ROLLBACK_COMPLETEis yellow/red and*ROLLBACK_IN_PROGRESSis yellow (signalling "recovering from failure").ROLLBACKin the status name first and color all rollback states yellow/red regardless of suffix.✗ Deploy to AWS Failedbanner provides enough context — but note that banner only appears once toolkit-lib throws, which can be well afterROLLBACK_COMPLETElands on screen.Option 1 or 2 would give users a clearer signal during the (potentially multi-minute) rollback window that #1610 is specifically about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed in 3fdae14. Went with option 2 (the cleanest):
getStatusColornow checks forROLLBACKin the status name first, so all rollback states are colored regardless of suffix:ROLLBACK_FAILED/UPDATE_ROLLBACK_FAILED→ red (worst case)ROLLBACK_COMPLETE/UPDATE_ROLLBACK_COMPLETE/*ROLLBACK_IN_PROGRESS→ yellow ("recovering from failure")This means
ROLLBACK_COMPLETEno longer renders green (the misleading-success case), and the user gets a yellow signal throughout the multi-minute rollback window — exactly the #1610 scenario — instead of waiting for the outer✗ Deploy to AWS Failedbanner.Added tests under a new
status color codingblock asserting thatCREATE_COMPLETEstays green whileROLLBACK_COMPLETE/UPDATE_ROLLBACK_COMPLETEare NOT green (yellow), in-progress rollbacks are yellow, andROLLBACK_FAILEDis red.