Raise the file limit for debug artifacts by producing zip64 files where necessary #2842
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the debug artifacts creation process by replacing the AdmZip library with archiver to accommodate zip64 files for large debug artifacts.
- Replaces AdmZip with archiver in both TypeScript and JavaScript files
- Implements asynchronous zip creation through archiver to support a higher file limit
Reviewed Changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/debug-artifacts.ts | Replaces AdmZip with archiver and sets up error/warning event handlers |
| lib/debug-artifacts.js | Mirrors the change in the TypeScript file for JavaScript usage |
Files not reviewed (1)
- package.json: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| zip.addLocalFolder(databasePath); | ||
| zip.writeZip(databaseBundlePath); | ||
| const output = fs.createWriteStream(databaseBundlePath); | ||
| const zip = (0, archiver_1.default)("zip"); |
There was a problem hiding this comment.
The PR title indicates support for large numbers of files with zip64; consider enabling zip64 explicitly by passing {forceZip64: true} to archiver to ensure the archive correctly supports large file sets.
| const zip = (0, archiver_1.default)("zip"); | |
| const zip = (0, archiver_1.default)("zip", { zlib: { level: 9 }, forceZip64: true }); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Suggestion in compiled code.
henrymercer
added
the
Update dependencies
github-actions
bot
removed
the
Update dependencies
|
Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks. |
src/debug-artifacts.ts
Outdated
| zip.on("warning", (err) => { | ||
| throw err; | ||
| }); |
There was a problem hiding this comment.
Two questions:
- In what cases will there be a warning and is this really something we want to raise an error on?
- What will happen if an error is thrown? Where will it be caught and will it be properly handled?
There was a problem hiding this comment.
Looking at the sample code in the packge, the recommendation is something like this:
if (err.code === "ENOENT") {
// log warning
} else {
// throw error
throw err;
}
I'll update.
src/debug-artifacts.ts
Outdated
| zip.on("warning", (err) => { | ||
| throw err; | ||
| }); |
There was a problem hiding this comment.
Looking at the sample code in the packge, the recommendation is something like this:
if (err.code === "ENOENT") {
// log warning
} else {
// throw error
throw err;
}
I'll update.
src/debug-artifacts.ts
Outdated
| }); | ||
|
|
||
| zip.on("warning", (err) => { | ||
| throw err; |
There was a problem hiding this comment.
| throw err; | |
| // Ignore ENOENT warnings. There's nothing anyone can do about it. | |
| if (err.code !== "ENOENT") { | |
| throw err; | |
| } |
aeisenberg
force-pushed
the
henrymercer/zip64
branch
from
117d1f1 to
fd8685f
Compare
|
I pushed the change since Henry is on vacation now. |
The previous library only supported the original zip format, which is limited to 65535 files. This caused issues producing debug artifacts for codebases with large numbers of files.
Merge / deployment checklist