Skip to content

feat(shared): migrate the appkit CLI off the legacy workspace client #643

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.

Sign up for GitHub

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

Draft
MarioCadenas wants to merge 1 commit into from
Draft

feat(shared): migrate the appkit CLI off the legacy workspace client #643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions packages/shared/src/cli/commands/doctor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ so the reported problem is the *root* cause, not a symptom.
| ------------ | ----------------------------------------------------- | ---------------------------------------------------------- |
| `auth` | Can we authenticate to the workspace at all? | validate `DATABRICKS_HOST` is a real URL, then `currentUser.me()` — once, app-wide; a failure skips the live layer |
| `config` | Are the resource's field env vars **present**? | offline presence check of `process.env` (presence only — see note) |
| `existence` | Does the resource exist and is it reachable? | cheapest per-type live probe (`warehouses.get`, `servingEndpoints.get`, …); Lakebase runs a real `SELECT 1` |
| `existence` | Does the resource exist and is it reachable? | cheapest per-type live probe (`warehouses.getWarehouse`, `GET /api/2.0/serving-endpoints/{name}`, …); Lakebase runs a real `SELECT 1` |

`config` checks env-var presence only; whether a value points at a real resource
is the `existence` layer's job. (`DATABRICKS_HOST` is the exception — `auth`
Expand Down Expand Up @@ -217,15 +217,16 @@ skipped outright, not merged.)

Because of that, a failed or conflicted auth row reports **both** `host:` and
`profile:`. The host shown is the one the SDK actually resolved, read from
`client.config.host` — which the SDK populates lazily on the first API call, so
it's read *after* `me()`, not at construction. When the client never got built,
the host is recovered from the `host=…` fragment the SDK appends to its
`ConfigError`; failing that, it falls back to `DATABRICKS_HOST`. Every path is
passed through `sanitizeHost`, so embedded `user:pass@` credentials can't leak.
`client.getHost()`. That resolves from env + profile only (no network), so it's
available even when the credentials are rejected. When resolution itself fails
(missing profile, no host anywhere), it falls back to `DATABRICKS_HOST`. Every
path is passed through `sanitizeHost`, so embedded `user:pass@` credentials
can't leak.

`HOST_PROFILE_CONFLICT` compares `DATABRICKS_HOST` against the profile's own
declared host, read offline from `~/.databrickscfg` via the SDK's exported
`loadConfigFile` (the resolved config is useless here — env has already won).
declared host, read offline from `~/.databrickscfg` via sdk-core's profile
`resolve` with the env overlay disabled (the resolved config is useless here —
env has already won).
Comparison ignores scheme, case, and trailing slash. It's a **warning**, not an
error: the credentials do work, so it must not gate CI, but a green tick would
hide a real misconfiguration. When auth *also* fails, the conflict replaces the
Expand Down
78 changes: 50 additions & 28 deletions packages/shared/src/cli/commands/doctor/checks-existence.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ function apiError(statusCode: number, message = "boom"): Error {
describe("runExistenceProbe — sql_warehouse", () => {
it("ok when the warehouse exists and is RUNNING", async () => {
const client = {
warehouses: { get: async () => ({ state: "RUNNING" }) },
warehouses: { getWarehouse: async () => ({ state: "RUNNING" }) },
};
const r = await runExistenceProbe(client, target());
expect(r.status).toBe("ok");
});

it("warns when the warehouse exists but is STOPPED", async () => {
const client = {
warehouses: { get: async () => ({ state: "STOPPED" }) },
warehouses: { getWarehouse: async () => ({ state: "STOPPED" }) },
};
const r = await runExistenceProbe(client, target());
expect(r.status).toBe("warn");
Expand All @@ -53,7 +53,7 @@ describe("runExistenceProbe — sql_warehouse", () => {
it("errors NOT_FOUND on a 404", async () => {
const client = {
warehouses: {
get: async () => {
getWarehouse: async () => {
throw apiError(404);
},
},
Expand All @@ -66,7 +66,7 @@ describe("runExistenceProbe — sql_warehouse", () => {
it("errors INVALID_VALUE on a 400, quoting the value (no type/blob leak)", async () => {
const client = {
warehouses: {
get: async () => {
getWarehouse: async () => {
throw Object.assign(
new Error(
'Response from server (Bad Request) {"error_code":"INVALID_PARAMETER_VALUE","message":"bogus is not a valid endpoint id."}',
Expand All @@ -90,7 +90,7 @@ describe("runExistenceProbe — sql_warehouse", () => {
it("errors ACCESS_DENIED on a 403", async () => {
const client = {
warehouses: {
get: async () => {
getWarehouse: async () => {
throw apiError(403);
},
},
Expand All @@ -103,7 +103,7 @@ describe("runExistenceProbe — sql_warehouse", () => {
it("hedges on a 403, which several APIs also return for a missing resource", async () => {
const client = {
warehouses: {
get: async () => {
getWarehouse: async () => {
throw apiError(403);
},
},
Expand All @@ -114,8 +114,31 @@ describe("runExistenceProbe — sql_warehouse", () => {
expect(r.detail).toContain("wh-1");
});

// The modular SDK's ApiError carries `httpStatusCode` / `code`, not the
// legacy `statusCode` / `errorCode`; both must classify the same.
it("classifies the modular ApiError shape (httpStatusCode / code)", async () => {
const modular = (httpStatusCode: number, code: string) => ({
getWarehouse: async () => {
throw Object.assign(new Error("boom"), { httpStatusCode, code });
},
});
const cases: Array<[number, string, string]> = [
[404, "RESOURCE_DOES_NOT_EXIST", "NOT_FOUND"],
[400, "INVALID_PARAMETER_VALUE", "INVALID_VALUE"],
[403, "PERMISSION_DENIED", "ACCESS_DENIED"],
[-1, "RESOURCE_DOES_NOT_EXIST", "NOT_FOUND"],
];
for (const [status, code, expected] of cases) {
const r = await runExistenceProbe(
{ warehouses: modular(status, code) },
target(),
);
expect(r.code, `${status}/${code}`).toBe(expected);
}
});

it("skips when the id field is missing", async () => {
const client = { warehouses: { get: async () => ({}) } };
const client = { warehouses: { getWarehouse: async () => ({}) } };
const r = await runExistenceProbe(client, target({ fieldValues: {} }));
expect(r.status).toBe("skipped");
expect(r.code).toBe("MISSING_FIELD");
Expand All @@ -124,7 +147,7 @@ describe("runExistenceProbe — sql_warehouse", () => {

describe("runExistenceProbe — job", () => {
it("errors on a non-integer job id", async () => {
const client = { jobs: { get: async () => ({}) } };
const client = { jobs: { getJob: async () => ({}) } };
const r = await runExistenceProbe(
client,
target({ type: "job", fieldValues: { id: "not-a-number" } }),
Expand All @@ -134,7 +157,7 @@ describe("runExistenceProbe — job", () => {
});

it("ok for a valid integer job id", async () => {
const client = { jobs: { get: async () => ({}) } };
const client = { jobs: { getJob: async () => ({}) } };
const r = await runExistenceProbe(
client,
target({ type: "job", fieldValues: { id: "42" } }),
Expand All @@ -149,8 +172,8 @@ describe("runExistenceProbe — job", () => {
let seen: unknown;
const client = {
jobs: {
get: async (req: { job_id: unknown }) => {
seen = req.job_id;
getJob: async (req: { jobId: unknown }) => {
seen = req.jobId;
return {};
},
},
Expand All @@ -164,7 +187,7 @@ describe("runExistenceProbe — job", () => {
});

it("rejects numeric forms the API can't take (1e3, 0x10)", async () => {
const client = { jobs: { get: async () => ({}) } };
const client = { jobs: { getJob: async () => ({}) } };
for (const id of ["1e3", "0x10", "4.5", "-1"]) {
const r = await runExistenceProbe(
client,
Expand Down Expand Up @@ -260,7 +283,7 @@ describe("runExistenceProbe — postgres (Lakebase)", () => {

describe("runExistenceProbe — per-type coverage with real manifest keys", () => {
it("serving_endpoint: ok via `name`", async () => {
const client = { servingEndpoints: { get: async () => ({}) } };
const client = { request: async () => ({}) };
const r = await runExistenceProbe(
client,
target({
Expand All @@ -273,10 +296,8 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =

it("serving_endpoint: hints id-vs-name when configured by id and the probe fails", async () => {
const client = {
servingEndpoints: {
get: async () => {
throw Object.assign(new Error("not found"), { statusCode: 404 });
},
request: async () => {
throw Object.assign(new Error("not found"), { statusCode: 404 });
},
};
const r = await runExistenceProbe(
Expand All @@ -289,10 +310,8 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =

it("serving_endpoint: no id-vs-name hint when configured by name", async () => {
const client = {
servingEndpoints: {
get: async () => {
throw Object.assign(new Error("not found"), { statusCode: 404 });
},
request: async () => {
throw Object.assign(new Error("not found"), { statusCode: 404 });
},
};
const r = await runExistenceProbe(
Expand All @@ -304,7 +323,7 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =
});

it("genie_space: ok via `id`", async () => {
const client = { genie: { getSpace: async () => ({}) } };
const client = { genie: { genieGetSpace: async () => ({}) } };
const r = await runExistenceProbe(
client,
target({ type: "genie_space", fieldValues: { id: "01ef" } }),
Expand All @@ -313,7 +332,7 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =
});

it("volume: ok via `path` (real manifest key)", async () => {
const client = { volumes: { read: async () => ({}) } };
const client = { request: async () => ({}) };
const r = await runExistenceProbe(
client,
target({
Expand All @@ -325,7 +344,7 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =
});

it("uc_function: ok via `name`", async () => {
const client = { functions: { get: async () => ({}) } };
const client = { request: async () => ({}) };
const r = await runExistenceProbe(
client,
target({ type: "uc_function", fieldValues: { name: "cat.sch.fn" } }),
Expand All @@ -334,8 +353,8 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =
});

it("vector_search_index: probes via camelCase `indexName`", async () => {
const getIndex = vi.fn(async () => ({}));
const client = { vectorSearchIndexes: { getIndex } };
const request = vi.fn(async () => ({}));
const client = { request };
const r = await runExistenceProbe(
client,
target({
Expand All @@ -344,11 +363,14 @@ describe("runExistenceProbe — per-type coverage with real manifest keys", () =
}),
);
expect(r.status).toBe("ok");
expect(getIndex).toHaveBeenCalledWith({ index_name: "main.default.idx" });
expect(request).toHaveBeenCalledWith({
method: "GET",
path: "/api/2.0/vector-search/indexes/main.default.idx",
});
});

it("vector_search_index: skips MISSING_FIELD when index name absent", async () => {
const client = { vectorSearchIndexes: { getIndex: async () => ({}) } };
const client = { request: async () => ({}) };
const r = await runExistenceProbe(
client,
target({ type: "vector_search_index", fieldValues: {} }),
Expand Down
Loading