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
feat(AJV): Add missing provider level schema properties #8297
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8297 +/- ##
==========================================
- Coverage 88.08% 88.02% -0.07%
==========================================
Files 250 249 -1
Lines 9368 9312 -56
==========================================
- Hits 8252 8197 -55
+ Misses 1116 1115 -1
Continue to review full report at Codecov.
|
@medikoo, while working on this PR, I stumbled upon S3 bucket name validation, for which one full service was dedicated (https://github.com/serverless/serverless/blob/master/lib/plugins/aws/lib/validateS3BucketName.js). This service allows use of However, as specified by AWS documentation for bucket name, bucket name including dots won't be able to use S3 transfer acceleration. Therefore, using a logic similar to the discussion we had regarding schema limitation of lambda env (limited to Ref and GetAtt only since |
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 @fredericbarthelet ! It looks very promising. Great cleanup, I've picked just few potential issues.
Therefore, using a logic similar to the discussion we had regarding schema limitation of lambda env (limited to Ref and GetAtt only since invoke local only support those), would you like me to limit bucket name, when specified, to non-doted ones only ?
Good question. I would enable dot, and ensure we throw with meaningful error if it's used and S3 acceleration is enabled (it's opt-in as I see).
Also as I think more about it now, we should have done probably the same to env vars (so allow all formats acceptable for deployment, but throw with meaningful error if local invocation is pursued with not supported env vars - but let's not cover this in this PR)
lib/plugins/aws/package/compile/events/apiGateway/lib/hack/updateStage.js
Outdated
Show resolved
Hide resolved
S3 acceleration is already disabled when a custom bucketName is provided, wether it includes a dot or not, thanks to the following code block serverless/lib/plugins/aws/package/lib/generateCoreTemplate.js Lines 78 to 86 in eb5e548
The original commit I made allowed for dot within bucket name. Considering your answer, I keep it that way :) |
a8bcf31
to
e4a4fc5
Compare
e4a4fc5
to
310fdc8
Compare
@fredericbarthelet indeed, good point, btw. I've already improved env vars handling with #8307, so that's covered |
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.
That looks great @fredericbarthelet I've commented just on a few last things, and we should be good to go!
310fdc8
to
0e05483
Compare
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.
@fredericbarthelet this looks very good, and seems ready to take! I have just one concern which I forgot to add in previous reviews. Let me know.
0056001
to
f764377
Compare
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 @fredericbarthelet! It's great to have that in
Closes: #8016