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
Add AwsCompileStreamEvents plugin #2250
Conversation
@eahefnawy would be great if you could review it |
7ee580c
to
9228a35
Compare
I'm personally not sure if it's not confusing that there is one event type ( Would be great if docs would also show how to create Kinesis stream via custom resources and pass ARN to event source config. |
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.
@pmuens we shouldn't put the table name in the logical ID of the event mapping resource. I tried using a table named test-stream
and it failed because it's not alpha numeric.
Other than that, it's working great. 👍 for handling the permissions as well, it's interesting that this is the first event that actually requires the lambda function to have permission via roles, not the other way around by adding a lambda:permission resource.
9228a35
to
c7ac744
Compare
@eahefnawy @mthenw thanks for the feedback. I just refactored the resource logical Id name generation so that all non-alphanumerics are removed (so e.g. We need those names inside of the resource logical id so that they are unique (and also easier to overwrite via custom resources). I've tried to do it with a counter variable (like it's done in other plugins) but this will break when you re-arrange or update the streams. |
@pmuens yeah you're right ... I was about to suggest removing the G2M from my side 👍 |
Awesome! Thanks for the review @eahefnawy @flomotlik could you take a look at it and merge it if it's appropriate? Thanks! |
ccad494
to
9d63ba0
Compare
events: | ||
- stream: | ||
arn: some:kinesis:stream:arn | ||
bathSize: 100 |
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.
Is this a typo? bathSize
-> batchSize
?
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.
Damn! Good catch @screamish
Just pushed an update which will fix this!
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.
"bathSize" 😂
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.
😹 #documentationGems
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.
Good stuff. A few changes in the docs to make sure that users are aware that we're not actually creating the Kinesis or DynamoDB ourselves
@@ -0,0 +1,56 @@ | |||
# Compile Stream Events |
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 don't think we need this README. Its basically just a copy of the documentation so the whole file can be removed.
|
||
# DynamoDB / Kinesis Streams | ||
|
||
This setup specifies that the `compute` function should be triggered whenever the corresponding DynamoDB table is modified (e.g. a new entry is added). |
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.
Mention that we do not create the DynamoDB, but it only connects to a previously created one.
compute: | ||
handler: handler.compute | ||
events: | ||
- stream: some:dynamodb:stream:arn |
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.
Add 2 different stream configs, one with an ARN and one that uses the Ref syntax and mention that you can reference a dynamodb or kinesis stream created in the same service through Ref
|
||
## Setting the BatchSize and StartingPosition | ||
|
||
This configuration sets up a disabled Kinesis stream event for the `preprocess` function which has a batch size of `100`. The starting position is |
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.
Same as above, mention that you're not creating the Kinesis Stream
This way both, DynamoDB and Kinesis streams can be used with the help of the "stream" event rather than two different event types ("dynamodb" and "kinesis").
dc33892
to
b6f60b6
Compare
@flomotlik just update the PR with the proposed changes. Unfortunately we cannot use Furthermore I've added a commented out example to all the |
What did you implement:
_Implementing Issue:_ #1441 and #1608
Adds support for the
stream
event definition (which covers DynamoDB and Kinesis streams). No need to set it up through custom resources anymore.How did you implement it:
Introduced a new plugin which will compile the corresponding EventSourceMappings for CloudFormation. Furthermore the
IamRoleStatements
are updated so that the Lambda function can be called when something in the stream is happening.How can we verify it:
First of all you need to create a new DynamoDB table and set up a new Stream for that table (In
Overview
-->Stream details
-->Manage Stream
/ Kinesis stream . Copy and paste the ARN for the corresponding stream.Next up use this
serverless.yml
file and paste the ARN you've just copied:Todos:
_Is this ready for review?:_ YES