FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support noDeploy option when use layers by naoyasugita · Pull Request #759 · serverless/serverless-python-requirements · GitHub

This repository was archived by the owner on Oct 24, 2025. It is now read-only.
/ serverless-python-requirements Public archive

Support noDeploy option when use layers - #759

Open
naoyasugita wants to merge 7 commits into
serverless:masterfrom
naoyasugita:enable-noDeploy-option-with-layer
ConversationCommits (7)ChecksFiles changed
Open

Support noDeploy option when use layers#759
naoyasugita wants to merge 7 commits into
serverless:masterfrom
naoyasugita:enable-noDeploy-option-with-layer

Conversation

naoyasugita commented Feb 12, 2023

Copy link
Copy Markdown

This change allows use of noDeploy option with the layers.

Should resolve:
#577

emann mentioned this pull request Mar 29, 2023
Open

emann commented Mar 29, 2023

Copy link
Copy Markdown

Thank you @naoyasugita! I need this as well, looking forward to it being merged.

naoyasugita commented Apr 15, 2023

Copy link
Copy Markdown
Author

Hi @pgrzesik - I'd be happy to check in with you when you're ready.

kaiosyng commented Jun 19, 2023

Copy link
Copy Markdown

I hope this gets merged soon!

pgrzesik reviewed Aug 1, 2023
View reviewed changes

pgrzesik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/zipTree.js Outdated
* @param {Object} noDeploy
* @return {Promise} a promise offering the original JSZip object.
*/
function addTreeNoDeploy(zip, src, noDeploy) {

pgrzesik Aug 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we don't need a new function just for that as most of the logic is copied over from the original addTree. I think it would be better to just have the optional argument of noDeploy to the original function. What do you think?

naoyasugita Aug 3, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the review :)
You are right. I'll fix it along with the test.

naoyasugita Aug 3, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@pgrzesik I'm done adding test and fixing the code. Please check it out.

pgrzesik commented Aug 17, 2023

Copy link
Copy Markdown
Contributor

Hey @naoyasugita - it looks like the tests are currently failing. Could you please look into that whenever you have a chance? 🙇

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
pgrzesik pgrzesik left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL