Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(lambda-event-sources): disable event source #6541

Closed
wants to merge 2 commits into from
Closed

feat(lambda-event-sources): disable event source #6541

wants to merge 2 commits into from

Conversation

zeynepsu
Copy link

@zeynepsu zeynepsu commented Mar 2, 2020

One concern may be that there are a lot of event source mapping options specific to streams that are not reflected in the CDK code base: https://docs.aws.amazon.com/lambda/latest/dg/API_CreateEventSourceMapping.html. If perceived to be needed ASAP, those can be added to the StreamEventSourceProps in this CR as well. I checked that CloudFormation does support all of these options. However, only passing the enabled flag helps close the feature request #5750 and limits the scope of the CR.
Unit tests are added and verification was added to existing integration tests as well. I also manually verified that all 3 event sources that support this (dynamo, sqs and kinesis) start out as disabled.

closes #5750

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@zeynepsu zeynepsu force-pushed the optional-disabled-parameter-for-lambda-event-sources branch from 67e362e to 45e4d4c Compare March 2, 2020 17:13
feat(aws-lambda-event-sources): add optional disabled event source ma…
4578bdf 
…pping options parameter to supported event sources in Lambda

closes aws#5750
@zeynepsu zeynepsu force-pushed the optional-disabled-parameter-for-lambda-event-sources branch from 45e4d4c to 4578bdf Compare March 2, 2020 17:16
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 557536f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 45e4d4c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@zeynepsu zeynepsu marked this pull request as ready for review March 2, 2020 17:55
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 67e362e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4578bdf
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: fddf7eb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nija-at nija-at changed the title feat(aws-lambda-event-sources): add optional disabled event source mapping options parameter to supported event sources in Lambda feat(lambda-event-sources): disable event source Mar 9, 2020
@nija-at
Copy link
Contributor

nija-at commented Mar 9, 2020

ne concern may be that there are a lot of event source mapping options specific to streams that are not reflected in the CDK code base: https://docs.aws.amazon.com/lambda/latest/dg/API_CreateEventSourceMapping.html. If perceived to be needed ASAP, those can be added to the StreamEventSourceProps in this CR as well. I checked that CloudFormation does support all of these options. However, only passing the enabled flag helps close the feature request #5750 and limits the scope of the CR.

This is fine. We have another PR that addresses the other properties coming in - #5929

nija-at
nija-at suggested changes Mar 9, 2020
View reviewed changes
packages/@aws-cdk/aws-lambda-event-sources/README.md
@@ -137,6 +138,7 @@ and add it to your Lambda function. The following parameters will impact Amazon

* __batchSize__: Determines how many records are buffered before invoking your lambda function - could impact your function's memory usage (if too high) and ability to keep up with incoming data velocity (if too low).
* __startingPosition__: Will determine where to being consumption, either at the most recent ('LATEST') record or the oldest record ('TRIM_HORIZON'). 'TRIM_HORIZON' will ensure you process all available data, while 'LATEST' will ignore all records that arrived prior to attaching the event source.
* __enabled__: Disables or enables the event source mapping to pause/start polling and invocation.
Copy link
Contributor

@nija-at nija-at Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this not applicable to other event sources, such as S3 and SNS?

The documentation here seems to suggest that it's applicable to all event source types, no?

packages/@aws-cdk/aws-lambda-event-sources/test/integ.dynamodb.ts
@@ -20,7 +20,8 @@ class DynamoEventSourceTest extends cdk.Stack {

fn.addEventSource(new DynamoEventSource(queue, {
batchSize: 5,
startingPosition: lambda.StartingPosition.TRIM_HORIZON
startingPosition: lambda.StartingPosition.TRIM_HORIZON,
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid changing an existing integration test.

Actually, an integration test is not needed for this feature; unit tests are sufficient.

@mergify
Copy link
Contributor

mergify bot commented Mar 25, 2020

Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review.

@Gtofig
Copy link
Contributor

Gtofig commented Apr 30, 2020

Is this still being worked on?

@nija-at nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 19, 2020
@github-actions
Copy link

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels May 20, 2020
@github-actions github-actions bot closed this May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nija-at nija-at nija-at requested changes

Assignees

@nija-at nija-at

Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lambda event sources can't be added as disabled when using add_event_source method
4 participants
@zeynepsu @aws-cdk-automation @nija-at @Gtofig