Skip to content

Commit

Permalink
feat(wrangler): make resources identifier optional if x-provision fla…
Browse files Browse the repository at this point in the history
…g is enabled (#7377)
  • Loading branch information
edmundhung authored Nov 29, 2024
1 parent 4f6f2ec commit 6ecc74e
Show file tree
Hide file tree
Showing 12 changed files with 133 additions and 31 deletions.
5 changes: 5 additions & 0 deletions .changeset/lucky-carrots-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

The `x-provision` experimental flag now skips validation of KV, R2, and D1 IDs in the configuration file.
61 changes: 61 additions & 0 deletions packages/wrangler/src/__tests__/configuration.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import path from "node:path";
import { readConfig } from "../config";
import { normalizeAndValidateConfig } from "../config/validation";
import { run } from "../experimental-flags";
import { normalizeString } from "./helpers/normalize";
import { runInTempDir } from "./helpers/run-in-tmp";
import { writeWranglerConfig } from "./helpers/write-wrangler-config";
Expand Down Expand Up @@ -2324,6 +2325,26 @@ describe("normalizeAndValidateConfig()", () => {
- \\"kv_namespaces[4]\\" bindings should have a string \\"id\\" field but got {\\"binding\\":\\"VALID\\",\\"id\\":\\"\\"}."
`);
});

it("should allow the id field to be omitted when the RESOURCES_PROVISION experimental flag is enabled", () => {
const { diagnostics } = run(
{
RESOURCES_PROVISION: true,
FILE_BASED_REGISTRY: false,
},
() =>
normalizeAndValidateConfig(
{
kv_namespaces: [{ binding: "VALID" }],
} as unknown as RawConfig,
undefined,
{ env: undefined }
)
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(false);
});
});

it("should error if send_email.bindings are not valid", () => {
Expand Down Expand Up @@ -2452,6 +2473,26 @@ describe("normalizeAndValidateConfig()", () => {
- \\"d1_databases[4]\\" bindings must have a \\"database_id\\" field but got {\\"binding\\":\\"VALID\\",\\"id\\":\\"\\"}."
`);
});

it("should allow the database_id field to be omitted when the RESOURCES_PROVISION experimental flag is enabled", () => {
const { diagnostics } = run(
{
RESOURCES_PROVISION: true,
FILE_BASED_REGISTRY: false,
},
() =>
normalizeAndValidateConfig(
{
d1_databases: [{ binding: "VALID" }],
} as unknown as RawConfig,
undefined,
{ env: undefined }
)
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(false);
});
});

describe("[hyperdrive]", () => {
Expand Down Expand Up @@ -2746,6 +2787,26 @@ describe("normalizeAndValidateConfig()", () => {
- \\"r2_buckets[4]\\" bindings should have a string \\"bucket_name\\" field but got {\\"binding\\":\\"R2_BINDING_1\\",\\"bucket_name\\":\\"\\"}."
`);
});

it("should allow the bucket_name field to be omitted when the RESOURCES_PROVISION experimental flag is enabled", () => {
const { diagnostics } = run(
{
RESOURCES_PROVISION: true,
FILE_BASED_REGISTRY: false,
},
() =>
normalizeAndValidateConfig(
{
d1_databases: [{ binding: "VALID" }],
} as unknown as RawConfig,
undefined,
{ env: undefined }
)
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(false);
});
});

describe("[services]", () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/api/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export interface UnstableDevOptions {
vars?: Record<string, string | Json>;
kv?: {
binding: string;
id: string;
id?: string;
preview_id?: string;
}[];
durableObjects?: {
Expand All @@ -51,7 +51,7 @@ export interface UnstableDevOptions {
}[];
r2?: {
binding: string;
bucket_name: string;
bucket_name?: string;
preview_bucket_name?: string;
}[];
ai?: {
Expand Down
8 changes: 4 additions & 4 deletions packages/wrangler/src/config/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ export interface EnvironmentNonInheritable {
/** The binding name used to refer to the KV Namespace */
binding: string;
/** The ID of the KV namespace */
id: string;
id?: string;
/** The ID of the KV namespace used during `wrangler dev` */
preview_id?: string;
}[];
Expand Down Expand Up @@ -565,7 +565,7 @@ export interface EnvironmentNonInheritable {
/** The binding name used to refer to the R2 bucket in the Worker. */
binding: string;
/** The name of this R2 bucket at the edge. */
bucket_name: string;
bucket_name?: string;
/** The preview name of this R2 bucket at the edge. */
preview_bucket_name?: string;
/** The jurisdiction that the bucket exists in. Default if not present. */
Expand All @@ -585,9 +585,9 @@ export interface EnvironmentNonInheritable {
/** The binding name used to refer to the D1 database in the Worker. */
binding: string;
/** The name of this D1 database. */
database_name: string;
database_name?: string;
/** The UUID of this D1 database (not required). */
database_id: string;
database_id?: string;
/** The UUID of this D1 database for Wrangler Dev (if specified). */
preview_database_id?: string;
/** The name of the migrations table for this D1 database (defaults to 'd1_migrations'). */
Expand Down
17 changes: 11 additions & 6 deletions packages/wrangler/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,11 @@ export function findWranglerConfig(
);
}

function addLocalSuffix(id: string, local: boolean = false) {
function addLocalSuffix(id: string | undefined, local: boolean = false) {
if (!id) {
return local ? "(local)" : "(remote)";
}

return `${id}${local ? " (local)" : ""}`;
}

Expand Down Expand Up @@ -403,13 +407,14 @@ export function printBindings(
name: friendlyBindingNames.d1_databases,
entries: d1_databases.map(
({ binding, database_name, database_id, preview_database_id }) => {
let databaseValue = `${database_id}`;
if (database_name) {
databaseValue = `${database_name} (${database_id})`;
}
let databaseValue =
database_id && database_name
? `${database_name} (${database_id})`
: database_id ?? database_name;

//database_id is local when running `wrangler dev --local`
if (preview_database_id && database_id !== "local") {
databaseValue += `, Preview: (${preview_database_id})`;
databaseValue = `${databaseValue ? `${databaseValue}, ` : ""}Preview: (${preview_database_id})`;
}
return {
key: binding,
Expand Down
22 changes: 15 additions & 7 deletions packages/wrangler/src/config/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import assert from "node:assert";
import path from "node:path";
import TOML from "@iarna/toml";
import { dedent } from "ts-dedent";
import { getFlag } from "../experimental-flags";
import { Diagnostics } from "./diagnostics";
import {
all,
Expand Down Expand Up @@ -2433,8 +2434,10 @@ const validateKVBinding: ValidatorFn = (diagnostics, field, value) => {
isValid = false;
}
if (
!isRequiredProperty(value, "id", "string") ||
(value as { id: string }).id.length === 0
getFlag("RESOURCES_PROVISION")
? !isOptionalProperty(value, "id", "string") ||
(value.id !== undefined && value.id.length === 0)
: !isRequiredProperty(value, "id", "string") || value.id.length === 0
) {
diagnostics.errors.push(
`"${field}" bindings should have a string "id" field but got ${JSON.stringify(
Expand Down Expand Up @@ -2595,8 +2598,11 @@ const validateR2Binding: ValidatorFn = (diagnostics, field, value) => {
isValid = false;
}
if (
!isRequiredProperty(value, "bucket_name", "string") ||
(value as { bucket_name: string }).bucket_name.length === 0
getFlag("RESOURCES_PROVISION")
? !isOptionalProperty(value, "bucket_name", "string") ||
(value.bucket_name !== undefined && value.bucket_name.length === 0)
: !isRequiredProperty(value, "bucket_name", "string") ||
value.bucket_name.length === 0
) {
diagnostics.errors.push(
`"${field}" bindings should have a string "bucket_name" field but got ${JSON.stringify(
Expand Down Expand Up @@ -2653,9 +2659,11 @@ const validateD1Binding: ValidatorFn = (diagnostics, field, value) => {
isValid = false;
}
if (
// TODO: allow name only, where we look up the ID dynamically
// !isOptionalProperty(value, "database_name", "string") &&
!isRequiredProperty(value, "database_id", "string")
getFlag("RESOURCES_PROVISION")
? !isOptionalProperty(value, "database_id", "string")
: // TODO: allow name only, where we look up the ID dynamically
// !isOptionalProperty(value, "database_name", "string") &&
!isRequiredProperty(value, "database_id", "string")
) {
diagnostics.errors.push(
`"${field}" bindings must have a "database_id" field but got ${JSON.stringify(
Expand Down
6 changes: 6 additions & 0 deletions packages/wrangler/src/d1/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ export function getDatabaseInfoFromConfig(
d1Database.database_id &&
(name === d1Database.database_name || name === d1Database.binding)
) {
if (!d1Database.database_name) {
throw new UserError(
`${name} bindings must have a "database_name" field`
);
}

return {
uuid: d1Database.database_id,
previewDatabaseUuid: d1Database.preview_database_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import assert from "node:assert";
import { readFileSync } from "node:fs";
import path from "node:path";
import { File, FormData } from "undici";
import { UserError } from "../errors";
import { handleUnsafeCapnp } from "./capnp";
import type { Observability } from "../config/environment";
import type {
Expand Down Expand Up @@ -225,6 +226,9 @@ export function createWorkerUploadForm(worker: CfWorkerInit): FormData {
});

bindings.kv_namespaces?.forEach(({ id, binding }) => {
if (id === undefined) {
throw new UserError(`${binding} bindings must have an "id" field`);
}
metadataBindings.push({
name: binding,
type: "kv_namespace",
Expand Down Expand Up @@ -275,6 +279,11 @@ export function createWorkerUploadForm(worker: CfWorkerInit): FormData {
});

bindings.r2_buckets?.forEach(({ binding, bucket_name, jurisdiction }) => {
if (bucket_name === undefined) {
throw new UserError(
`${binding} bindings must have a "bucket_name" field`
);
}
metadataBindings.push({
name: binding,
type: "r2_bucket",
Expand All @@ -285,6 +294,11 @@ export function createWorkerUploadForm(worker: CfWorkerInit): FormData {

bindings.d1_databases?.forEach(
({ binding, database_id, database_internal_env }) => {
if (database_id === undefined) {
throw new UserError(
`${binding} bindings must have a "database_id" field`
);
}
metadataBindings.push({
name: binding,
type: "d1",
Expand Down
8 changes: 4 additions & 4 deletions packages/wrangler/src/deployment-bundle/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export interface CfVars {
*/
export interface CfKvNamespace {
binding: string;
id: string;
id?: string;
}

/**
Expand Down Expand Up @@ -167,15 +167,15 @@ export interface CfQueue {

export interface CfR2Bucket {
binding: string;
bucket_name: string;
bucket_name?: string;
jurisdiction?: string;
}

// TODO: figure out if this is duplicated in packages/wrangler/src/config/environment.ts
export interface CfD1Database {
binding: string;
database_id: string;
database_name: string;
database_id?: string;
database_name?: string;
preview_database_id?: string;
database_internal_env?: string;
migrations_table?: string;
Expand Down
12 changes: 8 additions & 4 deletions packages/wrangler/src/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ import type {
Route,
Rule,
} from "./config/environment";
import type { CfModule, CfWorkerInit } from "./deployment-bundle/worker";
import type {
CfKvNamespace,
CfModule,
CfWorkerInit,
} from "./deployment-bundle/worker";
import type { WorkerRegistry } from "./dev-registry";
import type { CfAccount } from "./dev/create-worker-preview";
import type { LoggerLevel } from "./logger";
Expand Down Expand Up @@ -394,7 +398,7 @@ export type AdditionalDevProps = {
vars?: Record<string, string | Json>;
kv?: {
binding: string;
id: string;
id?: string;
preview_id?: string;
}[];
durableObjects?: {
Expand All @@ -411,7 +415,7 @@ export type AdditionalDevProps = {
}[];
r2?: {
binding: string;
bucket_name: string;
bucket_name?: string;
preview_bucket_name?: string;
jurisdiction?: string;
}[];
Expand Down Expand Up @@ -956,7 +960,7 @@ export function getBindings(
* config in `wrangler.toml`.
*/
// merge KV bindings
const kvConfig = (configParam.kv_namespaces || []).map(
const kvConfig = (configParam.kv_namespaces || []).map<CfKvNamespace>(
({ binding, preview_id, id }) => {
// In remote `dev`, we make folks use a separate kv namespace called
// `preview_id` instead of `id` so that they don't
Expand Down
6 changes: 3 additions & 3 deletions packages/wrangler/src/dev/miniflare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,13 @@ async function buildSourceOptions(
}

function kvNamespaceEntry({ binding, id }: CfKvNamespace): [string, string] {
return [binding, id];
return [binding, id ?? binding];
}
function r2BucketEntry({ binding, bucket_name }: CfR2Bucket): [string, string] {
return [binding, bucket_name];
return [binding, bucket_name ?? binding];
}
function d1DatabaseEntry(db: CfD1Database): [string, string] {
return [db.binding, db.preview_database_id ?? db.database_id];
return [db.binding, db.preview_database_id ?? db.database_id ?? db.binding];
}
function queueProducerEntry(
queue: CfQueue
Expand Down
1 change: 0 additions & 1 deletion packages/wrangler/src/kv/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ export const kvNamespaceCreateCommand = createCommand({
formatConfigSnippet(
{
kv_namespaces: [
// @ts-expect-error intentional subset
{
binding: getValidBindingName(args.namespace, "KV"),
[`${previewString}id`]: namespaceId,
Expand Down

0 comments on commit 6ecc74e

Please sign in to comment.