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

Speed up cleanup process #462

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Speed up cleanup process #462

merged 1 commit into from
Jul 30, 2020

Conversation

neilime
Copy link
Contributor

@neilime neilime commented Nov 11, 2018

What did you implement:

Removing ".webpack" directory may be is very slow on when this directory is huge.

How did you implement it:

  • Performs remove async (no need to wait the remove of this directory before running the next process)

How can we verify it:

Run serverless deploy

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

@neilime neilime changed the title Improve cleanup process for win32 platform Speed up cleanup process Dec 17, 2018
@neilime
Copy link
Contributor Author

neilime commented Jan 27, 2019

Any chance to get this PR reviewed ?

@HyperBrain
Copy link
Member

@serverless-heaven/serverless-webpack-contributors Can someone help to review this PR? If it is ok, it will be part of 5.3.0

@designfrontier
Copy link
Contributor

I'll take a look at it right now...

@designfrontier designfrontier self-requested a review April 7, 2019 13:31
Copy link
Contributor

@designfrontier designfrontier left a comment

Choose a reason for hiding this comment

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

A few small changes and then it should be good to go. Looks great! If you wouldn't mind resolving the conflict while you are updating this patch that would be fantastic.

Thanks for your contribution!

lib/cleanup.js Outdated Show resolved Hide resolved
lib/cleanup.js Outdated Show resolved Hide resolved
@neilime
Copy link
Contributor Author

neilime commented Apr 7, 2019

@designfrontier : I've made some change tell me if it's good to you now, thank you.

Copy link
Contributor

@designfrontier designfrontier left a comment

Choose a reason for hiding this comment

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

Sorry for the nitpick on spaces... everything else looks amazing

lib/cleanup.js Outdated Show resolved Hide resolved
lib/cleanup.js Outdated Show resolved Hide resolved
@designfrontier
Copy link
Contributor

one super tiny thing. Thanks for your work on this

@designfrontier designfrontier added this to the 5.3.0 milestone Apr 10, 2019
Copy link
Contributor

@hassankhan hassankhan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@HyperBrain
Copy link
Member

@designfrontier Can you finish the review and eventually merge?

@designfrontier
Copy link
Contributor

designfrontier commented Apr 14, 2019 via email

@hassankhan
Copy link
Contributor

Just had a chance to test it on my own projects, seems to work fine but can't really tell since I don't have large deployment sizes.

Copy link
Contributor

@designfrontier designfrontier left a comment

Choose a reason for hiding this comment

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

LGTM

@designfrontier
Copy link
Contributor

Actually... on a second look it looks like there is a new branch in the code that is untested. Details are here: https://coveralls.io/builds/23015127/source?filename=lib/cleanup.js

I think you should be able to test this by setting everything up and removing the webpack directory before you run cleanup. Then checking that you get the correct error output in the log with verbose set.

Bonus points would be testing that ternary that changes where the logs go

@designfrontier designfrontier self-assigned this May 2, 2019
@HyperBrain HyperBrain modified the milestones: 5.3.1, 5.4.0 Jun 6, 2019
@neilime neilime requested a review from hassankhan April 14, 2020 21:50
@miguel-a-calles-mba miguel-a-calles-mba changed the base branch from master to release/5.4.0 May 6, 2020 23:14
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.

I've squashed all commits, added more tests to increase coverage and rebased against the release/5.4.0 branch.
I think it's ready to be merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants