-
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
base: master
Are you sure you want to change the base?
Changes from all commits
1122ce2
6322f2c
e3cb16f
b55a4e3
913b8ba
19d3d09
33a95ac
8118d1a
a98e11f
4f5ec32
22b1854
d3c49a4
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; | ||
| } | ||
|
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. Nit: to avoid type-checking for optional
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. see #1199 (comment)
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. Yeah :( Please, add a TODO comment for later then. Thanks.
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. Technically, ... is possible, but still you get a
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. TODO added |
||
|
|
||
| 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', | ||
| }; | ||
|
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: similarly as above, why don't we have distinguished
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. updated |
||
| case ACTOR_JOB_STATUSES.FAILED: | ||
| return { resultLabel: 'FAILED', exitCode: CommandExitCodes.BuildFailed, ok: false, errorMessage: 'Build failed' }; | ||
|
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. Nit: not super sure about the default. If there is ever a new state, it might fall back to FAILED even if it is not.
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. well yes, I think we can move FAILED to its own case statement and introduce UKNOWN as default?
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. Yeah, sounds better.
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. UKNOWN introduced |
||
| 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}`; | ||
|
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. Nit: it is alright now, but I noticed we are hardcoding apify domain (api, console, ...) a lot in the cli. At some point we should develop a registry of domains so that we have a single source of truth. Ignore as it is out of scope of this PR. |
||
| 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). |
||
| `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'); | ||
| }); | ||
| }); |
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.
Nit: prefer
typeoverinterface.See https://www.notion.so/apify/Apify-Coding-Standards-Guide-264f39950a2280588be3f2268b9cb609?source=copy_link#264f39950a2280898b98c84f9342d686
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.
for some reason we use
'typescript/consistent-type-definitions': ['error', 'interface'],(even before oxlint migration). We use @apify/oxlint-config but there is few additional changes in it, I guess lets align this with Apify Coding Standards in separate PR (too much noise to change all interfaces to types here).@vladfrangu do you have some context about this type/interface decision ?
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, agree. Please, create an issue in GH. I'll plan it for a next sprint.
I'm also wondering how did we get this far with disallowed
type:DThere 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.
#1211