Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ The build step (`npm run build`) also runs `npm run docs`, which regenerates the
2. Validates OS compatibility using version config from `src/versions.ts`
3. Optionally installs SQL Native Client (`src/install-native-client.ts`) and ODBC driver (`src/install-odbc.ts`)
4. Downloads or cache-hits the SQL Server installer (box+exe, standalone exe, or SSEI bootstrapper)
5. Optionally downloads cumulative updates
5. Optionally downloads cumulative updates (resolves Microsoft download-page JSON or legacy links, retries transient page-fetch failures, and warns then installs without updates if the download fails)
6. Runs the installer via `@actions/exec`
7. Waits for the database to be ready (exponential backoff)

Expand Down
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ See [action.yml](./action.yml):
```
<!-- end usage -->

When `install-updates: true` is set for a version with a configured update URL,
the action downloads the update before starting SQL Server setup. Transient
download-page failures are tried up to three times. If the update still can't be
downloaded, the action logs a warning with the reason and installs SQL Server
without updates. Versions without a configured update URL skip updates.
Comment on lines +50 to +54

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this paragraph sits between the generated usage block and ### Basic usage without a heading. A ### Cumulative updates heading would make it easier to find and link to. (On "tried up to three times", see my comment on the retry status codes in src/utils.ts.)


### Basic usage

```yml
Expand Down
6 changes: 3 additions & 3 deletions lib/main/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lib/main/index.js.map

Large diffs are not rendered by default.

9 changes: 8 additions & 1 deletion src/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,14 @@ export default async function install() {
if (!config.updateUrl) {
core.info('Skipping update installation - version not supported');
} else {
const updatePath = await core.group(`Fetching cumulative updates for ${version}`, () => findOrDownloadUpdates(config));
const updatePath = await core.group(`Fetching cumulative updates for ${version}`, async () => {
try {
return await findOrDownloadUpdates(config);
} catch (error) {
core.warning(`Unable to download cumulative updates; installing without updates. ${error}`);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: ${error} becomes Error: Unable to …, so the warning reads …installing without updates. Error: Unable to fetch…. error instanceof Error ? error.message : error would read a bit more cleanly. The test regex would need updating to match.

return '';
}
});
if (updatePath) {
installArgs.push('/UPDATEENABLED=1', `/UpdateSource=${dirname(updatePath)}`);
}
Expand Down
97 changes: 74 additions & 23 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { basename, extname, dirname, join as joinPaths } from 'node:path';
import { readdir } from 'node:fs/promises';
import { setTimeout as delay } from 'node:timers/promises';
import * as core from '@actions/core';
import * as exec from '@actions/exec';
import * as glob from '@actions/glob';
Expand Down Expand Up @@ -226,8 +227,75 @@ export async function downloadExeInstaller(config: VersionConfig): Promise<strin
return joinPaths(toolPath, 'setup.exe');
}

function isUpdateDownloadUrl(value: unknown): value is string {
return typeof value === 'string' && /^https:\/\/download\.microsoft\.com\/[^\s"'<>?#]+\.exe$/i.test(value);
}

function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === 'object' && value !== null && !Array.isArray(value);
}

function extractUpdateDownloadUrl(body: string): string {
const links = new Set<string>();
for (const [, script] of body.matchAll(/<script\b[^>]*>([\s\S]*?)<\/script\s*>/gi)) {
const assignment = script.match(/^\s*window\.__DLCDetails__\s*=\s*(\{[\s\S]*\})\s*;?\s*$/);
if (!assignment) continue;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: the rest of the codebase always uses braces for if bodies (also lines 257 and 262), so it'd be good to match that here. Similarly, const [link] = links; return link; on line 270 would avoid the non-null assertion.

let details: unknown;
try {
details = JSON.parse(assignment[1]);
} catch (error) {
throw new Error('Invalid cumulative update metadata in Microsoft download page', { cause: error });
}
if (!isRecord(details) || !isRecord(details.dlcDetailsView)) {
throw new Error('Invalid cumulative update metadata in Microsoft download page');
}
const files = details.dlcDetailsView.downloadFile;
if (!Array.isArray(files)) {
throw new Error('Invalid cumulative update file list in Microsoft download page');
}
Comment on lines +244 to +255

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could problems with the metadata fall back to the <a href> scan below instead of throwing? Today all three CU pages carry both the __DLCDetails__ JSON and a legacy download link. If Microsoft renames dlcDetailsView/downloadFile, or the JSON stops parsing, this would fail in cases where the old href regex would still have worked. We want the extra parser to make us more resilient, not less.

I'd suggest treating any parse or shape problem as "no usable metadata": log the reason with core.debug and carry on to the link scan. Only throw if neither path finds an installer, and include the metadata problem in that final error so it isn't lost. The rejects malformed metadata tests would then become "falls back to legacy links when metadata is malformed" cases, plus one where there's no link either.

for (const file of files) {
if (isRecord(file) && isUpdateDownloadUrl(file.url)) links.add(file.url);
}
}
if (!links.size) {
for (const [, link] of body.matchAll(/<a\b[^>]*?\s+href\s*=\s*["']([^"']+)["'][^>]*>/gi)) {
if (isUpdateDownloadUrl(link)) links.add(link);
}
}
if (links.size !== 1) {
throw new Error(links.size
? 'Multiple cumulative update installers found in Microsoft download page'
: 'No HTTPS download.microsoft.com .exe cumulative update installer found in Microsoft download page');
}
Comment on lines +265 to +269

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: the previous implementation called core.debug(body) when it couldn't find a link. That's handy the next time Microsoft changes the page, because you can re-run with debug logging and see exactly what we received. Could we keep that before throwing here?

Also, the metadata has a dlcDetailsView.error field (empty today). If Microsoft ever fills it in, including it in this error would tell us more than "no installer found".

return links.values().next().value!;
}

async function fetchUpdatePage(url: string): Promise<string> {
const attempts = 3;
for (let attempt = 1; ; attempt++) {
let retryable = false;
try {
const res = await fetch(url, { signal: AbortSignal.timeout(30_000) });
if (!res.ok) {
retryable = res.status === 408 || res.status === 429 || res.status >= 500;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The README now says transient download-page failures are retried, but the failures we've actually seen from these pages are 403 and 404 (the tedious runs mentioned in the description). Those turned out to be transient, since the page was back to 200 shortly afterwards.

updateUrl is a fixed, known-good details page rather than user input, so I think it's reasonable to retry any non-2xx here, or at least 403 and 404. If a page is ever genuinely retired, the worst case is a few seconds' delay before the warning. A slightly longer backoff than 1s/2s might also improve the odds of recovering. The fails explicitly without retrying HTTP ${status} tests would flip to asserting the retries.

throw new Error(`HTTP ${res.status}`);
}
return await res.text();
} catch (error) {
retryable ||= error instanceof TypeError || (error instanceof Error && error.name === 'TimeoutError');
const reason = error instanceof Error ? error.message : String(error);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: for network failures, undici's message is just fetch failed. The useful detail (e.g. ECONNRESET, ENOTFOUND, UND_ERR_SOCKET) is on error.cause, and it gets dropped from the message when we wrap it here. Since the aim is to show the reason in the warning, it'd be worth appending the cause's message or code when there is one.

if (!retryable || attempt === attempts) {
throw new Error(`Unable to fetch cumulative update page ${url} after ${attempt} attempt(s): ${reason}`, { cause: error });
}
core.warning(`Cumulative update page fetch failed (${reason}); retrying (${attempt + 1}/${attempts})`);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: core.warning adds an annotation to the run summary, so a fetch that succeeds on its second attempt still leaves a warning on an otherwise green job. Could we use core.info for the intermediate retries? The warning in install.ts still flags the case where we give up. (@actions/tool-cache logs its own download retries at info level too.)

await delay(1000 * attempt);
}
}
}

/**
* Downloads cumulative updates for supported versions.
* Downloads cumulative updates for supported versions. Throws with the failure
* reason if a configured update cannot be fetched or resolved.
*
* @param {VersionConfig} config
* @returns {Promise<string>}
Expand All @@ -236,28 +304,11 @@ export async function downloadUpdateInstaller(config: VersionConfig): Promise<st
if (!config.updateUrl) {
throw new Error('No update url provided');
}
// resolve download url
let downloadLink: string | null = null;
if (!config.updateUrl.endsWith('.exe')) {
const res = await fetch(config.updateUrl);
if (res.ok) {
const body = await res.text();
const [, link] = body.match(/\s+href\s*=\s*["'](https:\/\/download\.microsoft\.com\/.*\.exe)['"]/) ?? [];
if (link) {
downloadLink = link;
} else {
core.info('Unable to find download link in body');
core.debug(body);
}
}
if (!downloadLink) {
core.warning('Unable to download cumulative updates');
core.info(`Response code: ${res.status}`);
return '';
}
}
core.info(`Downloading cumulative update from ${downloadLink ?? config.updateUrl}`);
const updatePath = await downloadTool(downloadLink ?? config.updateUrl);
const downloadLink = config.updateUrl.endsWith('.exe')
? config.updateUrl
: extractUpdateDownloadUrl(await fetchUpdatePage(config.updateUrl));
core.info(`Downloading cumulative update from ${downloadLink}`);
const updatePath = await downloadTool(downloadLink);
if (core.isDebug()) {
const hash = await generateFileHash(updatePath);
core.debug(`Got update file with hash SHA256=${hash.toString('base64')}`);
Expand Down
11 changes: 11 additions & 0 deletions test/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,17 @@ describe('install', () => {
assert.ok(args.includes('/UPDATEENABLED=1'));
assert.ok(args.includes('/UpdateSource=C:/tool-cache/sql-update'));
});
it('installs without updates and warns if requested updates fail', async () => {
utils.gatherInputs.mock.mockImplementation(() => defaultInputs({ installUpdates: true }));
utils.downloadUpdateInstaller.mock.mockImplementation(async () => {
throw new Error('Unable to fetch cumulative update page: HTTP 403');
});
await install();
const args = exec.exec.mock.calls[0].arguments[1] as string[];
assert.ok(!args.includes('/UPDATEENABLED=1'));
assert.ok(!args.some((arg) => arg.startsWith('/UpdateSource=')));
assert.ok(core.warning.mock.calls.some((call) => /installing without updates\. Error: Unable to fetch cumulative update page: HTTP 403/.test(String(call.arguments[0]))));
});
it('skips cumulative updates if no update url', async () => {
utils.gatherInputs.mock.mockImplementation(() => defaultInputs({ version: 'minOs', installUpdates: true }));
await install();
Expand Down
Loading