- Notifications
You must be signed in to change notification settings - Fork294
Support noDeploy option when use layers#759
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Support noDeploy option when use layers#759
Conversation
emann commentedMar 29, 2023
Thank you@naoyasugita! I need this as well, looking forward to it being merged. |
Hi@pgrzesik - I'd be happy to check in with you when you're ready. |
kaiosyng commentedJun 19, 2023
I hope this gets merged soon! |
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.
Thanks for proposing the fix and sorry for not responding sooner. Please see my comment and let me know what do you think. Also, would be good if we could have a test case covering this.
lib/zipTree.js Outdated
| * @param {Object} noDeploy | ||
| * @return {Promise} a promise offering the original JSZip object. | ||
| */ | ||
| function addTreeNoDeploy(zip, src, noDeploy) { |
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 think we don't need a new function just for that as most of the logic is copied over from the originaladdTree. I think it would be better to just have the optional argument ofnoDeploy to the original function. What do you think?
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.
Thanks for the review :)
You are right. I'll fix it along with the test.
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.
@pgrzesik I'm done adding test and fixing the code. Please check it out.
Hey@naoyasugita - it looks like the tests are currently failing. Could you please look into that whenever you have a chance? 🙇 |
This change allows use of noDeploy option with the layers.
Should resolve:
#577