feat(lambda): sns topic dlq #4763

nmussy · 2019-10-30T14:26:57Z

create a new dlq property, with DeadLetterQueue helper class
add DeadLetterQueue.fromSnsTopic feature
deprecate deadLetterQueue and deadLetterQueueEnabled props

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

mergify · 2019-10-30T14:27:37Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

nmussy · 2019-10-30T14:28:10Z

yarn.lock

@@ -1387,6 +1387,11 @@ acorn-globals@^4.1.0:
    acorn "^6.0.1"
    acorn-walk "^6.0.1"

+acorn-jsx@^5.1.0:


Not sure where the huge diff in the yarn.lock is coming from. I can revert it if necessary

nmussy · 2019-10-30T14:30:44Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+   *
+   * @default - no dead-letter queue will be used or created
+   */
+  readonly dlq?: DeadLetterQueue;


I'm not a fan of dlq as a property name. The "queue" part isn't even true anymore. How about deadLetter? Same for the class, DeadLetter?

Use deadLetterTarget and DeadLetterTarget; consistent with the name Lambda has given it - https://docs.aws.amazon.com/lambda/latest/dg/API_DeadLetterConfig.html

aws-cdk-automation · 2019-10-30T14:35:22Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-30T14:48:29Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-30T14:59:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-30T15:15:07Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

nmussy · 2019-10-30T15:17:52Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+/**
+ * Describes the type of resource to use as a Lambda dead-letter queue
+ */
+enum DeadLetterQueueType {


Not sure how to please JSII on this one. I've tried:

export with @internal

export

no export

None of which build successfully

@eladb Any idea what to do about that?

We ship enums in the CDK - https://github.com/aws/aws-cdk/search?l=TypeScript&q=enum. What's the problem?

eladb · 2019-10-30T17:23:13Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+   */
+  // tslint:disable-next-line:ban-types
+  public bind(fn: Function) {
+    switch (this.type) {


Instead of this, just make bind abstract and implement it in-place for each fromXxx method:

export abstract class DeadLetterQueue { public static fromSqsQueue(queue?: sqs.IQueue): DeadLetterQueue { return { bind: fn => { /* code for SQS queue */ } }; } public static fromSnsTopic(topic?: sns.ITopic): DeadLetterQueue { return { bind: fn => { /* code for SNS */ } } public abstract bind(fn: lambda.Function): void; }

eladb · 2019-11-07T09:06:54Z

packages/@aws-cdk/aws-lambda/README.md

@@ -110,7 +115,7 @@ const fn = new lambda.Function(this, 'MyFunction', {
    runtime: lambda.Runtime.NODEJS_8_10,
    handler: 'index.handler',
    code: lambda.Code.fromInline('exports.handler = function(event, ctx, cb) { return cb(null, "hi"); }'),
-    deadLetterQueueEnabled: true
+    dlq: lambda.DeadLetterQueue.fromSqsQueue(),


I think that in this case the from prefix doesn't feel right because we allow no parameters and so "from what?". I'd argue that lambda.DeadLetterQueue.sqsQueue() reads better in this case and cal also accomnodate DeadLetterQueue.sqsQueue(x) (@rix0rrr, thoughts?).

Alternatively we can do: fromNewQueue() and fromQueue(x).

nija-at

Refer to comments from Elad and the failing build.

mergify · 2019-12-16T12:34:19Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:01:25Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:32Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:39:18Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:53:00Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:18:19Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

nija-at · 2020-01-02T10:10:30Z

Closing this for now, since there's no activity on here for 2 months.

Feel free to re-open and pick this back up when ready.

feat(lambda): sns topic dlq

aac63c5

nmussy requested review from eladb and nija-at as code owners October 30, 2019 14:26

nmussy commented Oct 30, 2019

View reviewed changes

chore: remove internal tags

7ff297f

chore: ignore tslint

c6ce081

chore: remove internal interface export

9777037

nmussy commented Oct 30, 2019

View reviewed changes

SomayaB assigned nija-at Oct 30, 2019

eladb reviewed Nov 7, 2019

View reviewed changes

nija-at suggested changes Nov 12, 2019

View reviewed changes

SomayaB mentioned this pull request Nov 15, 2019

[lambda] support for dead letter targets #2739

Closed

nija-at closed this Jan 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): sns topic dlq #4763

feat(lambda): sns topic dlq #4763

nmussy commented Oct 30, 2019 •

edited

Loading

mergify bot commented Oct 30, 2019

nmussy Oct 30, 2019

nmussy Oct 30, 2019

nija-at Nov 12, 2019

aws-cdk-automation commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

nmussy Oct 30, 2019

nmussy Nov 7, 2019

nija-at Nov 12, 2019

eladb Oct 30, 2019

eladb Nov 7, 2019

nija-at left a comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

nija-at commented Jan 2, 2020

feat(lambda): sns topic dlq #4763

feat(lambda): sns topic dlq #4763

Conversation

nmussy commented Oct 30, 2019 • edited Loading

mergify bot commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

nija-at commented Jan 2, 2020

nmussy commented Oct 30, 2019 •

edited

Loading