Conversation
flakey5
force-pushed
the
flakey5/159
branch
2 times, most recently
from
0ade6ef to
7fb3816
Compare
| "kv_namespaces": [ | ||
| { | ||
| "binding": "DIRECTORY_CACHE", | ||
| "id": "TODO", |
There was a problem hiding this comment.
blocking till kv namespace is created
| # Triggered by https://github.com/nodejs/node/blob/main/.github/workflows/update-release-links.yml | ||
| workflow_dispatch: | ||
| inputs: | ||
| version: |
There was a problem hiding this comment.
blocking till node core pr passing this input in gets made + merged
|
cc @flakey5 some of the CI steps are failing |
| * @param {number} retryCount | ||
| * @returns {Promise<import('../src/providers/provider.js').ReadDirectoryResult | undefined>} | ||
| */ | ||
| export async function listR2Directory( |
There was a problem hiding this comment.
I wonder if this mega function could be broken down into smaller chunks to make it easier to read. Also should this function and file be named listR2Directory do we list directories from other sources? Otherwise just call it listDirectory?
There was a problem hiding this comment.
Also, why are these inside a folder called "common" instead of "src"?
There was a problem hiding this comment.
do we list directories from other sources?
Yes for e2e tests
release-cloudflare-worker/vitest-setup.ts
Line 34 in d1f0e1e
why are these inside a folder called "common" instead of "src"?
The idea was the common folder is made up of things shared between the worker code (src) and tooling (scripts)
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Didn't finish the review yet, but leaving initial comments here :)
There was a problem hiding this comment.
Imo yes since it gives context as to what the folder is there for
| * @param {number} retryCount | ||
| * @returns {Promise<import('../src/providers/provider.js').ReadDirectoryResult | undefined>} | ||
| */ | ||
| export async function listR2Directory( |
There was a problem hiding this comment.
Also, why are these inside a folder called "common" instead of "src"?
| import { env, fetchMock, createExecutionContext } from 'cloudflare:test'; | ||
| import { test, beforeAll, afterEach, expect } from 'vitest'; | ||
| import { populateR2WithDevBucket } from './util'; | ||
| import { test, beforeAll, afterEach, expect, describe, vi } from 'vitest'; |
There was a problem hiding this comment.
OOC, can we use node:test instead of vitest? Vitest is great, but just wondering if we can use our out of the box stuff :)
There was a problem hiding this comment.
tldr; yes but vitest is the officially recommended & easiest runner for this use case
| subdirectory.startsWith('v') || subdirectory.startsWith('latest') | ||
| ); | ||
|
|
||
| docsDirectory.subdirectories = Array.from( |
There was a problem hiding this comment.
Hmmm... I wonder i this can be simplified. merging arrays -> set -> array -> sort, that's quite the overhead.
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
2 participants
Makes use of KV for directory listings. This aims to replace the
build-r2-symlinks.mjsfile with two different ones:build-directory-cache.mjs- This reads the entire dist-prod bucket and caches it into the KV namespace. This is pretty much only for the initial setup of the cache and for anytime we need to rebuild all of it.update-directory-cache.mjs- This reads only the necessary paths from the dist-prod bucket and updates them in the KV namespace. This will be the one that we will call in the workflowA
USE_KVenvironment variable is added to the worker that acts as a toggle between KV and S3 listings. This acts as a safeguard in case something goes wrong while we're still testing it, so we can easily switch between the two. Once we're more confident in KV, we can remove it + the entireS3Providerfrom the worker.Closes #159
Closes #314
TODO:
versionargument in Node core when invoking the update redirect links action