Repository navigation
fix: resolve cumulative update links from new Microsoft download pages - #295
cheenamalhotra wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the stated behavior and is supported by focused, comprehensive tests.
Review effort: Balanced
Findings: None
What changed in this PR
Updates cumulative-update resolution for Microsoft’s JSON-based download pages while preserving legacy links and warn-and-continue behavior.
Changes:
- Securely parses and validates update installer URLs.
- Retries transient page-fetch failures with backoff.
- Adds comprehensive tests and documentation.
| File | Description |
|---|---|
.github/copilot-instructions.md |
Documents the updated installation flow. |
README.md |
Explains retry and fallback behavior. |
lib/main/index.js |
Updates the generated action bundle. |
lib/main/index.js.map |
Updates the generated source map. |
src/install.ts |
Warns and continues when updates fail. |
src/utils.ts |
Adds URL extraction, validation, and retries. |
test/install.ts |
Tests warn-and-continue behavior. |
test/utils.ts |
Tests parsing, validation, errors, and retries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dhensby
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @cheenamalhotra!
Parsing the __DLCDetails__ metadata (without executing anything) and limiting accepted URLs to HTTPS download.microsoft.com .exe links is good future-proofing. Surfacing the actual failure reason in the warning will also make these failures much easier to diagnose next time. I'm happy with warning and installing without updates rather than failing the action.
I've left a few comments inline. The main ones are falling back to the href scan when the metadata can't be used, and retrying 403/404 so we match what the README now says. The rest are nits.
Commits: the two commits currently contradict each other. 42f3e01 says the action will now fail when a requested update can't be resolved or downloaded, and 5a5a08f changes that back to warn-and-continue. We merge with merge commits, so both would land on master and both would show up in the changelog as bug fixes with opposite descriptions. Could you squash them into a single fix: commit whose message describes the final behaviour?
SQL Server 2025 updates: Microsoft now publishes cumulative updates for 2025 at https://www.microsoft.com/en-us/download/details.aspx?id=108788 (currently KB5122048), and the new metadata parsing resolves that page to a single x64 installer. That means the comment in src/versions.ts saying the updateUrl can be added once Microsoft publishes them is out of date. This PR doesn't need to cover it, but if you want to add the URL, a separate commit would be great. Otherwise we can pick it up in a follow-up.
Heads-up: this conflicts with #296 (README, the node:fs/promises import in src/utils.ts, the module mocks in test/utils.ts, and lib/). Whichever PR lands second will need a rebase and a fresh npm run build.
Parse Microsoft download metadata without running scripts and fall back to legacy links when metadata is unusable. Accept only HTTPS Microsoft EXE URLs and retain metadata, page and network failure diagnostics. Retry all non-2xx download-page responses, network errors and timeouts up to three times with 5s/10s delays. Log retries at info level and warn then install without updates if resolution or download ultimately fails. Refs tediousjs/tedious#1807 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
5a5a08f to
de2365c
Compare
|
Squashed the two contradictory commits and the review fixes into de2365c, rebased onto current master, and refreshed the generated action bundle. The commit message now describes the final warn-and-continue behavior. I have left SQL Server 2025 cumulative updates for a separate follow-up, as suggested. |
Fixes the cumulative update downloader for tediousjs/tedious#1807.
Microsoft download pages now put the installer URL in embedded JSON, which the old
hrefregex didn't handle reliably.Changes
window.__DLCDetails__JSON withJSON.parse(page scripts are never run), and fall back to legacy<a href>links when metadata is missing, malformed, or unusable.download.microsoft.com.exeURLs. Report an error if none or more than one is found, preserving metadata diagnostics and logging the page body at debug level..exeupdate URLs (2016) are unchanged.Testing
Review follow-up
masterand squashed the contradictory commits into onefix:commit describing the final warn-and-continue behavior.Note: the linked tedious CI runs got HTTP 404/403 from the 2022 page. Those responses are now retried, and persistent failures retain the actual reason in the warning.
@arthurschreiber @saurabh500 @David-Engel