feat(repo): add utility scripts#8447
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Node.js script, bin/check-release-discrepancies.cjs, designed to scan the monorepo for public packages and compare their local versions against the NPM registry. The feedback highlights several key areas for improvement: enhancing the fetch retry logic to handle transient 5xx errors and parse HTTP-date retry-after headers; correcting the script name in the help message; narrowing the directory exclusion filter to prevent skipping valid packages containing the word 'template'; utilizing the standard semver package instead of a custom version parser; and restricting progress bar updates to TTY environments to keep CI/CD logs clean.
| // Fetch wrapper with retry and exponential backoff on 429 rate limit | ||
| async function fetchWithRetry(url, options = {}, retries = 6, delay = 3000) { | ||
| try { | ||
| const res = await fetch(url, options); | ||
| if (res.status === 429 && retries > 0) { | ||
| const retryAfterHeader = res.headers.get('retry-after'); | ||
| const waitTime = retryAfterHeader ? (parseInt(retryAfterHeader, 10) * 1000) : delay; | ||
| await new Promise(resolve => setTimeout(resolve, waitTime)); | ||
| return fetchWithRetry(url, options, retries - 1, delay * 2); | ||
| } | ||
| return res; | ||
| } catch (err) { | ||
| if (retries > 0) { | ||
| await new Promise(resolve => setTimeout(resolve, delay)); | ||
| return fetchWithRetry(url, options, retries - 1, delay * 2); | ||
| } | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two issues with the current retry logic:\n1. It only retries on HTTP 429 and network exceptions, but transient 5xx server errors (e.g., 500, 502, 503, 504) should also be retried.\n2. The retry-after header can be either a number of seconds or an HTTP-date (per RFC 7231). If it is an HTTP-date, parseInt will return NaN, causing the script to retry immediately without waiting. Parsing the date fallback correctly prevents this.
// Fetch wrapper with retry and exponential backoff on transient errors\nasync function fetchWithRetry(url, options = {}, retries = 6, delay = 3000) {\n try {\n const res = await fetch(url, options);\n const isTransientError = res.status === 429 || (res.status >= 500 && res.status < 600);\n if (isTransientError && retries > 0) {\n let waitTime = delay;\n if (res.status === 429) {\n const retryAfterHeader = res.headers.get('retry-after');\n if (retryAfterHeader) {\n const parsedSeconds = parseInt(retryAfterHeader, 10);\n if (!isNaN(parsedSeconds)) {\n waitTime = parsedSeconds * 1000;\n } else {\n const parsedDate = Date.parse(retryAfterHeader);\n if (!isNaN(parsedDate)) {\n waitTime = Math.max(0, parsedDate - Date.now());\n }\n }\n }\n }\n await new Promise(resolve => setTimeout(resolve, waitTime));\n return fetchWithRetry(url, options, retries - 1, delay * 2);\n }\n return res;\n } catch (err) {\n if (retries > 0) {\n await new Promise(resolve => setTimeout(resolve, delay));\n return fetchWithRetry(url, options, retries - 1, delay * 2);\n }\n throw err;\n }\n}
| console.log(`${BOLD}google-cloud-node Package Release Discrepancy Checker${RESET}`); | ||
| console.log('\nChecks all local Node.js packages against the NPM registry to find released version mismatches.\n'); | ||
| console.log(`${BOLD}Usage:${RESET}`); | ||
| console.log(' ./bin/check-release-discrepancies.sh [options]\n'); |
There was a problem hiding this comment.
The help message references ./bin/check-release-discrepancies.sh, but the script is named check-release-discrepancies.cjs. Updating this ensures the usage instructions are accurate.
console.log(' ./bin/check-release-discrepancies.cjs [options]\n');
| name === 'templates' || | ||
| name === 'template' || | ||
| name.includes('template') |
There was a problem hiding this comment.
Using name.includes('template') is overly broad and redundant. It will accidentally skip any legitimate package directory that contains the word 'template' (e.g., google-cloud-template-parser). It is safer to match exact directory names like template and templates.
name === 'templates' ||\n name === 'template'
| function compareVersions(v1, v2) { | ||
| const clean = (v) => v.replace(/^[^0-9]+/, ''); | ||
| const parts1 = clean(v1).split(/[-+]/)[0].split('.').map(Number); | ||
| const parts2 = clean(v2).split(/[-+]/)[0].split('.').map(Number); | ||
| for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) { | ||
| const p1 = parts1[i] || 0; | ||
| const p2 = parts2[i] || 0; | ||
| if (p1 > p2) return 1; | ||
| if (p1 < p2) return -1; | ||
| } | ||
|
|
||
| const idx1 = clean(v1).indexOf('-'); | ||
| const idx2 = clean(v2).indexOf('-'); | ||
| const pre1 = idx1 !== -1 ? clean(v1).substring(idx1 + 1) : null; | ||
| const pre2 = idx2 !== -1 ? clean(v2).substring(idx2 + 1) : null; | ||
|
|
||
| if (pre1 && !pre2) return -1; | ||
| if (!pre1 && pre2) return 1; | ||
| if (pre1 && pre2) { | ||
| return pre1.localeCompare(pre2, undefined, { numeric: true, sensitivity: 'base' }); | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Implementing a custom SemVer parser is prone to subtle bugs. For example, the current implementation does not correctly strip build metadata (e.g., +build) from prerelease tags before comparing them, and comparing non-numeric characters (like NaN from invalid version formats) will result in incorrect comparisons. Since this is a Node.js monorepo, using the standard semver package is highly recommended for robustness.
function compareVersions(v1, v2) {\n const semver = require('semver');\n return semver.compare(v1, v2);\n}
| function updateProgress() { | ||
| const total = packages.length; | ||
| const percentage = Math.round((completed / total) * 100); | ||
| const width = 30; | ||
| const filledWidth = Math.round((percentage / 100) * width); | ||
| const emptyWidth = width - filledWidth; | ||
| const bar = '='.repeat(filledWidth) + (filledWidth < width ? '>' : '') + ' '.repeat(Math.max(0, emptyWidth - 1)); | ||
| process.stdout.write(`\r\x1b[36m[Checking NPM Registry]\x1b[0m Progress: ${percentage}% [${bar}] ${completed}/${total}`); | ||
| } |
There was a problem hiding this comment.
Writing carriage returns (\\r) and ANSI escape codes to stdout on every completed task can severely clutter logs in non-TTY environments (such as CI/CD pipelines). Checking process.stdout.isTTY before printing the progress bar avoids this issue.
function updateProgress() {\n if (!process.stdout.isTTY) {\n return;\n }\n const total = packages.length;\n const percentage = Math.round((completed / total) * 100);\n const width = 30;\n const filledWidth = Math.round((percentage / 100) * width);\n const emptyWidth = width - filledWidth;\n const bar = '='.repeat(filledWidth) + (filledWidth < width ? '>' : '') + ' '.repeat(Math.max(0, emptyWidth - 1));\n process.stdout.write(`\\r\\x1b[36m[Checking NPM Registry]\\x1b[0m Progress: ${percentage}% [${bar}] ${completed}/${total}`);\n}
So far there's just the one: a release version discrepancy checker. I'm holding this open a bit though in case anyone else has something.