Skip to content

feat: gate dev warehouse auto-resolution behind opt-in env var #460

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
base: main
Choose a base branch
from
+27 −4
Draft

feat: gate dev warehouse auto-resolution behind opt-in env var #460

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
4 changes: 2 additions & 2 deletions apps/dev-playground/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
"scripts": {
"start": "node build/index.mjs",
"start:local": "NODE_ENV=production node --env-file=./server/.env build/index.mjs",
"dev": "NODE_ENV=development tsx watch server/index.ts",
"dev:inspect": "NODE_ENV=development tsx --inspect --tsconfig ./tsconfig.json ./server",
"dev": "NODE_ENV=development APPKIT_AUTO_RESOLVE_WAREHOUSE=1 tsx watch server/index.ts",
"dev:inspect": "NODE_ENV=development APPKIT_AUTO_RESOLVE_WAREHOUSE=1 tsx --inspect --tsconfig ./tsconfig.json ./server",
"build": "npm run build:app",
"build:app": "tsdown --out-dir build server/index.ts && cd client && npm run build",
"build:server": "tsdown --out-dir build server/index.ts",
Expand Down
6 changes: 5 additions & 1 deletion packages/appkit/src/context/service-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,11 @@ export class ServiceContext {
return process.env.DATABRICKS_WAREHOUSE_ID;
}

if (process.env.NODE_ENV === "development") {
const autoResolve =
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE === "true" ||
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE === "1";

if (process.env.NODE_ENV === "development" && autoResolve) {
const response = (await client.apiClient.request({
path: "/api/2.0/sql/warehouses",
method: "GET",
Expand Down
21 changes: 20 additions & 1 deletion packages/appkit/src/context/tests/service-context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,25 @@ describe("ServiceContext", () => {
expect(await state.warehouseId).toBe("env-wh-abc");
});

test("should auto-discover warehouse in development mode", async () => {
test("should throw in dev mode when auto-resolve is not opted in", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
delete process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE;
process.env.NODE_ENV = "development";

await expect(
ServiceContext.initialize({ warehouseId: true }),
).rejects.toThrow(ConfigurationError);
// Must not attempt to list warehouses without explicit opt-in.
expect(mockApiRequest).not.toHaveBeenCalledWith(
expect.objectContaining({ path: "/api/2.0/sql/warehouses" }),
);
});

test("should auto-discover warehouse in dev mode when opted in", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
process.env.NODE_ENV = "development";
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE = "1";

mockApiRequest.mockImplementation(({ path }: { path: string }) => {
if (path === "/api/2.0/sql/warehouses") {
return Promise.resolve({
Expand All @@ -365,6 +380,7 @@ describe("ServiceContext", () => {
test("should sort warehouses by state priority in dev mode", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
process.env.NODE_ENV = "development";
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE = "1";

mockApiRequest.mockImplementation(({ path }: { path: string }) => {
if (path === "/api/2.0/sql/warehouses") {
Expand All @@ -388,6 +404,7 @@ describe("ServiceContext", () => {
test("should throw in dev mode when no warehouses are available", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
process.env.NODE_ENV = "development";
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE = "1";

mockApiRequest.mockImplementation(({ path }: { path: string }) => {
if (path === "/api/2.0/sql/warehouses") {
Expand All @@ -404,6 +421,7 @@ describe("ServiceContext", () => {
test("should throw in dev mode when all warehouses are deleted", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
process.env.NODE_ENV = "development";
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE = "1";

mockApiRequest.mockImplementation(({ path }: { path: string }) => {
if (path === "/api/2.0/sql/warehouses") {
Expand All @@ -425,6 +443,7 @@ describe("ServiceContext", () => {
test("should throw in dev mode when best warehouse has no id", async () => {
delete process.env.DATABRICKS_WAREHOUSE_ID;
process.env.NODE_ENV = "development";
process.env.APPKIT_AUTO_RESOLVE_WAREHOUSE = "1";

mockApiRequest.mockImplementation(({ path }: { path: string }) => {
if (path === "/api/2.0/sql/warehouses") {
Expand Down
Loading