Skip to content
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.

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

feat: add support for redirecting Wrangler to a generated config when running deploy commands #7442

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Dec 4, 2024

Fixes #5579

This feature is designed for build tools and frameworks to provide a deploy-specific configuration, which Wrangler can use instead of user configuration when running deploy commands.
It is not expected that developers of Workers will need to use this feature directly.

Affected commands

The commands that use this feature are:

  • wrangler deploy
  • wrangler dev
  • wrangler versions upload
  • wrangler versions deploy
  • wrangler pages deploy
  • wrangler pages build
  • wrangler pages build-env

Config redirect file

When running these commands, Wrangler will look up the directory tree from the current working directory for a file at the path .wrangler/deploy/config.json. This file must contain only a single JSON object of the form:

{ "configPath": "../../path/to/wrangler.json" }

When this file exists Wrangler will use the configPath (relative to the config.json file) to find an alternative Wrangler configuration file to load and use as part of this command.

When this happens Wrangler will display a warning to the user to indicate that the configuration has been redirected to a different file than the user's configuration file.

Custom build tool example

A common approach that a build tool might choose to implement.

  • The user writes code that uses Cloudflare Workers resources, configured via a user wrangler.toml file.

    name = "my-worker"
    main = "src/index.ts"
    [[kv_namespaces]]
    binding = "<BINDING_NAME1>"
    id = "<NAMESPACE_ID1>"

    Note that this configuration points main at user code entry-point.

  • The user runs a custom build, which might read the wrangler.toml to find the entry-point:

    > my-tool build
  • This tool generates a dist directory that contains both compiled code and a new deployment configuration file, but also a .wrangler/deploy/config.json file that redirects Wrangler to this new deployment configuration file:

    - dist
      - index.js
    	- wrangler.json
    - .wrangler
      - deploy
    	  - config.json
    

    The dist/wrangler.json will contain:

    {
    	"name": "my-worker",
    	"main": "./index.js",
    	"kv_namespaces": [{ "binding": "<BINDING_NAME1>", "id": "<NAMESPACE_ID1>" }]
    }

    And the .wrangler/deploy/config.json will contain:

    {
    	"configPath": "../../dist/wrangler.json"
    }

Copy link

changeset-bot bot commented Dec 4, 2024

🦋 Changeset detected

Latest commit: 1f9c943

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch from 5a91f5b to b3bf041 Compare December 4, 2024 19:50
Copy link
Contributor

github-actions bot commented Dec 4, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-wrangler-7442

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7442/npm-package-wrangler-7442

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-wrangler-7442 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-workers-bindings-extension-7442 -O ./cloudflare-workers-bindings-extension.0.0.0-v356c10f90.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v356c10f90.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-create-cloudflare-7442 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-kv-asset-handler-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-miniflare-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-pages-shared-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-unenv-preset-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-vitest-pool-workers-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-workers-editor-shared-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-workers-shared-7442
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12480724262/npm-package-cloudflare-workflows-shared-7442

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241218.0
workerd 1.20241218.0 1.20241218.0
workerd --version 1.20241218.0 2024-12-18

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch 4 times, most recently from e251387 to 3756ab2 Compare December 12, 2024 12:08
@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch 3 times, most recently from 0e8fc8a to a54d88c Compare December 16, 2024 08:22
@petebacondarwin petebacondarwin added the e2e Run e2e tests on a PR label Dec 16, 2024
@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch 2 times, most recently from 48b73e0 to a06374a Compare December 16, 2024 11:27
petebacondarwin added a commit to petebacondarwin/cloudflare-docs that referenced this pull request Dec 16, 2024
@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch 2 times, most recently from e7fd32d to f33b32a Compare December 17, 2024 09:50
@petebacondarwin petebacondarwin marked this pull request as ready for review December 17, 2024 10:43
@petebacondarwin petebacondarwin requested review from a team as code owners December 17, 2024 10:43
"check:type": "npm run _clean_install && npm run check:type --workspaces",
"test:ci": "npm run _clean_install && npm run test:ci --workspaces",
"test:watch": "npm run _clean_install && npm run test:watch --workspaces",
"type:tests": "npm run _clean_install && npm run type:tests --workspaces"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that this fixture's tests kept failing; and I think it was because we need to ensure that the node_modules directory was in fact created by the npm i command here, and not a pnpm i command that is naturally run when setting up the workers-sdk project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Furthemore I couldn't just run _clean_install in front of each of these scripts because turbo tries to run then in parallel. So the solution was to add a turbo dependency instead, which means that this only runs once.

@@ -99,27 +99,25 @@ describe("unstable_dev()", () => {
const childWorker = await unstable_dev(
"${child.replaceAll("\\", "/")}/src/index.ts",
{
configPath: "${child.replaceAll("\\", "/")}/wrangler.toml",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that:

a) the configPath property is not valid so was not being used
b) Wrangler was using the script path above to infer the location of the config path

I could have changed the configPath property to config which is more correct, but actually not having this here at all is a good check that we are resolving the config by "finding up" from the script path (rather than the current working directory).

@@ -8,6 +8,9 @@ import WebSocket from "ws";
import { WranglerE2ETestHelper } from "./helpers/e2e-wrangler-test";
import { generateResourceName } from "./helpers/generate-resource-name";

const port = await getPort();
const inspectorPort = await getPort();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only changes in this file are to explicitly set both the port and inspector port to avoid concurrent tests from failing due to port 9229 being blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

These tests don't run concurrently in CI—was this a problem you were seeing locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was when I running tests locally.

await helper.seed({
"_worker.js": dedent`
const port = await getPort();
const inspectorPort = await getPort();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only changes in this file are to explicitly set both the port and inspector port to avoid concurrent tests from failing due to port 9229 being blocked.

@@ -327,7 +330,7 @@ function applyPythonConfig(
config: Config,
args: NormalizeAndValidateConfigArgs
) {
const mainModule = "script" in args ? args.script : config.main;
const mainModule = args.script ?? config.main;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that the args.script property exists but is set to undefined, which means that we should still defer to config.main in that case.

@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch from f33b32a to 13d920f Compare December 17, 2024 12:04
This creates a lot of unwanted logging in the console.

The logging is still dumped to the console if the test times out, or if `WRANGLER_LOG=debug` is set as env var.
… running deploy commands

This new feature is designed for build tools and frameworks to provide a deploy-specific configuration,
which Wrangler can use instead of user configuration when running deploy commands.
It is not expected that developers of Workers will need to use this feature directly.

The commands that use this feature are:

- `wrangler deploy`
- `wrangler dev`
- `wrangler versions upload`
- `wrangler versions deploy`

When running these commands, Wrangler will look up the directory tree from the current working directory for a file at the path `.wrangler/deploy/config.json`. This file must contain only a single JSON object of the form:

```json
{ "configPath": "../../path/to/wrangler.json" }
```

When this file exists Wrangler will use the `configPath` (relative to the `deploy.json` file) to find an alternative Wrangler configuration file to load and use as part of this command.

When this happens Wrangler will display a warning to the user to indicate that the configuration has been redirected to a different file than the user's configuration file.

A common approach that a build tool might choose to implement.

- The user writes code that uses Cloudflare Workers resources, configured via a user `wrangler.toml` file.

  ```toml
  name = "my-worker"
  main = "src/index.ts"
  [[kv_namespaces]]
  binding = "<BINDING_NAME1>"
  id = "<NAMESPACE_ID1>"
  ```

  Note that this configuration points `main` at user code entry-point.

- The user runs a custom build, which might read the `wrangler.toml` to find the entry-point:

  ```bash
  > my-tool build
  ```

- This tool generates a `dist` directory that contains both compiled code and a new deployment configuration file, but also a `.wrangler/deploy/config.json` file that redirects Wrangler to this new deployment configuration file:

  ```plain
  - dist
    - index.js
  	- wrangler.json
  - .wrangler
    - deploy
  	  - config.json
  ```

  The `dist/wrangler.json` will contain:

  ```json
  {
  	"name": "my-worker",
  	"main": "./index.js",
  	"kv_namespaces": [{ "binding": "<BINDING_NAME1>", "id": "<NAMESPACE_ID1>" }]
  }
  ```

  And the `.wrangler/deploy/config.json` will contain:

  ```json
  {
  	"configPath": "../../dist/wrangler.json"
  }
  ```
…ectory containing:

```
CLOUDFLARE_ACCOUNT_ID=<dev-prod-account-id>
CLOUDFLARE_API_TOKEN=<dev-prod-account-api-key>
WRANGLER="node <path to workers-sdk/packages/wrangler/bin/wrangler.js>"
WRANGLER_IMPORT="<path to /workers-sdk/packages/wrangler/wrangler-dist/cli.js>"
````
This fixture does not need a package lockfile to work.
@petebacondarwin petebacondarwin force-pushed the pbd/add-config-redirect-feature branch from 244c408 to e153667 Compare December 23, 2024 14:03
config?: string;
script?: string;
},
options: { useRedirectIfAvailable?: boolean }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this (and the default value/behavior) be documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes documentation is in cloudflare/cloudflare-docs#18757

): string | undefined {
return (
referencePath: string = process.cwd(),
{ useRedirectIfAvailable = false } = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

document this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Approved
4 participants