Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace unzipper with unzip-stream #1601

Closed
wants to merge 2 commits into from
Closed

Conversation

jtamsut
Copy link
Contributor

@jtamsut jtamsut commented Dec 8, 2023

There is a bug in the unzipper library that causes files to become corrupted when streamed. This stems from changes from Node's handling of streams. We are replacing unzipper with unzip-stream to as its a drop-in replacement with an identical API that doesn't suffer from the same issue.

Closes https://github.com/github/actions-results-team/issues/2096

@jtamsut jtamsut requested a review from a team as a code owner December 8, 2023 21:11
@jtamsut jtamsut requested a review from a team as a code owner December 8, 2023 21:25
@jtamsut jtamsut changed the title Replaced unzipper with unzip-stream Replace unzipper with unzip-stream Dec 8, 2023
@jtamsut jtamsut changed the title Replace unzipper with unzip-stream Replace unzipper with unzip-stream Dec 8, 2023
@jtamsut jtamsut marked this pull request as draft December 8, 2023 23:12
@@ -9,6 +9,7 @@
"@types/jest": "^29.5.4",
"@types/node": "^20.5.7",
"@types/signale": "^1.4.1",
"@types/unzip-stream": "^0.3.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be installed here, it should be within packages/artifact

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the unzip-stream package doesn't have the #promise method so we'll need something like this at the end of streamExtract:

  return new Promise((resolve, reject) => {
    response.message
      .pipe(unzip.Extract({path: directory}))
      .on('close', resolve)
      .on('error', reject)
  })

@robherley
Copy link
Contributor

Closed in favor of: #1602

@robherley robherley closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants