feat: support outputing ND-JSON files via an environment variable #6383
Conversation
🦋 Changeset detectedLatest commit: 916ccb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
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/10282849166/npm-package-wrangler-6383You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6383/npm-package-wrangler-6383Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-wrangler-6383 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-create-cloudflare-6383 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-cloudflare-kv-asset-handler-6383npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-miniflare-6383npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-cloudflare-pages-shared-6383npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10282849166/npm-package-cloudflare-vitest-pool-workers-6383Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
petebacondarwin
changed the title
getWranglerSendMetricsFromEnv() function
petebacondarwin
force-pushed
the
add-output-file
branch
2 times, most recently
from
0163cb9 to
4ccaa37
Compare
petebacondarwin
force-pushed
the
add-output-file
branch
3 times, most recently
from
4241d6b to
b985b36
Compare
| writeOutput({ | ||
| type: "deployment", | ||
| version: 1, | ||
| worker_id: name, |
| | "WRANGLER_OUTPUT_FILE_DIRECTORY" | ||
| | "WRANGLER_OUTPUT_FILE_PATH"; |
There was a problem hiding this comment.
To reduce API surface, can we make this one env var WRANGLER_OUTPUT_PATH? Like WRANGLER_LOG_PATH (.log) if it ends with .json we assume it's a filepath otherwise we assume it's a directory path
There was a problem hiding this comment.
That feels somewhat magical? The surface is basically the same it is just that the user needs to know that specifying .json on the end makes a difference to the behaviour. What if they decide they want to use .ndjson or something instead?
| if (deploymentId) { | ||
| writeOutput({ | ||
| type: "deployment", | ||
| version: 1, | ||
| worker_id: name, | ||
| deployment_id: deploymentId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think you intended to have something like this for versions upload in versions/index.ts (which matches this deploy/deploy.ts + deploy/index.ts file structure)
| command_line_args: ["--help"], | ||
| }); | ||
| const outputFile = readFileSync(WRANGLER_OUTPUT_FILE_PATH, "utf8"); | ||
| expect(outputFile).toContainEntries([ |
There was a problem hiding this comment.
Is this custom matcher necessary? To understand what it's doing, you'd have to go looking for the definition whereas if it was a plain function call (expect(parseEntries(outputFile)).toEqual([...) you could just cmd+click (parseEntries) to get to the implementation
There was a problem hiding this comment.
I think custom matchers are great for common use-cases to a whole codebase and should be introduced globally so they can be defined clearly, learnt and relied upon. Implementing them as one-offs per suite I think is obfuscating
There was a problem hiding this comment.
It just a helper function to avoid duplicating the same logic throughout and keeping the tests themselves nice and readable.
It is only used in this file (where it is defined) and it makes no sense outside of this file. It is nicer than using a vanilla function because Vitest treats it specially, for example it will format the errors nicely and do diffing of the actual and expected results.
There was a problem hiding this comment.
If you Cmd-Click on the function it takes you to the type definition below, which is exactly next to the implementation. So I don't think it is too hard to understand.
There was a problem hiding this comment.
Somewhat ironically, when you create global custom matchers like this it is common to put the type in a .d.ts file - e.g. https://github.com/cloudflare/workers-sdk/blob/f6300a71adb83e5f92cd9c9ddd56a85132e03f98/packages/create-cloudflare/vitest.d.ts
In this case it is actually much harder to find the original implementation of the matcher. For example the toExist() matcher implementation is over here: https://github.com/cloudflare/workers-sdk/blob/cc335ee1c1dbd58009714c4b49e23e9dcf121d1e/packages/create-cloudflare/e2e-tests/setup.ts
petebacondarwin
force-pushed
the
add-output-file
branch
from
b985b36 to
8483a68
Compare
| @@ -275,6 +277,15 @@ export async function versionsUploadHandler( | |||
| tag: args.tag, | |||
| message: args.message, | |||
| }); | |||
|
|
|||
| if (versionId && name) { | |||
There was a problem hiding this comment.
Why does this need && name but the same code in deploy/deploy.ts doesn't? Not sure which is correct but I think they should be the same, right?
There was a problem hiding this comment.
This is because the check for whether the name is defined or not (causing an error to be thrown) is deep inside the versionsUpload() function. So it should always be defined by the time it gets here. But I felt it was too large a refactor to pull that out.
There was a problem hiding this comment.
Isn't that the same for deploy? Both call getScriptName. Both should be consistent.
| writeOutput({ | ||
| type: "deployment", | ||
| version: 1, | ||
| worker_id: name, |
There was a problem hiding this comment.
Something I was just chatting with the team about - It would be a lot better for us to get a stable ID for the worker. Ideally we'd use the script_tag - Would that be possible?
There was a problem hiding this comment.
The script_tag is internal to EWC and not made available to Wrangler.
There was a problem hiding this comment.
All the APIs that Wrangler hits use the Worker name and not the ID. AFAIK.
There was a problem hiding this comment.
Wrangler does use script_tag for fetching deployments
The problem with usingname for us is that users can update the name in dash and then all of our internal references would be out-of-sync
There was a problem hiding this comment.
So basically someone has to make this call:
fetchResult<ServiceMetadataRes>(`/accounts/${accountId}/workers/services/${scriptName}`)).default_environment.script.tag;
I guess that Wrangler could do it since it will also have the authentication tokens etc.
I'll update to get that.
There was a problem hiding this comment.
deploy() already makes that call https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/deploy/deploy.ts so you can add it to the returned object
There was a problem hiding this comment.
Thanks @RamIdeas
| @@ -361,6 +363,15 @@ export async function deployHandler( | |||
| experimentalVersions: args.experimentalVersions, | |||
| }); | |||
|
|
|||
| if (deploymentId) { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Ahah. So in that case I made the entry allow worker_id to be undefined!
There was a problem hiding this comment.
I can do that too for version uploads I guess.
petebacondarwin
force-pushed
the
add-output-file
branch
from
8483a68 to
916ccb4
Compare
What this PR solves / how to test
Adds a feature needed for the Workers CI/CD team.
Author has addressed the following