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

Convert packageModules to use bestzip instead of archiver #596

Merged
merged 15 commits into from Nov 20, 2020

Conversation

liron-strattic
Copy link
Contributor

@liron-strattic liron-strattic commented Jun 28, 2020

What did you implement:

Updated packageModules to use bestzip, allowing the module zip to use the native zip function on the server instead of archiver, if it exists. This greatly speeds up the build time when zipping archives. In our case, it changed from about 25 seconds per function to about 1.5 seconds per function.

Closes #561

How did you implement it:

Replaced the code that uses archiver for bestzip, and tested it to make sure that the resultant zip created by bestzip has the same file structure as the zips currently created by archiver. Bestzip falls back to using archiver, so if zip is not installed on the server it should still function as it does today.

To get the unit tests to pass, I created a bestzip mock, similar to what was previously done for archiver. I think it makes sense as a future enhancement to actually test that the zip is working as expected instead of just mocking it.

How can we verify it:

To verify it needs to be tested by running a package build on a server that contains a native zip function as well as on a server that doesn't, to make sure that both paths of bestzip work.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@miguel-a-calles-mba miguel-a-calles-mba added this to the 5.4.0 milestone Jun 28, 2020
@miguel-a-calles-mba miguel-a-calles-mba changed the base branch from master to release/5.4.0 June 28, 2020 21:20
Copy link
Member

@miguel-a-calles-mba miguel-a-calles-mba left a comment

Choose a reason for hiding this comment

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

May you please add the file conflicts when you get a chance?

@liron-strattic
Copy link
Contributor Author

May you please add the file conflicts when you get a chance?

Done.

Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Any chance to fix failing tests?

package.json Outdated Show resolved Hide resolved
@liron-strattic
Copy link
Contributor Author

liron-strattic commented Jul 23, 2020 via email

@j0k3r
Copy link
Member

j0k3r commented Jul 23, 2020

In that case we can also remove all the archiver mocks for the test framework.

I think so

@j0k3r
Copy link
Member

j0k3r commented Jul 29, 2020

I've rebased against the release/5.4.0 branch and removed archiver stuff (mock and package)

@liron-strattic
Copy link
Contributor Author

Awesome, thanks!

As a side note, I think it might be a good idea to have a test that tests packaging with real, and not mocked zips, especially for performance testing, but I totally understand why it was left out.

@j0k3r
Copy link
Member

j0k3r commented Jul 29, 2020

@liron-strattic I like the idea but it might be more complex to implement.

@j0k3r
Copy link
Member

j0k3r commented Jul 29, 2020

I've tested it on a real project. It saves about 6s (for 8 functions).
Also, I fixed what you did about using the .serverless folder to create the zip because it wasn't working, got that error:

Serverless: Run scripts: /Users/j0k/tests/sw/.webpack/service [1 ms]
Serverless: Zip service: /Users/j0k/tests/sw/.webpack/service [9053 ms]
Serverless: Copying existing artifacts...

  Error --------------------------------------------------

  Error: ENOENT: no such file or directory, copyfile '/Users/j0k/tests/sw/.webpack/dummy-project.zip' -> '.serverless/dummy-project.zip'
      at Object.copyFileSync (fs.js:1790:3)
      at ServerlessWebpack.copyArtifactByName (/Users/j0k/tests/sw/node_modules/serverless-webpack/lib/packageModules.js:84:6)
      at ServerlessWebpack.copyExistingArtifacts (/Users/j0k/tests/sw/node_modules/serverless-webpack/lib/packageModules.js:135:26)
      at ServerlessWebpack.tryCatcher (/Users/j0k/tests/sw/node_modules/bluebird/js/release/util.js:16:23)

@liron-strattic
Copy link
Contributor Author

I think that was an issue because I originally started on a different branch, and that got changed in the release5.4.0 branch.

@j0k3r j0k3r merged commit 4ff8955 into serverless-heaven:release/5.4.0 Nov 20, 2020
@j0k3r
Copy link
Member

j0k3r commented Nov 20, 2020

Thanks @liron-strattic!

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.

Massive Performance Degradation on zipping when upgrading from Node 8
3 participants