feat(aws stream): provisioned concurrency support for aws stream event #8342
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8342 +/- ##
=======================================
Coverage 88.01% 88.02%
=======================================
Files 249 249
Lines 9306 9312 +6
=======================================
+ Hits 8191 8197 +6
Misses 1115 1115
Continue to review full report at Codecov.
|
| @@ -22,3 +22,18 @@ functions: | |||
| - Ref: AWS::Region | |||
| - Ref: AWS::AccountId | |||
| - ${self:service.name}-provisioned | |||
| provisionedKinesis: | |||
There was a problem hiding this comment.
How about using same function for testing both?
That'll probably ensure that we do not prolong integration test time.
It feels a bit quirky, but I think it still tests well setup (or do I miss something?)
confirmCloudWatchLogs has checkIsComplete option, through which we may ensure that second test waits for right logs to confirm that trigger worked
There was a problem hiding this comment.
Thanks about the suggestion to use checkIsComplete 👍 I've migrated to one function that handles both integration test cases
| expect( | ||
| awsCompileStreamEvents.serverless.service.provider.compiledCloudFormationTemplate.Resources | ||
| .FirstEventSourceMappingDynamodbFoo.DependsOn | ||
| ).to.have.lengthOf(1); |
There was a problem hiding this comment.
I wouldn't touch those tests (Ideally all such tests should be refactored to rely on runServelress, but ofc that's not in a scope of this PR).
They're pain to work with on any refactors, so adding to them increases maintenance burden
There was a problem hiding this comment.
It's a slight adjustment to them as to make the things a bit easier I defaulted to have dependsOn as an array instead of a string if it's depending on just one resource. What would you suggest here then?
As for refactoring to rely on runServerless - I was thinking about it, but given the fact that there's an issue that I would like to tackle in the coming days (#8137) I felt like refactoring these tests is not worth it at the moment
There was a problem hiding this comment.
It's a slight adjustment to them as to make the things a bit easier I defaulted to have dependsOn as an array instead of a string if it's depending on just one resource. What would you suggest here then?
Update existing tests, that started to fail and need to be updated cause of that change, but do not introduce a new ones
I felt like refactoring these tests is not worth it at the moment
Yes, it could be time taking. At some point it'll be good if we do it though, as it'll allow to do greater internal rerfactors without a need of doing full test rewrite.
There was a problem hiding this comment.
Adjusted 💯
Yes, it could be time taking. At some point it'll be good if we do it though, as it'll allow to > do greater internal rerfactors without a need of doing full test rewrite.
I totally agree with that approach, I just had in mind this specific test suite, as if #8137 will be implemented, stream will be eventually removed
Codecov Report
@@ Coverage Diff @@
## master #8342 +/- ##
==========================================
+ Coverage 88.01% 88.05% +0.03%
==========================================
Files 249 249
Lines 9306 9293 -13
==========================================
- Hits 8191 8183 -8
+ Misses 1115 1110 -5
Continue to review full report at Codecov.
|
pgrzesik
force-pushed
the
provisioned-concurrency-support-for-aws-stream-event
branch
from
cdb9433
to
9d53840
Compare
| await createSqsQueue(queueName); | ||
| log.notice(`Creating Kinesis stream "${streamName}"...`); | ||
| await createKinesisStream(streamName); |
There was a problem hiding this comment.
I think we can create them in parallel (?)
There was a problem hiding this comment.
of course - great idea 👍
| log.notice(`Deleting Kinesis stream "${streamName}"...`); | ||
| await deleteKinesisStream(streamName); | ||
| log.notice(`Deleting SQS queue "${queueName}"...`); | ||
| return deleteSqsQueue(queueName); |
Ensures that
EventSourceMappingforstreamevents has properaliasattached if one exists, It also ensures that the mapping resource will depend on that alias. This PR also adds a corresponding integration test case forprovisionedConcurrency.test.js.Note: The average time it takes to run the test now is pretty much the same, around 7-8 minutes.
Closes: #7802