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

Fix jbrowse create/jbrowse upgrade CLI intermittent failures #3686

Merged
merged 2 commits into from May 8, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented May 5, 2023

Some people are reporting corrupt files from node-unzipper both to our project and in issues on their repo e.g. ZJONSSON/node-unzipper#271

In the absence of this getting fixed upstream, this PR proposes using an alternative library, yauzl (yet another un-zip library) with a front end provided by npm package 'decompress' (see https://github.com/kevva/decompress-unzip/blob/master/package.json#L31) since yauzl is a bit tricky to use by itself

The firebase project maybe ran into same issue and created their own unzipper based on native nodejs zlib which seemed nice but it's not published on npm and I couldn't get it to work firebase/firebase-tools#5714

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label May 5, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented May 5, 2023

This PR only speculatively fixes the issue as i haven't been able to reproduce issues people are experiencing

@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels May 5, 2023
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented May 5, 2023

Note that this PR changes from using non-streaming unzip which should be more stable anyways, the https://github.com/thejoshwolfe/yauzl/ readme describes how streaming zip is pretty awkward at best

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #3686 (8bda020) into main (b904fce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3686   +/-   ##
=======================================
  Coverage   62.60%   62.61%           
=======================================
  Files         894      894           
  Lines       30246    30246           
  Branches     7322     7322           
=======================================
+ Hits        18937    18939    +2     
+ Misses      11126    11124    -2     
  Partials      183      183           
Impacted Files Coverage Δ
products/jbrowse-cli/src/commands/create.ts 88.88% <100.00%> (ø)
products/jbrowse-cli/src/commands/upgrade.ts 76.31% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin merged commit 21e2cd0 into main May 8, 2023
12 checks passed
@cmdcolin cmdcolin changed the title Fix CLI unzipping having intermittent failure Fix jbrowse create/jbrowse upgrade CLI intermittent failures May 8, 2023
@cmdcolin cmdcolin deleted the fix_cli_intermittent_fail branch May 8, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant