Conversation
flakey5
force-pushed
the
flakey5/159
branch
2 times, most recently
from
0ade6ef to
7fb3816
Compare
| # 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 |
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 :)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| export async function listR2DirectoryRecursive( | ||
| client, | ||
| bucket, | ||
| directory = undefined, |
There was a problem hiding this comment.
| directory = undefined, | |
| directory, |
There was a problem hiding this comment.
That parameter should be optional?
There was a problem hiding this comment.
Why does explicitly saying it defaults to undefined any different than leaving it blank?
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
PR SummaryMedium Risk Overview Replaces the old release-link generation script with KV cache maintenance tooling. Introduces shared Plumbs KV through CI and local config. The Reviewed by Cursor Bugbot for commit e15aa67. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -0,0 +1,68 @@ | |||
| import { S3Client } from '@aws-sdk/client-s3'; | |||
| @@ -0,0 +1,245 @@ | |||
| import { readFile, writeFile } from 'node:fs/promises'; | |||
| import { basename, dirname } from 'node:path'; | |||
| import { S3Client } from '@aws-sdk/client-s3'; | |||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Version argument crashes before null check executes
- I now validate the raw version argument before calling
toLowerCase()so missing input throws the intended explicit error.
- I now validate the raw version argument before calling
- ✅ Fixed: Subdirectories cached without trailing slashes in e2e tests
- The e2e cache population now appends
/to each subdirectory name so test data matches production directory-cache shape.
- The e2e cache population now appends
- ✅ Fixed:
latestkey incorrectly added as directory symlink- I restricted added directory symlinks to keys starting with
latest-, which excludes bothlatestandnode-latest.tar.gz.
- I restricted added directory symlinks to keys starting with
Or push these changes by commenting:
@cursor push 29199a84ef
Preview (29199a84ef)
diff --git a/e2e-tests/util.ts b/e2e-tests/util.ts
--- a/e2e-tests/util.ts
+++ b/e2e-tests/util.ts
@@ -47,7 +47,9 @@
});
const cachedDirectory: ReadDirectoryResult = {
- subdirectories: Object.keys(directory.subdirectories),
+ subdirectories: Object.keys(directory.subdirectories).map(
+ subdirectory => `${subdirectory}/`
+ ),
files,
hasIndexHtmlFile,
lastModified: new Date(),
diff --git a/scripts/update-directory-cache.mjs b/scripts/update-directory-cache.mjs
--- a/scripts/update-directory-cache.mjs
+++ b/scripts/update-directory-cache.mjs
@@ -20,12 +20,13 @@
import { createCloudflareClient, writeKeysToKv } from './utils/kv.mjs';
import { createS3Client, listR2DirectoryRecursive } from './utils/r2.mjs';
-const VERSION = process.argv[2].toLowerCase();
-
-if (!VERSION) {
+const VERSION_ARG = process.argv[2];
+if (!VERSION_ARG) {
throw new TypeError('version missing from args');
}
+const VERSION = VERSION_ARG.toLowerCase();
+
if (!VERSION.startsWith('v')) {
throw new TypeError(
'provided version not in correct format, expected vX.X.X'
diff --git a/scripts/utils/addSymlinksToDirectoryCache.mjs b/scripts/utils/addSymlinksToDirectoryCache.mjs
--- a/scripts/utils/addSymlinksToDirectoryCache.mjs
+++ b/scripts/utils/addSymlinksToDirectoryCache.mjs
@@ -55,8 +55,8 @@
) {
releaseDirectory.subdirectories.push(
...Object.keys(latestVersionMap)
- // We only want directories, remove the only file in the map
- .filter(version => version !== NODE_LATEST_FILE_NAME)
+ // We only want the `latest-*` directory symlinks.
+ .filter(version => version.startsWith('latest-'))
.map(version => `${version}/`)
);
releaseDirectory.subdirectories.sort();
diff --git a/scripts/utils/addSymlinksToDirectoryCache.test.ts b/scripts/utils/addSymlinksToDirectoryCache.test.ts
--- a/scripts/utils/addSymlinksToDirectoryCache.test.ts
+++ b/scripts/utils/addSymlinksToDirectoryCache.test.ts
@@ -97,6 +97,7 @@
const latestVersionMap: LatestVersionMapping = {
'latest-v20.x': 'v20.1.2',
'latest-v22.x': 'v22.2.1',
+ latest: 'v22.2.1',
'node-latest.tar.gz': 'latest/node-v22.2.1',
};This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e15aa67. Configure here.
|
|
||
| if (!VERSION) { | ||
| throw new TypeError('version missing from args'); | ||
| } |
There was a problem hiding this comment.
Version argument crashes before null check executes
High Severity
process.argv[2] can be undefined when no argument is provided, and calling .toLowerCase() on it throws an unhandled TypeError with a cryptic message before the intended validation on the next line can produce the helpful 'version missing from args' error.
Reviewed by Cursor Bugbot for commit e15aa67. Configure here.
| }); | ||
|
|
||
| const cachedDirectory: ReadDirectoryResult = { | ||
| subdirectories: Object.keys(directory.subdirectories), |
There was a problem hiding this comment.
Subdirectories cached without trailing slashes in e2e tests
Medium Severity
populateDirectoryCache sets subdirectories to Object.keys(directory.subdirectories) which yields names like v20.0.0 without trailing slashes. The production cache scripts (listR2Directory) always produce subdirectory names with trailing / (from S3 CommonPrefixes). This mismatch means e2e tests validate against incorrectly shaped data, potentially hiding real bugs.
Reviewed by Cursor Bugbot for commit e15aa67. Configure here.
| // We only want directories, remove the only file in the map | ||
| .filter(version => version !== NODE_LATEST_FILE_NAME) | ||
| .map(version => `${version}/`) | ||
| ); |
There was a problem hiding this comment.
latest key incorrectly added as directory symlink
Medium Severity
addLatestDirectorySymlinksToCache filters only node-latest.tar.gz from the version map, but the map also contains a latest key (mapping to e.g. v25.9.0). This causes a spurious latest/ entry to be added to the release directory's subdirectories, which isn't a real directory or symlink.
Reviewed by Cursor Bugbot for commit e15aa67. Configure here.
4 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