-
Notifications
You must be signed in to change notification settings - Fork 49
feat(push): add final status block and structured JSON output #1199
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
Changes from all commits
1122ce2
6322f2c
e3cb16f
b55a4e3
913b8ba
19d3d09
33a95ac
8118d1a
a98e11f
4f5ec32
22b1854
d3c49a4
79175e9
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 |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ import { CommandExitCodes, DEPRECATED_LOCAL_CONFIG_NAME, LOCAL_CONFIG_PATH } fro | |
| import { sumFilesSizeInBytes } from '../../lib/files.js'; | ||
| import { useAbortJobOnSignal } from '../../lib/hooks/useAbortJobOnSignal.js'; | ||
| import { useActorConfig } from '../../lib/hooks/useActorConfig.js'; | ||
| import { error, info, link, run, success, warning } from '../../lib/outputs.js'; | ||
| import { error, info, run, simpleLog, warning } from '../../lib/outputs.js'; | ||
| import { transformEnvToEnvVars } from '../../lib/secrets.js'; | ||
| import { | ||
| createActZip, | ||
|
|
@@ -48,6 +48,79 @@ const DEFAULT_ACTOR_VERSION_NUMBER = '0.0'; | |
| // that changes, we have to add it. | ||
| const DEFAULT_BUILD_TAG = 'latest'; | ||
|
|
||
| // How many trailing log lines to surface as the failure reason. | ||
| const BUILD_LOG_TAIL_LINES = 10; | ||
|
|
||
| // TODO: switch to `type` once the `consistent-type-definitions` lint rule is | ||
| // aligned with the Apify Coding Standards (tracked in | ||
| // https://github.com/apify/apify-cli/issues/1211). | ||
| interface PushResult { | ||
| ok: boolean; | ||
| operation: 'push'; | ||
| actor: { id: string; url: string }; | ||
| build: { id: string; number: string; status: string; url: string }; | ||
| error?: { phase: 'build'; message: string; logTail: string[] }; | ||
| exitCode?: number; | ||
| } | ||
|
DaveHanns marked this conversation as resolved.
|
||
|
|
||
| interface PushOutcome { | ||
| resultLabel: string; | ||
| exitCode?: number; | ||
| ok: boolean; | ||
| errorMessage?: string; | ||
| } | ||
|
|
||
| // Maps the final build status to the overall push outcome. A still-running | ||
| // fire-and-forget build is not a failure (`ok: true`) — its pending state is | ||
| // conveyed by the build status, and it carries no exit code yet. | ||
| export function resolvePushOutcome(buildStatus: string): PushOutcome { | ||
| switch (buildStatus) { | ||
| case ACTOR_JOB_STATUSES.SUCCEEDED: | ||
| return { resultLabel: 'SUCCEEDED', exitCode: 0, ok: true }; | ||
| case ACTOR_JOB_STATUSES.READY: | ||
| return { resultLabel: 'PENDING', ok: true }; | ||
| case ACTOR_JOB_STATUSES.RUNNING: | ||
| return { resultLabel: 'RUNNING', ok: true }; | ||
| case ACTOR_JOB_STATUSES.ABORTING: | ||
| return { | ||
| resultLabel: 'ABORTING', | ||
| exitCode: CommandExitCodes.BuildAborted, | ||
| ok: false, | ||
| errorMessage: 'Build is aborting', | ||
| }; | ||
| case ACTOR_JOB_STATUSES.ABORTED: | ||
| return { | ||
| resultLabel: 'ABORTED', | ||
| exitCode: CommandExitCodes.BuildAborted, | ||
| ok: false, | ||
| errorMessage: 'Build aborted', | ||
| }; | ||
| case ACTOR_JOB_STATUSES.TIMING_OUT: | ||
| return { | ||
| resultLabel: 'TIMING_OUT', | ||
| exitCode: CommandExitCodes.BuildTimedOut, | ||
| ok: false, | ||
| errorMessage: 'Build is timing out', | ||
| }; | ||
| case ACTOR_JOB_STATUSES.TIMED_OUT: | ||
| return { | ||
| resultLabel: 'TIMED_OUT', | ||
| exitCode: CommandExitCodes.BuildTimedOut, | ||
| ok: false, | ||
| errorMessage: 'Build timed out', | ||
| }; | ||
|
DaveHanns marked this conversation as resolved.
|
||
| case ACTOR_JOB_STATUSES.FAILED: | ||
| return { resultLabel: 'FAILED', exitCode: CommandExitCodes.BuildFailed, ok: false, errorMessage: 'Build failed' }; | ||
|
DaveHanns marked this conversation as resolved.
|
||
| default: | ||
| return { | ||
| resultLabel: 'UNKNOWN', | ||
| exitCode: CommandExitCodes.BuildFailed, | ||
| ok: false, | ||
| errorMessage: `Build finished with unexpected status "${buildStatus}"`, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export class ActorsPushCommand extends ApifyCommand<typeof ActorsPushCommand> { | ||
| static override name = 'push' as const; | ||
|
|
||
|
|
@@ -424,44 +497,70 @@ Skipping push. Use --force to override.`, | |
| } | ||
| } | ||
|
|
||
| if (this.flags.json) { | ||
| printJsonToStdout(build); | ||
| return; | ||
| const buildStatus = build.status as string; | ||
| const outcome = resolvePushOutcome(buildStatus); | ||
|
|
||
| const actorUrl = `https://console.apify.com${redirectUrlPart}/actors/${build.actId}`; | ||
|
DaveHanns marked this conversation as resolved.
|
||
| const buildUrl = `${actorUrl}#/builds/${build.buildNumber}`; | ||
|
|
||
| // Surface the tail of the build log as the failure reason. Best-effort: | ||
| // the build status already conveys the outcome if the log can't be read. | ||
| let logTail: string[] = []; | ||
| if (outcome.errorMessage) { | ||
| try { | ||
| const log = await apifyClient.log(build.id).get(); | ||
| if (log) { | ||
| logTail = log | ||
| .split('\n') | ||
| .map((line) => line.trimEnd()) | ||
| .filter((line) => line.length > 0) | ||
| .slice(-BUILD_LOG_TAIL_LINES); | ||
| } | ||
| } catch { | ||
| // ignore — reason block is optional | ||
| } | ||
| } | ||
|
|
||
| link({ | ||
| message: 'Actor build detail', | ||
| url: `https://console.apify.com${redirectUrlPart}/actors/${build.actId}#/builds/${build.buildNumber}`, | ||
| }); | ||
| if (outcome.exitCode) { | ||
| process.exitCode = outcome.exitCode; | ||
| } | ||
|
|
||
| link({ | ||
| message: 'Actor detail', | ||
| url: `https://console.apify.com${redirectUrlPart}/actors/${build.actId}`, | ||
| }); | ||
| const result: PushResult = { | ||
| ok: outcome.ok, | ||
| operation: 'push', | ||
| actor: { id: build.actId, url: actorUrl }, | ||
| build: { id: build.id, number: build.buildNumber, status: buildStatus, url: buildUrl }, | ||
| }; | ||
| if (outcome.exitCode !== undefined) { | ||
| result.exitCode = outcome.exitCode; | ||
| } | ||
| if (outcome.errorMessage) { | ||
| result.error = { phase: 'build', message: outcome.errorMessage, logTail }; | ||
| } | ||
|
|
||
| if (this.flags.open) { | ||
| await open(`https://console.apify.com${redirectUrlPart}/actors/${build.actId}`); | ||
| if (this.flags.json) { | ||
| printJsonToStdout(result); | ||
| return; | ||
| } | ||
|
|
||
| if (build.status === ACTOR_JOB_STATUSES.SUCCEEDED) { | ||
| success({ message: 'Actor was deployed to Apify cloud and built there.' }); | ||
| // @ts-expect-error FIX THESE TYPES 😢 | ||
| } else if (build.status === ACTOR_JOB_STATUSES.READY) { | ||
| warning({ message: 'Build is waiting for allocation.' }); | ||
| // @ts-expect-error FIX THESE TYPES 😢 | ||
| } else if (build.status === ACTOR_JOB_STATUSES.RUNNING) { | ||
| warning({ message: 'Build is still running.' }); | ||
| // @ts-expect-error FIX THESE TYPES 😢 | ||
| } else if (build.status === ACTOR_JOB_STATUSES.ABORTED || build.status === ACTOR_JOB_STATUSES.ABORTING) { | ||
| warning({ message: 'Build was aborted!' }); | ||
| process.exitCode = CommandExitCodes.BuildAborted; | ||
| // @ts-expect-error FIX THESE TYPES 😢 | ||
| } else if (build.status === ACTOR_JOB_STATUSES.TIMED_OUT || build.status === ACTOR_JOB_STATUSES.TIMING_OUT) { | ||
| warning({ message: 'Build timed out!' }); | ||
| process.exitCode = CommandExitCodes.BuildTimedOut; | ||
| } else { | ||
| error({ message: 'Build failed!' }); | ||
| process.exitCode = CommandExitCodes.BuildFailed; | ||
| const lines = [ | ||
|
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. Q: do we already implement same style logging in case the upload itself fails?
Contributor
Author
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. not now, It is not so big change but I would like to merge this PR and do it in followup, upload fails should not be common if platform is not in some serious problems (or user is in some isolated env).
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. Makes sense 👍 You can either use the same issue (in that case don't close it with this PR) or create a new one for that once you tackle it. |
||
| `Apify push result: ${outcome.resultLabel}`, | ||
| '', | ||
| 'Upload: SUCCEEDED', | ||
| `Build: ${buildStatus}`, | ||
| `Actor ID: ${build.actId}`, | ||
| `Build ID: ${build.id}`, | ||
| `Build number: ${build.buildNumber}`, | ||
| ...(outcome.exitCode ? [`Exit code: ${outcome.exitCode}`] : []), | ||
| '', | ||
| `Actor URL: ${actorUrl}`, | ||
| `Build URL: ${buildUrl}`, | ||
| ...(outcome.errorMessage && logTail.length ? ['', 'Reason:', ...logTail] : []), | ||
| ]; | ||
| simpleLog({ stdout: true, message: lines.join('\n') }); | ||
|
|
||
| if (this.flags.open) { | ||
| await open(actorUrl); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { ACTOR_JOB_STATUSES } from '@apify/consts'; | ||
|
|
||
| import { resolvePushOutcome } from '../../../src/commands/actors/push.js'; | ||
| import { CommandExitCodes } from '../../../src/lib/consts.js'; | ||
|
|
||
| describe('resolvePushOutcome', () => { | ||
| test.each([ | ||
| [ACTOR_JOB_STATUSES.SUCCEEDED, { resultLabel: 'SUCCEEDED', exitCode: 0, ok: true }], | ||
| [ACTOR_JOB_STATUSES.READY, { resultLabel: 'PENDING', ok: true }], | ||
| [ACTOR_JOB_STATUSES.RUNNING, { resultLabel: 'RUNNING', ok: true }], | ||
| [ACTOR_JOB_STATUSES.ABORTING, { resultLabel: 'ABORTING', exitCode: CommandExitCodes.BuildAborted, ok: false }], | ||
| [ACTOR_JOB_STATUSES.ABORTED, { resultLabel: 'ABORTED', exitCode: CommandExitCodes.BuildAborted, ok: false }], | ||
| [ACTOR_JOB_STATUSES.TIMING_OUT, { resultLabel: 'TIMING_OUT', exitCode: CommandExitCodes.BuildTimedOut, ok: false }], | ||
| [ACTOR_JOB_STATUSES.TIMED_OUT, { resultLabel: 'TIMED_OUT', exitCode: CommandExitCodes.BuildTimedOut, ok: false }], | ||
| [ACTOR_JOB_STATUSES.FAILED, { resultLabel: 'FAILED', exitCode: CommandExitCodes.BuildFailed, ok: false }], | ||
| ['SOME_UNKNOWN_STATUS', { resultLabel: 'UNKNOWN', exitCode: CommandExitCodes.BuildFailed, ok: false }], | ||
| ])('maps build status %s to the expected outcome', (status, expected) => { | ||
| expect(resolvePushOutcome(status)).toMatchObject(expected); | ||
| }); | ||
|
|
||
| test('in-progress builds carry no exit code yet, terminal ones do', () => { | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.READY).exitCode).toBeUndefined(); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.RUNNING).exitCode).toBeUndefined(); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.SUCCEEDED).exitCode).toBe(0); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.ABORTING).exitCode).toBe(CommandExitCodes.BuildAborted); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.FAILED).exitCode).toBe(CommandExitCodes.BuildFailed); | ||
| }); | ||
|
|
||
| test('failing outcomes carry an error message, successful ones do not', () => { | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.SUCCEEDED).errorMessage).toBeUndefined(); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.READY).errorMessage).toBeUndefined(); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.RUNNING).errorMessage).toBeUndefined(); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.ABORTING).errorMessage).toBe('Build is aborting'); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.ABORTED).errorMessage).toBe('Build aborted'); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.TIMING_OUT).errorMessage).toBe('Build is timing out'); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.TIMED_OUT).errorMessage).toBe('Build timed out'); | ||
| expect(resolvePushOutcome(ACTOR_JOB_STATUSES.FAILED).errorMessage).toBe('Build failed'); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.