fix: builds abort, runs --status filter, and dataset command correctness - #1316
fix: builds abort, runs --status filter, and dataset command correctness#1316Ayush7614 wants to merge 5 commits into
Conversation
Add builds abort parity with runs, --status on runs ls, correct dataset info messaging/pricing, richer get-items export options, and rename help text fixes.
l2ysho
left a comment
There was a problem hiding this comment.
thx @Ayush7614, here are some quick findings
| expect(result.exitCode).not.toBe(0); | ||
| expect(result.stdout).toContain('Build with ID "invalid-id" was not found'); |
There was a problem hiding this comment.
These two assertions are mutually exclusive, so this test can't pass.
src/commands/builds/log.ts replaced throw new Error(...) with error({ ... }); return. error() (src/lib/outputs.ts) only logs — the non-zero exit code came from the framework's catch in src/lib/command-framework/apify-command.ts. So apify builds log <bad-id> now exits 0 and line 192 fails.
Which branch is live: apify-client's _get swallows 404 into undefined, so !build is the path taken here — that's confirmed by the untouched, currently-green builds info test above (fails with invalid build ID), which runs identical code. So stdout will match and only the exit-code assertion fails. If .get() did reject instead, line 193 would be the one to fail. Either way the pair can't both hold.
Two ways out:
- drop line 192 and assert stdout only (this is what the new
builds aborttests do, correctly), or - set
process.exitCode = 1before thereturninlog.tsand keep both.
I'd prefer the second — losing the non-zero exit code on "build not found" is a user-visible regression for anyone scripting this.
| try { | ||
| await outputJobLog({ job: build, apifyClient }); | ||
| } catch (err) { | ||
| error({ message: `Failed to get log for build with ID "${buildId}": ${(err as Error).message}` }); | ||
| } |
There was a problem hiding this comment.
This swallows log-fetch failures and reports success. Before, an exception from outputJobLog propagated and the command exited 1. Now, if the build exists but client.log(id).get() 5xx's or the log is gone, the user gets Info: Log for build with ID "x": on stdout, Error: Failed to get log… on stderr, and $? == 0.
Concretely: apify builds log $ID > build.log && next-step proceeds with an empty file.
I see this was copied from src/commands/runs/log.ts — but there the swallow is annotated (// This should never happen..., dropped in the copy), and regardless it's a new regression for builds log. Either rethrow, or process.exitCode = 1 in the catch.
Minor inconsistency while you're here: line 35 sends its error to stdout (stdout: true) and line 44 to stderr, in the same command.
|
@l2ysho thanks for the catch — addressed in the latest commit:
|
Summary
apify builds abortfor parity withruns abort(platform build abort already existed via signal handling, but had no dedicated command).apify runs ls --statusso runs can be filtered by job status (API-supported).datasets info; corrects misleading rename help text for datasets and key-value stores.datasets get-itemswith--fields,--omit,--unwind,--clean,--desc, and--output, and routes resolution through sharedtryToGetDataset.builds lognot-found handling withruns log, regenerates docs, and extends e2e/local coverage.These are independent fixes/features with no claimed open issues.
Test plan
pnpm run lintpnpm run formatpnpm run buildpnpm run test:local(377 passed, including newstorage-sizeunit tests)TEST_USER_TOKEN