uranusjr commented on code in PR #71144:
URL: https://github.com/apache/airflow/pull/71144#discussion_r3793840334
##########
ts-sdk/src/cli/pack.ts:
##########
Review Comment:
Since key validation moved to buildBundleManifest, this catch is now the
only place a bad Dag or task ID is reported. `String(error)` makes it hard to
read. Packing `new Dag("bad id!")` currently gives:
```
Running the bundle with --airflow-metadata failed: Error: Command failed:
/usr/bin/node /tmp/out/bundle.pack-staging.mjs --airflow-metadata
...
Error: Dag "bad id!" must be made of alphanumeric characters, dashes, dots,
and underscores
at validateKey (file:///tmp/out/bundle.pack-staging.mjs:2527:11)
...
```
The one actionable sentence is buried, and every frame points into
bundle.pack-staging.mjs, which the finally block deletes — so no line the trace
names can be opened.
`execFileSync` puts the child's stderr on error.stderr, so the message is
already available here:
```ts
} catch (error) {
const stderr = (error as { stderr?: string }).stderr ?? "";
const reported = stderr.split("\n").find((l) => /^\w*Error:
/.test(l.trim()))?.trim();
throw new Error(
reported ?? `Running the bundle with ${AIRFLOW_METADATA_FLAG} failed:
${String(error)}`,
{ cause: error },
);
}
```
Worth considering to catch inside `--airflow-metadata` mode and emit the
failure on the sentinel channel. That drops the stack entirely and lets pack
distinguish "this Dag ID is invalid" from "the bundle threw on import", which
are indistinguishable today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
