-
Notifications
You must be signed in to change notification settings - Fork 417
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
Speed up cleanup process #462
Conversation
Any chance to get this PR reviewed ? |
@serverless-heaven/serverless-webpack-contributors Can someone help to review this PR? If it is ok, it will be part of 5.3.0 |
I'll take a look at it right now... |
There was a problem hiding this 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!
@designfrontier : I've made some change tell me if it's good to you now, thank you. |
There was a problem hiding this 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
one super tiny thing. Thanks for your work on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@designfrontier Can you finish the review and eventually merge? |
Yeah I’ll take a look today
… On Apr 14, 2019, at 07:47, Frank Schmid ***@***.***> wrote:
@designfrontier Can you finish the review and eventually merge?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 |
There was a problem hiding this 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!
What did you implement:
Removing ".webpack" directory may be is very slow on when this directory is huge.
How did you implement it:
How can we verify it:
Run serverless deploy
Todos:
Is this ready for review?: YES
Is it a breaking change?: NO