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
Fix packaging with files that contain dots as part of the name #8130
Conversation
Codecov Report
@@ Coverage Diff @@
## v2 #8130 +/- ##
=======================================
Coverage 88.28% 88.28%
=======================================
Files 248 248
Lines 9432 9432
=======================================
Hits 8327 8327
Misses 1105 1105
Continue to review full report at Codecov.
|
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.
but given some changes in the test, it probably needs to be flagged as a breaking change.
@crash7 change looks good to me, still I don't think it's breaking. I believe files with dots in name were expected to be included, and it was a bug they weren't
Did you have any other concern in mind pointing that it should be marked as breaking?
@medikoo Just pointed that because codebases using just
This fill include files with dots and also the handler file. Those codebases will need to change the "exclude all" rule to However I see what you mean, those files should have been included initially and they doesn't due to this bug, so maybe it's fine. Thanks! |
@crash7 thanks for explanation. Do you understand why handling of |
@medikoo while trying to find a good answer to your question, I ended doing a lot of digging and finding something else. Short version: Long version: That version of In fact, Unfortunately, the changelog for On the other hand, Hope you are not bored after reading all of this 😂 |
@medikoo UPDATE: ok, I can't update Also found that one of the tests in my initial commit was wrong, so I fixed that too. Let me know what you think, thanks! |
@medikoo upgrading globby and replacing nanomatch fixed it (e297d82) but the CI check for node6 failed, that's why I reverted the changes. I don't mind rebasing this against v2 if you are planning to release the stable version soon, as long as I can use a |
This ports a fix introduced in serverless-components: serverless-components/aws-lambda#26 but given some changes in the test, it probably needs to be flagged as a breaking change. Fixes serverless#8125
That's clear. In v1 we need to maintain support for Node.js v6, so it's a no go. With v2 we drop support for Node.js v6 and v8 and branch has already a CI update to test against those versions.
It'll be released next week, and it contains few minor (but breaking changes). Check the milestone that outlines them: https://github.com/serverless/serverless/milestone/82 If you do not see any deprecation logs during deployment it means you'll be able to upgrade right away |
c605024
to
fdd8e0c
Compare
fdd8e0c
to
42de6cc
Compare
@medikoo just rebased with
Let me know if you need a hand with something from that milestone. |
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.
Thank you!
@crash7 We appreciate help with any marked by help wanted label, here: https://github.com/serverless/serverless/milestone/82 |
This ports a fix introduced to serverless-components: serverless-components/aws-lambda#26 but given some changes in the test, it probably needs to be flagged as a breaking change.
Closes: #8125