Skip to content

fix: api-key field names, dataset readiness check, punycode warning#14

Merged
lukekim merged 2 commits into
trunkfrom
fix/punycode-deprecation
May 3, 2026
Merged

fix: api-key field names, dataset readiness check, punycode warning#14
lukekim merged 2 commits into
trunkfrom
fix/punycode-deprecation

Conversation

@lukekim
Copy link
Copy Markdown
Contributor

@lukekim lukekim commented May 3, 2026

Summary

Three issues that all came out of a real failed run on the lukekim/home workflow:

```
Using app "home" (id=4573).
PUT /v1/apps/4573 tags={...}

status: in_progress → succeeded
##[group]Run 1 runtime probe(s)
##[error]Cannot run runtime probes: no API key returned for app 4573.
```

… plus the persistent (node:NNNN) [DEP0040] DeprecationWarning: The 'punycode' module is deprecated warning every workflow has been printing.

1. Fix getApiKeys field names (the actual blocker)

`GET /v1/apps/{id}/api-keys` returns `{ api_key, api_key_2 }` per the OpenAPI schema, but the action was reading `{ primary, secondary }`. Both fields were always undefined → smoke tests bailed every run with "no API key returned". Updated `SpiceApiClient.getApiKeys` and the deploy code that consumes it.

The OAuth scopes documented in the README are correct (apps:read is enough to fetch the app's API key). The user's report wasn't a scope problem — it was this parsing bug.

2. Dataset readiness check (/v1/datasets?status=true)

Spice runtime can report `succeeded` for the deployment while datasets are still loading or in an error state. After `isSpiceReady()` succeeds, the action now polls `GET /v1/datasets?status=true` until every dataset leaves `initializing`, fails immediately on `error`, and surfaces each dataset's status in the step summary.

Status Action treats as
`ready`, `disabled`, `refreshing` terminal, OK
`initializing` keep polling
`error` fail (with each dataset's `error_message`)
`shuttingdown` shouldn't happen post-deploy; treated like initializing

Status comparison is case-insensitive — the public docs list lowercase values but the OSS runtime examples emit PascalCase, so we normalize.

New input: `dataset-ready-timeout-seconds` (default `300`, `0` skips). New action output: `datasets` (JSON array of `{ name, status, from?, error?, error_message? }`). New step-summary table.

3. `DEP0040` punycode deprecation warning

The transitive chain `@spiceai/spice → node-fetch@2 → whatwg-url@5 → tr46@0.0.3` calls `require("punycode")`, which Node resolves to the deprecated built-in. Installed the userland `punycode@^2` package and added `--alias:punycode=./node_modules/punycode/punycode.js` to the esbuild build script. The rebuilt bundle now bundles the userland implementation; running `node dist/index.js` no longer prints the warning.

Test plan

  • `npm run all` — 90 tests passing (up from 83), lint/typecheck/build/dist freshness all clean.
  • `node dist/index.js` no longer prints `DEP0040`.
  • CI green on this PR.
  • Manual: re-run the `lukekim/home` workflow on the merged commit and confirm:
    • No `DEP0040` warning.
    • Smoke test runs (no "no API key returned") and reports per-dataset state.
    • Step summary shows the "Datasets" table.

Out of scope / docs

  • The OAuth scope cheat sheet doesn't change — `apps:read` already covers `GET /v1/apps/{id}/api-keys` and the runtime API key works for the dataset check.

Three bugs/improvements that all surfaced from a real workflow run.

1. Fix api-key parsing
   GET /v1/apps/{id}/api-keys returns `{ api_key, api_key_2 }` per the
   OpenAPI schema, but the action was reading `{ primary, secondary }`,
   so smoke tests bailed with `Cannot run runtime probes: no API key
   returned for app …` immediately after a successful deploy. Update
   `getApiKeys` to return `{ api_key, api_key_2 }` and select the
   primary (or the secondary as a fallback).

2. Add dataset readiness check
   After the deployment reports `succeeded` and the runtime answers
   `isSpiceReady()`, poll `GET /v1/datasets?status=true` (per
   https://spiceai.org/docs/api/HTTP/get-datasets) until every dataset
   leaves the `initializing` state. Fail immediately if any dataset
   enters `error`, surfacing the `error_message` so the user knows
   which dataset is broken. New input `dataset-ready-timeout-seconds`
   (default 300, set to 0 to skip), new `datasets` action output, and
   a "Datasets" table in the GitHub job step summary. Status comparison
   is case-insensitive so the action handles both the lowercase values
   the public docs list and the PascalCase values the OSS runtime emits
   in its OpenAPI examples.

3. Stop the DEP0040 punycode warning
   `@spiceai/spice → node-fetch@2 → whatwg-url@5 → tr46@0.0.3` all
   `require("punycode")`, which Node resolves to its deprecated
   built-in. Install the userland `punycode@^2` package and add an
   esbuild `--alias:punycode=…` flag so the bundle ships the userland
   implementation instead.

Tests: 90 passing (up from 83). New cases cover api-key field-name
parsing, the runtime dataset poll loop (success / error / timeout /
no-op), and the deploy orchestration paths for both successful and
failing dataset checks.
Copilot AI review requested due to automatic review settings May 3, 2026 00:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a deployment-action regression where API keys were never parsed, adds a post-deploy dataset readiness gate to avoid probing an incompletely loaded runtime, and removes the Node DEP0040 punycode deprecation warning from the bundled action.

Changes:

  • Parse /v1/apps/{id}/api-keys response fields as { api_key, api_key_2 } and propagate through deploy flow.
  • Add runtime dataset polling via GET /v1/datasets?status=true, new input dataset-ready-timeout-seconds, new output datasets, and step-summary reporting.
  • Bundle userland punycode via esbuild aliasing to suppress DEP0040 warnings.

Reviewed changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/api.ts Updates getApiKeys return shape to match the API response fields.
src/deploy.ts Runs “post-deploy checks” (dataset readiness + probes) and returns datasets in deploy result.
src/runtime.ts Adds dataset fetch + polling logic and a dedicated DatasetReadinessError.
src/inputs.ts Adds datasetReadyTimeoutSeconds input parsing with default and validation.
src/main.ts Exposes datasets as an action output and renders a datasets table in the step summary.
action.yml Documents new input/output and defaults for dataset readiness checking.
tests/runtime.test.ts Adds coverage for dataset fetch/poll behavior and error/timeout paths.
tests/deploy.test.ts Updates API key mocks and adds dataset readiness integration tests.
package.json / package-lock.json Adds punycode dependency and esbuild alias in the build script.
README.md Documents new input/output in user-facing docs.
CHANGELOG.md Records the new dataset readiness feature and API key parsing fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/runtime.ts Outdated
Comment thread src/deploy.ts
- Explicitly classify dataset statuses into ok/error/pending buckets so
  `shuttingdown` and any unrecognized values are treated as still-pending
  rather than slipping through as a false-positive "loaded". Only
  `ready`/`disabled`/`refreshing` count as terminal-ok; only `error`
  short-circuits with `DatasetReadinessError`. Timeout messages now
  include each non-terminal-ok dataset's status so the user can tell
  `initializing` from `shuttingdown` from a typo.
- Make dataset readiness failures always fatal once the check is opted in
  (`dataset-ready-timeout-seconds > 0`), independent of
  `fail-on-test-error` (which only governs runtime-probe results). Match
  action.yml/README/CHANGELOG to that behavior. The opt-out is
  `dataset-ready-timeout-seconds: 0`.
- Add deploy + runtime tests covering each new path: terminal-ok values
  beyond `ready`, `shuttingdown`/unknown handling, and the
  fail-on-test-error-false-but-dataset-error-still-fatal case.
@lukekim lukekim merged commit ca6f77b into trunk May 3, 2026
4 checks passed
@lukekim lukekim deleted the fix/punycode-deprecation branch May 3, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants