fix(trigger): Accept 2xx-3xx response codes/use string property for timeout #23650

DerkSchooltink · 2023-01-11T15:52:35Z

Fixes #23407

Expand success check to incorporate all 2xx-3xx response codes
Use string properties on custom resource instead of numbers

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2023-01-11T15:52:46Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

perpil · 2023-01-14T00:31:14Z

packages/@aws-cdk/triggers/lib/lambda/index.ts

-  const invokeResponse = await invoke(handlerArn, invocationType, timeout);
+  const parsedTimeout = parseInt(timeout);
+  if (isNaN(parsedTimeout)) {
+    throw new Error(`The "Timeout" property with value ${timeout} is not a parseable to a number`);


is not a parseable -> is not parseable

Woops, will fix in next commit.

TheRealAmazonKendra

Typically we ask for one change per pr, but since this is fixing a set of bugs introduced in a prior PR, I think that it's probably fine in this case. Can you help me understand, however, why the integ tests succeeded on the prior PR and didn't catch these bugs? Was the output manually changed instead of the tests being run?

TheRealAmazonKendra · 2023-01-18T01:29:14Z

yarn.lock

@@ -2771,6 +2771,38 @@ aws-sdk@^2.1211.0, aws-sdk@^2.596.0, aws-sdk@^2.928.0:
    uuid "8.0.0"
    xml2js "0.4.19"

+aws-sdk@^2.1282.0:


Unless there is a specific feature in this version that you need, updates to yarn.lock shouldn't be committed.

TheRealAmazonKendra · 2023-01-18T01:29:48Z

packages/@aws-cdk/triggers/package.json

@@ -77,7 +77,7 @@
    "@aws-cdk/cdk-build-tools": "0.0.0",
    "@aws-cdk/integ-runner": "0.0.0",
    "@aws-cdk/aws-sns": "0.0.0",
-    "aws-sdk": "^2.1211.0",
+    "aws-sdk": "^2.1282.0",


Why the manual update here?

My bad, in hindsight this wasn't needed. I could not find the httpOptions property on the SDK clients, which did appear if I installed another version. But that is because the node_modules are in the root of the project instead of in the subproject for triggers only.

Reverted in next commit.

Pull request has been modified.

DerkSchooltink · 2023-01-19T15:05:29Z

Typically we ask for one change per pr, but since this is fixing a set of bugs introduced in a prior PR, I think that it's probably fine in this case. Can you help me understand, however, why the integ tests succeeded on the prior PR and didn't catch these bugs? Was the output manually changed instead of the tests being run?

Yes, that was sort of what happened. It was not entirely clear to me what the integration tests were expecting. I could not get them to run in this repo so I deployed it standalone and copied over the output.

aws-cdk-automation · 2023-01-23T00:09:02Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2. Upgrade aws-sdk to allow for httpOptions#timeout property

The `{@link }` annotation doesn't work and has never worked, yet has cropped up in a lot of places in our docstrings, and it will only get worse as people look around and copy/paste. Do a mass removal of the `{@link}` tags. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This adds the `--output` flag as an option when building docker containers. This fixes aws#20566. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…aws#23591) The `AwsCustomResource` reaches out to the internet to install the latest AWS SDK by default. This will make it fail if it is being bound to a VPC that doesn't have internet connectivity, or in regions/partitions that are not able to freely connect to `npmjs.com`. This was a poorly chosen default from the time we didn't know any better, but we do know right now. Switch the behavior off by default (under feature flag), and explicitly disable it for all `AwsCustomResource`s the L2 library uses. Lambda advertises 2.1055.0 of the SDK everywhere, and I checked to make sure that all APIs we use are part of that SDK version, so we don't need any newer version. That version is a year old (!) so this is not the end of the story, but it's at least an improvement over what we currently have. Fixes aws#23113. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

2. Upgrade aws-sdk to allow for httpOptions#timeout property

aws#22943) fixes aws#22942 Description: AWS Launched the support for Amazon OpenSearch Service 2.3 as per the announcement here: https://aws.amazon.com/about-aws/whats-new/2022/11/amazon-opensearch-service-supports-opensearch-version-2-3/ AWS CDK does not support the latest version yet, and can not create an OpenSearch cluster with CDK using 2.3. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

DerkSchooltink · 2023-01-23T09:26:09Z

Will reopen a new PR to include the re-revert

aws-cdk-automation · 2023-01-23T10:04:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ca26af7
Result: SUCCEEDED
Build Logs (available for 30 days)

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

@TheRealAmazonKendra

…timeouts (#24435) Reopened PR from #23650 and #23788 And re-revert from #23062 ([v2.61.1](https://github.com/aws/aws-cdk/releases/tag/v2.61.1)) Implements #23058 and fixes #23407 Worked on comments from @TheRealAmazonKendra as well. Added an integration test scenario where a Lambda function will be triggered by a Trigger construct, send a message to an SQS queue, which will be consumed by the integration test assertion.

aws-cdk-automation requested a review from a team January 11, 2023 15:52

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Jan 11, 2023

aws-cdk-automation previously requested changes Jan 11, 2023

View reviewed changes

DerkSchooltink force-pushed the bugfix/triggerfunctioninvokefailure branch 3 times, most recently from 3422698 to b8e8dd7 Compare January 12, 2023 10:04

DerkSchooltink force-pushed the bugfix/triggerfunctioninvokefailure branch from 1167855 to ec90471 Compare January 12, 2023 11:35

perpil reviewed Jan 14, 2023

View reviewed changes

TheRealAmazonKendra previously requested changes Jan 18, 2023

View reviewed changes

DerkSch and others added 9 commits January 23, 2023 10:06

1. Expand success check to incorporate all 2xx-3xx response codes

eeea191

2. Upgrade aws-sdk to allow for httpOptions#timeout property

1. Expand success check to incorporate all 2xx-3xx response codes

3aa6fd9

2. Upgrade aws-sdk to allow for httpOptions#timeout property

Fix typo in error thrown on isNaN

0b2ffc7

Revert aws-sdk version increment

04a5b11

Restore readme from before revert

ca26af7

DerkSchooltink force-pushed the bugfix/triggerfunctioninvokefailure branch from 322bfdd to ca26af7 Compare January 23, 2023 09:24

DerkSchooltink closed this Jan 23, 2023

DerkSchooltink mentioned this pull request Jan 23, 2023

feat(trigger): Allow trigger to work with Lambda functions with long timeouts #23788

Closed

4 tasks

DerkSchooltink mentioned this pull request Mar 3, 2023

feat(trigger): Allow trigger to work with Lambda functions with long timeouts #24435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(trigger): Accept 2xx-3xx response codes/use string property for timeout #23650

fix(trigger): Accept 2xx-3xx response codes/use string property for timeout #23650

DerkSchooltink commented Jan 11, 2023

gitpod-io bot commented Jan 11, 2023

aws-cdk-automation left a comment

perpil Jan 14, 2023

DerkSchooltink Jan 16, 2023

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jan 18, 2023

DerkSchooltink Jan 19, 2023

TheRealAmazonKendra Jan 18, 2023

DerkSchooltink Jan 19, 2023

DerkSchooltink commented Jan 19, 2023

aws-cdk-automation commented Jan 23, 2023

DerkSchooltink commented Jan 23, 2023

aws-cdk-automation commented Jan 23, 2023

fix(trigger): Accept 2xx-3xx response codes/use string property for timeout #23650

fix(trigger): Accept 2xx-3xx response codes/use string property for timeout #23650

Conversation

DerkSchooltink commented Jan 11, 2023

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 11, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

perpil Jan 14, 2023

Choose a reason for hiding this comment

DerkSchooltink Jan 16, 2023

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jan 18, 2023

Choose a reason for hiding this comment

DerkSchooltink Jan 19, 2023

Choose a reason for hiding this comment

TheRealAmazonKendra Jan 18, 2023

Choose a reason for hiding this comment

DerkSchooltink Jan 19, 2023

Choose a reason for hiding this comment

DerkSchooltink commented Jan 19, 2023

aws-cdk-automation commented Jan 23, 2023

DerkSchooltink commented Jan 23, 2023

aws-cdk-automation commented Jan 23, 2023

AWS CodeBuild CI Report