Skip to content

Commit dcf55e4

Browse files
justinmk3justinmk3
authored
Merge #3761 surface SAM CLI failures
2 parents 6fcb85a + 89fcb18 commit dcf55e4

8 files changed

+114
-97
lines changed

‎.changes/next-release/Feature-c4dffad5-a36b-4f38-95d2-efbb2196c103.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"type": "Feature",
3+
"description": "SAM run/debug detects and displays SAM CLI errors, so you spend less time inspecting the Output"
4+
}

‎.changes/next-release/Feature-f3f8fadc-c390-4099-8eff-894b78f4cc58.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"type": "Feature",
3+
"description": "Toolkit no longer explicitly checks if Docker is running, instead it lets SAM CLI decide that #3588"
4+
}

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2181,7 +2181,7 @@
21812181
{
21822182
"command": "aws.ec2.copyInstanceId",
21832183
"title": "%AWS.command.ec2.copyInstanceId%",
2184-
"category": "%AWS.title",
2184+
"category": "%AWS.title%",
21852185
"cloud9": {
21862186
"cn": {
21872187
"category": "%AWS.title.cn%"

‎src/shared/sam/cli/samCliInvokerUtils.ts‎

Lines changed: 65 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,14 @@ import { SpawnOptions } from 'child_process'
77
import { getLogger } from '../../logger'
88
import { getUserAgent } from '../../telemetry/util'
99
import { ChildProcessResult, ChildProcessOptions } from '../../utilities/childProcess'
10+
import { ErrorInformation, ToolkitError } from '../../errors'
11+
12+
/** Generic SAM CLI invocation error. */
13+
export class SamCliError extends ToolkitError.named('SamCliError') {
14+
public constructor(message?: string, info?: ErrorInformation) {
15+
super(message ?? 'SAM CLI failed', { ...info, code: 'SamCliFailed' })
16+
}
17+
}
1018

1119
export interface SamCliProcessInvokeOptions {
1220
spawnOptions?: SpawnOptions
@@ -34,71 +42,86 @@ export interface SamCliProcessInvoker {
3442
}
3543

3644
export function makeUnexpectedExitCodeError(message: string): Error {
37-
return new Error(`Error with child process: ${message}`)
45+
const msg = message ? message : 'SAM CLI failed'
46+
return new SamCliError(msg)
3847
}
3948

40-
export function logAndThrowIfUnexpectedExitCode(processResult: ChildProcessResult, expectedExitCode: number): void {
41-
if (processResult.exitCode === expectedExitCode) {
49+
export function logAndThrowIfUnexpectedExitCode(r: ChildProcessResult, expectedExitCode: number): void {
50+
if (r.exitCode === expectedExitCode) {
4251
return
4352
}
4453

45-
const logger = getLogger()
46-
47-
logger.error(`Unexpected exitcode (${processResult.exitCode}), expecting (${expectedExitCode})`)
48-
logger.error(`Error: ${processResult.error}`)
49-
logger.error(`stderr: ${processResult.stderr}`)
50-
logger.error(`stdout: ${processResult.stdout}`)
54+
const errIndented = r.stderr.replace(/\n/g, '\n ').trim()
55+
const outIndented = r.stdout.replace(/\n/g, '\n ').trim()
5156

52-
let message: string | undefined
57+
getLogger().error(`SAM CLI failed (exitcode: ${r.exitCode}, expected ${expectedExitCode}): ${r.error?.message ?? ''}
58+
stdout:
59+
${outIndented}
60+
stderr:
61+
${errIndented}
62+
`)
5363

54-
if (processResult.error instanceof Error) {
55-
if (processResult.error.message) {
56-
message = processResult.error.message
57-
}
58-
}
59-
const usefulErrors = collectAcceptedErrorMessages(processResult.stderr).join('\n')
60-
throw makeUnexpectedExitCodeError(message ?? usefulErrors)
64+
const message = r.error instanceof Error ? r.error.message : collectSamErrors(r.stderr).join('\n')
65+
throw makeUnexpectedExitCodeError(message)
6166
}
6267

6368
/**
64-
* Collect known errors messages from a sam error message
65-
* that may have multiple errors in one message.
66-
* @param errors A string that can have multiple error messages
69+
* Collect known error messages from sam cli output, so they can be surfaced to the user.
70+
*
71+
* @param samOutput SAM CLI output containing potential error messages
6772
*/
68-
export function collectAcceptedErrorMessages(errorMessage: string): string[] {
69-
const errors = errorMessage.split('\n')
70-
const shouldCollectFuncs = [startsWithEscapeSequence, startsWithError]
71-
return errors.filter(error => {
72-
return shouldCollectFuncs.some(shouldCollect => {
73-
return shouldCollect(error)
74-
})
75-
})
73+
export function collectSamErrors(samOutput: string): string[] {
74+
const lines = samOutput.split('\n')
75+
const matchedLines: string[] = []
76+
const matchers = [matchSamError, matchAfterEscapeSeq]
77+
for (const line of lines) {
78+
for (const m of matchers) {
79+
const match = m(line)
80+
if (match && match.trim() !== '') {
81+
matchedLines.push(match)
82+
break // Skip remaining matchers, go to next line.
83+
}
84+
}
85+
}
86+
return matchedLines
7687
}
7788

78-
/**
79-
* All accepted escape sequences.
80-
*/
89+
/** All accepted escape sequences. */
8190
const yellowForeground = '[33m'
8291
const acceptedSequences = [yellowForeground]
8392

84-
/**
85-
* Returns true if text starts with an escape
86-
* sequence with one of the accepted sequences.
87-
*/
88-
function startsWithEscapeSequence(text: string, sequences = acceptedSequences): boolean {
93+
/** Returns text after a known escape sequence, or empty string. */
94+
function matchAfterEscapeSeq(text: string, sequences = acceptedSequences): string {
95+
text = text.trim()
8996
const escapeInDecimal = 27
9097
if (text.charCodeAt(0) !== escapeInDecimal) {
91-
return false
98+
return ''
9299
}
93100

94101
const remainingText = text.substring(1)
95-
return sequences.some(code => {
96-
return remainingText.startsWith(code)
97-
})
102+
for (const seq of sequences) {
103+
if (remainingText.startsWith(seq)) {
104+
return remainingText.substring(seq.length).trim()
105+
}
106+
}
107+
return ''
98108
}
99109

100-
function startsWithError(text: string): boolean {
101-
return text.startsWith('Error:')
110+
function matchSamError(text: string): string {
111+
// These should be ordered by "specificity", to make the result more relevant for users.
112+
const patterns = [
113+
/\s*(Running.*requires Docker\.?)/,
114+
/\s*(Docker.*not reachable\.?)/,
115+
/\bError:\s*(.*)/, // Goes last because it is the least specific.
116+
]
117+
for (const re of patterns) {
118+
const match = text.match(re)
119+
if (match?.[1]) {
120+
// Capture group 1 is the first (…) group in the regex pattern.
121+
return match[1].trim() // Return _only_ the matched text. The rest is noise.
122+
}
123+
}
124+
return ''
102125
}
103126

104127
export async function addTelemetryEnvVar(options: SpawnOptions | undefined): Promise<SpawnOptions> {

‎src/shared/sam/cli/samCliLocalInvoke.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { removeAnsi } from '../../utilities/textUtilities'
1414
import * as vscode from 'vscode'
1515
import globals from '../../extensionGlobals'
1616
import { SamCliSettings } from './samCliSettings'
17-
import { addTelemetryEnvVar } from './samCliInvokerUtils'
17+
import { addTelemetryEnvVar, collectSamErrors, SamCliError } from './samCliInvokerUtils'
1818

1919
const localize = nls.loadMessageBundle()
2020

@@ -122,7 +122,13 @@ export class DefaultSamLocalInvokeCommand implements SamLocalInvokeCommand {
122122
if (code === 0) {
123123
resolve()
124124
} else if (code !== 0) {
125-
reject(new Error(`"${samCommandName}" command stopped (error code: ${code})`))
125+
const samErrors = collectSamErrors(result.stderr)
126+
if (samErrors.length > 0) {
127+
const e = new SamCliError(samErrors.join('\n'))
128+
reject(e)
129+
} else {
130+
reject(new Error(`"${samCommandName}" command stopped (error code: ${code})`))
131+
}
126132
}
127133

128134
// Forces debugger to disconnect (sometimes it fails to disconnect on its own)

‎src/shared/sam/localLambdaRunner.ts‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import { CloudFormation } from '../cloudformation/cloudformation'
3030
import { sleep } from '../utilities/timeoutUtils'
3131
import { showMessageWithCancel } from '../utilities/messages'
3232
import { ToolkitError, UnknownError } from '../errors'
33+
import { SamCliError } from './cli/samCliInvokerUtils'
3334

3435
const localize = nls.loadMessageBundle()
3536

@@ -159,11 +160,11 @@ async function buildLambdaHandler(
159160
const failure = err ?? samBuild.failure()
160161
if (failure) {
161162
// TODO(sijaden): ask SAM CLI for a way to map exit codes to error codes
162-
// We can also scrape the debug output though that won't be as reliable
163163
if (typeof failure === 'string') {
164164
throw new ToolkitError(failure, { code: 'BuildFailure' })
165165
} else {
166-
throw ToolkitError.chain(failure, 'Failed to build SAM application', { code: 'BuildFailure' })
166+
const msg = `SAM build failed${err instanceof SamCliError ? ': ' + err.message : ''}`
167+
throw ToolkitError.chain(err, msg, { code: 'BuildFailure' })
167168
}
168169
}
169170
getLogger('channel').info(localize('AWS.output.building.sam.application.complete', 'Build complete.'))
@@ -222,10 +223,10 @@ async function invokeLambdaHandler(
222223
timeout: timer,
223224
name: config.name,
224225
})
225-
.catch(e => {
226-
const message = localize('AWS.error.during.apig.local', 'Failed to start local API Gateway')
226+
.catch(err => {
227+
const msg = `SAM local start-api failed${err instanceof SamCliError ? ': ' + err.message : ''}`
227228

228-
throw ToolkitError.chain(e, message)
229+
throw ToolkitError.chain(err, msg)
229230
})
230231
} else {
231232
// 'target=code' or 'target=template'
@@ -253,10 +254,8 @@ async function invokeLambdaHandler(
253254
try {
254255
return await command.execute(timer)
255256
} catch (err) {
256-
throw ToolkitError.chain(
257-
err,
258-
localize('AWS.error.during.sam.local', 'Failed to run SAM application locally')
259-
)
257+
const msg = `SAM local invoke failed${err instanceof SamCliError ? ': ' + err.message : ''}`
258+
throw ToolkitError.chain(err, msg)
260259
} finally {
261260
if (config.sam?.buildDir === undefined) {
262261
await remove(config.templatePath)
@@ -277,13 +276,6 @@ export async function runLambdaFunction(
277276
config: SamLaunchRequestArgs,
278277
onAfterBuild: () => Promise<void>
279278
): Promise<SamLaunchRequestArgs> {
280-
// Verify if Docker is running
281-
const dockerResponse = await new ChildProcess('docker', ['ps'], { logging: 'no' }).run()
282-
if (dockerResponse.exitCode !== 0 || dockerResponse.stdout.includes('error during connect')) {
283-
throw new ToolkitError('Running AWS SAM projects locally requires Docker. Is it installed and running?', {
284-
code: 'NoDocker',
285-
})
286-
}
287279
// Switch over to the output channel so the user has feedback that we're getting things ready
288280
ctx.outputChannel.show(true)
289281
if (!config.noDebug) {

‎src/test/shared/sam/cli/samCliInvokerUtils.test.ts‎

Lines changed: 23 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import * as assert from 'assert'
77
import {
88
addTelemetryEnvVar,
9-
collectAcceptedErrorMessages,
9+
collectSamErrors,
1010
logAndThrowIfUnexpectedExitCode,
1111
makeUnexpectedExitCodeError,
1212
} from '../../../../shared/sam/cli/samCliInvokerUtils'
@@ -45,41 +45,32 @@ describe('logAndThrowIfUnexpectedExitCode', async function () {
4545
})
4646
})
4747

48-
/**
49-
* Returns a string with the 'Escape' character
50-
* prepended to the given text.
51-
*
52-
* This exists because using '\e' does not
53-
* work.
54-
*/
48+
/** Prepends ESC control character to `text`. */
5549
function prependEscapeCode(text: string): string {
5650
return String.fromCharCode(27) + text
5751
}
5852

59-
describe('collectAcceptedErrorMessages()', async () => {
60-
let result: string[]
61-
62-
before(async () => {
63-
const input = [
64-
prependEscapeCode('[33m This is an accepted escape sequence'),
65-
prependEscapeCode('[100m This is not an accepted escape sequence'),
66-
'This will be ignored',
67-
'Error: This is accepted due to the prefix',
68-
].join('\n')
69-
result = collectAcceptedErrorMessages(input)
70-
})
71-
72-
it('has the expected amount of messages', async () => {
73-
assert.strictEqual(result.length, 2)
74-
})
75-
it('collects the "Error:" prefix', async () => {
76-
assert(result.includes('Error: This is accepted due to the prefix'))
77-
})
78-
it('collects accepted escape sequence prefixes', async () => {
79-
assert(result.includes(prependEscapeCode('[33m This is an accepted escape sequence')))
80-
})
81-
it('ignores non-accepted escape sequence prefixes', async () => {
82-
assert(!result.includes(prependEscapeCode('[100m This is not an accepted escape sequence')))
53+
describe('collectSamErrors()', async () => {
54+
it('collects messages', async () => {
55+
const input = `
56+
This line is ignored
57+
foo bar Error: xxx Docker is not reachable!!
58+
another line
59+
foo Error: user is bored Error:
60+
Error: Running AWS SAM projects locally requires Docker.
61+
'urllib3.exceptions.ProtocolError: ('Connection aborted.', FileNotFoundError(2, 'No such file or directory'))'
62+
ok
63+
${prependEscapeCode('[33m known escape sequence')}
64+
ok again
65+
${prependEscapeCode('[100m unknown escape sequence')}
66+
`
67+
const result = collectSamErrors(input)
68+
assert.deepStrictEqual(result, [
69+
'Docker is not reachable',
70+
'user is bored Error:',
71+
'Running AWS SAM projects locally requires Docker.',
72+
'known escape sequence',
73+
])
8374
})
8475
})
8576

‎src/test/shared/sam/cli/testSamCliProcessInvoker.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,12 +94,9 @@ export async function assertLogContainsBadExitInformation(
9494
): Promise<void> {
9595
const expectedTexts = [
9696
{
97-
text: `Unexpected exitcode (${errantChildProcessResult.exitCode}), expecting (${expectedExitCode})`,
97+
text: `SAM CLI failed (exitcode: ${errantChildProcessResult.exitCode}, expected ${expectedExitCode}`,
9898
verifyMessage: 'Log message missing for exit code',
9999
},
100-
{ text: `Error: ${errantChildProcessResult.error}`, verifyMessage: 'Log message missing for error' },
101-
{ text: `stderr: ${errantChildProcessResult.stderr}`, verifyMessage: 'Log message missing for stderr' },
102-
{ text: `stdout: ${errantChildProcessResult.stdout}`, verifyMessage: 'Log message missing for stdout' },
103100
]
104101

105102
const logText = logger

0 commit comments

Comments
 (0)