Raise the file limit for debug artifacts by producing zip64 files where necessary - #2842
aeisenberg merged 3 commits into
Conversation
Sorry, something went wrong.
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 |
- package.json: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
Sorry, something went wrong.
| zip.addLocalFolder(databasePath); | ||
| zip.writeZip(databaseBundlePath); | ||
| const output = fs.createWriteStream(databaseBundlePath); | ||
| const zip = (0, archiver_1.default)("zip"); |
Copilot
AI
Apr 4, 2025
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 }); |
Sorry, something went wrong.
There was a problem hiding this comment.
Suggestion in compiled code.
Sorry, something went wrong.
|
Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks. |
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| }); | ||
|
|
||
| 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; | |
| } |
Sorry, something went wrong.
|
I pushed the change since Henry is on vacation now. |
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
Sorry, something went wrong.
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