feat(ecs-patterns) expose ALB idleTimeout property by michaeldrey · Pull Request #21221 · aws/aws-cdk

michaeldrey · 2022-07-19T03:10:47Z

This PR exposes the idleTime property for both EC2 and Fargate, and the MultiTargetGroup ALB services. Per the CFN specs, I have set the idleTimeout to default to 60 seconds, and it cannot exceed 4000 seconds.

If no value is provided we set the value to undefined. We do this because:

The default value is set for the user via the CFN. The behavior for the property is to either add a value or leave it undefined and let CFN set the default. See here
Setting a default causes snapshot tests to fail for dependent services as we're adding a new attribute to the cloudformation.

In addition to this, I've updated all the Integ tests to a working state (would not build otherwise) and added working tests for all cases listed above.

Lastly I've added two examples to the README.

closes #12913

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional 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 · 2022-07-19T03:10:54Z

… into specify_idle_timeout

aws-cdk-automation · 2022-07-19T05:19:27Z

AWS CodeBuild CI Report

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

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

TheRealAmazonKendra

Thank you for your contribution. This looks like really excellent work (and includes everything I ever ask for in terms of tests and documentation updates). 120 files, however, makes it hard to provide a meaningful and thorough review. Could you please split this up into multiple PRs so that we can review them more effectively? For instance, I see that some test changes are just a correction of a typo. Great catch, but those could be their own PR to reduce the noise.

Also to cut down the number of files, I noticed that some files just include spacing changes. I don't think most of those are necessary, but the ones that are could go in the same PR as the typo fixes. I think I would generally split this into three:

The typo/formatting fixes.
The change to application-load-balanced-service-base.ts and corresponding tests.
The change to application-multiple-target-groups-service-base.ts and corresponding tests.

michaeldrey · 2022-07-19T06:23:15Z

@TheRealAmazonKendra Thanks for the quick response!

You have a great point, this has kind of snowballed a bit. I will begin splitting this out into the PR's you've suggested

TheRealAmazonKendra · 2022-07-19T17:40:01Z

@TheRealAmazonKendra Thanks for the quick response!

You have a great point, this has kind of snowballed a bit. I will begin splitting this out into the PR's you've suggested

Thank you! I know it can be kind of a pain in the ass to split up a PR once you've got it all prepared. I appreciate your efforts here.

…nstruct (#21237) Currently when you run `yarn build+test+extract`, integration tests all fail with `No stack found for test x`. This PR updates all the integration tests to the new standard using `integ-runner` + `integ-tests` libraries. Integration tests tend to generate a lot of files for the snapshot, hence the 111 files in the PR. This PR is breaking up my previous PR, #21221 ---- ### All Submissions: * [ ] 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 * [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*

A few integration tests slipped under the radar and were not updated with the new integ-tests library. Also corrected typos. Relates to: #21237 #21221 ---- ### 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 * [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*

…nstruct (aws#21237) Currently when you run `yarn build+test+extract`, integration tests all fail with `No stack found for test x`. This PR updates all the integration tests to the new standard using `integ-runner` + `integ-tests` libraries. Integration tests tend to generate a lot of files for the snapshot, hence the 111 files in the PR. This PR is breaking up my previous PR, aws#21221 ---- ### All Submissions: * [ ] 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 * [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*

A few integration tests slipped under the radar and were not updated with the new integ-tests library. Also corrected typos. Relates to: aws#21237 aws#21221 ---- ### 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 * [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*

TheRealAmazonKendra · 2022-07-29T17:41:13Z

@michaeldrey Do you plan to make any updates to this PR or just the other two that came out of this? If the latter, let's close this one so we don't have extra PRs open that we're not going to review.

michaeldrey · 2022-07-29T17:48:30Z

@TheRealAmazonKendra I'll close this one as the other two capture all the features in this PR.

…21261) This PR exposes the idleTime property for both EC2 and Fargate ALB services. [Per the CFN specs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-loadbalancer-loadbalancerattributes.html), I have set the idleTimeout to default to 60 seconds, and it cannot exceed 4000 seconds. If no value is provided we set the value to undefined. We do this because: 1. The default value is set for the user via the CFN. The behavior for the property is to either add a value or leave it undefined and let CFN set the default.[ See here](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts#L102) 2. Setting a default causes snapshot tests to fail for dependent services as we're adding a new attribute to the cloudformation. A README entry has been created for this property. Relates to #21221 #21266 closes #12913 ---- ### 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 * [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*

…ices (#21266) This PR exposes the idleTime property for both EC2 and Fargate **multi** ALB services. [Per the CFN specs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-loadbalancer-loadbalancerattributes.html), I have set the idleTimeout to default to 60 seconds, and it cannot exceed 4000 seconds. If no value is provided we set the value to undefined. We do this because: 1. The default value is set for the user via the CFN. The behavior for the property is to either add a value or leave it undefined and let CFN set the default.[ See here](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts#L102) 2. Setting a default causes snapshot tests to fail for dependent services as we're adding a new attribute to the cloudformation. A README entry has been created for this property. Relates to #21221 #21261 closes #12913 ---- ### 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 * [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*

Mike Dreyfus added 4 commits July 18, 2022 16:35

adding idletime property; updated tests

08a0017

add section to README

4fb9b29

added idleTimeout to multiTargetGroup service

1c97fac

added another example in the readme

5e1b304

aws-cdk-automation requested a review from a team July 19, 2022 03:11

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 19, 2022

michaeldrey changed the title ~~feat(ecs-patterns) expose idleTimeout property~~ feat(ecs-patterns) expose ALB idleTimeout property Jul 19, 2022

michaeldrey and others added 4 commits July 18, 2022 20:19

Merge branch 'main' into specify_idle_timeout

a185a3f

removed default as it is set in cfn

15a647c

Merge branch 'specify_idle_timeout' of github.com:michaeldrey/aws-cdk…

182712d

… into specify_idle_timeout

fixing tests

4485598

TheRealAmazonKendra requested changes Jul 19, 2022

View reviewed changes

michaeldrey mentioned this pull request Jul 19, 2022

chore(ecs-patterns) update integration tests to use new integ-test construct #21237

Merged

4 tasks

This was referenced Jul 21, 2022

chore(ecs-patterns) updating integration tests #21260

Merged

feat(ecs-patterns) add idle timeout to alb fargate and ec2 services #21261

Merged

michaeldrey mentioned this pull request Jul 21, 2022

feat(ecs-patterns) add idle timeout to multi alb fargate and ec2 services #21266

Merged

4 tasks

michaeldrey closed this Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs-patterns) expose ALB idleTimeout property#21221

feat(ecs-patterns) expose ALB idleTimeout property#21221
michaeldrey wants to merge 8 commits intoaws:mainfrom
michaeldrey:specify_idle_timeout

michaeldrey commented Jul 19, 2022 •

edited

Loading

Uh oh!

gitpod-io bot commented Jul 19, 2022

Uh oh!

aws-cdk-automation commented Jul 19, 2022

Uh oh!

TheRealAmazonKendra left a comment •

edited

Loading

Uh oh!

michaeldrey commented Jul 19, 2022

Uh oh!

TheRealAmazonKendra commented Jul 19, 2022

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

michaeldrey commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

michaeldrey commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

closes #12913

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 19, 2022

Uh oh!

aws-cdk-automation commented Jul 19, 2022

AWS CodeBuild CI Report

Uh oh!

TheRealAmazonKendra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeldrey commented Jul 19, 2022

Uh oh!

TheRealAmazonKendra commented Jul 19, 2022

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

michaeldrey commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaeldrey commented Jul 19, 2022 •

edited

Loading

TheRealAmazonKendra left a comment •

edited

Loading