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: replace unzipper to resolve corrupt binaries on node > v18.15 #5161

Closed
wants to merge 2 commits into from

Conversation

shumailxyz
Copy link
Contributor

@shumailxyz shumailxyz commented Sep 29, 2023

What this PR does / why we need it:
Replaces unzipper with unzip-stream due to upstream issue ZJONSSON/node-unzipper#271 not being fixed yet. And that was blocking us from updating our node version from v18.15.

Replaced with unzip-stream as the api is almost the same and it does resolve the issue.

Also a step forward for #4158

Which issue(s) this PR fixes:

Fixes #4467

#4467 was previously fixed by pinning the node version to 18.15.

Special notes for your reviewer:

  • I built and published the runner image to dockerhub already and the CI uses the new image for this PR.

  • Also looked into the yauzl as that seems to be most popular package for dealing with zip files however yauzl doesn't support streams.

  • Please also test it by running and using garden util fetch-tools.

@shumailxyz shumailxyz changed the title chore: replace unzipper to resolve corrupt binaries on node > v18.15 fix: replace unzipper to resolve corrupt binaries issue on node > v18.15 Sep 29, 2023
@shumailxyz shumailxyz changed the title fix: replace unzipper to resolve corrupt binaries issue on node > v18.15 fix: replace unzipper to resolve corrupt binaries on node > v18.15 Sep 29, 2023
@shumailxyz
Copy link
Contributor Author

Example, from few months back, of the failing CI step due to corrupt binaries issue before we downgraded to node v18.15.

https://app.circleci.com/pipelines/github/garden-io/garden/17991/workflows/f023b92f-2435-443c-94fd-1bd0b57cf084/jobs/314086

@shumailxyz shumailxyz marked this pull request as ready for review September 29, 2023 10:35
@shumailxyz shumailxyz requested a review from a team September 29, 2023 10:35
Copy link
Contributor

@TimBeyer TimBeyer left a comment

Choose a reason for hiding this comment

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

That's great. Should we also load pkg from github so that it uses the newest pgk-fetch version and updates the CLI node version then?

@shumailxyz
Copy link
Contributor Author

@TimBeyer We could, my only concern is about the stability if pkg if we fetch the main branch as that could be unstable. Although, it doesn't seem to be actively developed anymore so the risk is low.

@TimBeyer
Copy link
Contributor

We also should pin it to a specific commit and then revisit that every once in a while.

@shumailxyz
Copy link
Contributor Author

How about this approach and overriding the pkg-fetch?

vercel/pkg#1998 (comment)

@TimBeyer
Copy link
Contributor

Actually let's try this instead: vercel/pkg#1998 (comment)

@TimBeyer
Copy link
Contributor

Haha exactly

@TimBeyer
Copy link
Contributor

Package overrides are somehow fundamentally broken in npm so we went with the github install instead.

@TimBeyer TimBeyer force-pushed the replace-unzipper branch 2 times, most recently from 902843d to f414b8e Compare September 30, 2023 09:31
@TimBeyer
Copy link
Contributor

pkg-fetch in the newest version doesn't have prebuilt binaries for macos-arm64 and would need them to be compiled during the build. That fails since we're not building on macos.
Will skip this for now and come back to this some time in the future.

@shumailxyz shumailxyz enabled auto-merge October 5, 2023 13:49
@vvagaytsev vvagaytsev disabled auto-merge October 5, 2023 14:43
Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

There are some test failures, please check.

@stefreak stefreak mentioned this pull request Nov 2, 2023
@stefreak
Copy link
Member

stefreak commented Nov 6, 2023

This has been superseded by #5233 (We had to cherry-pick due to Node update in the ESM PR)

@stefreak stefreak closed this Nov 6, 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.

Bug: plugin / tool installation can result in corrupt binaries
4 participants