fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout by michelle-wangg · Pull Request #31446 · aws/aws-cdk

michelle-wangg · 2024-09-13T22:11:44Z

Issue # (if applicable)

Reason for this change

When specifiying both resourceSignalTimeout and initOptions.timeout in the options for creating an EC2 Instance, only the value from resourceSignalTimeout is used.

Description of changes

If both initOptions.timeout and resourceSignalTimeout are set, timeout consisting of the sum of the values and a warning suggesting that only one field should be specified
Else, timeout is set to field specified, else defaults to 5 min

Description of how you validated changes

Update integration test for instance-init.js with both fields and verify that warning is logged
Add unit tests:
- initOptions.timeout and resourceSignalTimeout are both not set, timeout is set to default of 5 min
- initOptions.timeout set and resourceSignalTimeout not set, timeout is set to initOptions.timeout
- initOptions.timeout not set and resourceSignalTimeout is set, timeout is set to resourceSignalTimeout
- initOptions.timeout and resourceSignalTimeout are both set, timeout is set to sum of timeouts and warning is logged

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

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

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

GavinZZ

Nice Michelle! Left some comments.

I see your fix would now be summing the two values up. This would mean that users (who haven't changed their template) would have the values updated in their next deployment if they upgrade their CDK version. This is generally a sign of breaking change, however, in this case it's extending the timeout value. I think this can be argued to be a non-breaking change. Can you include @moelasmar in the review and get his though?

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

...g/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/integ-init.template.json

moelasmar

left minor comments, and you need to rerun the integration test cases with --update-on-failed flag

...e-init.js.snapshot/EnableSumTimeoutInstanceTestDefaultTestDeployAssert5CD5B9B5.template.json

...nce-init.js.snapshot/EnableSumTimeoutInstanceTestDefaultTestDeployAssert5CD5B9B5.assets.json

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

aws-cdk-automation · 2024-09-23T20:40:52Z

AWS CodeBuild CI Report

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

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

GavinZZ · 2024-09-23T23:40:34Z

@mergify update

mergify · 2024-09-23T23:40:38Z

update

☑️ Nothing to do

Details

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

Dismissing Mohamed's review since it's all addressed.

mergify · 2024-09-24T00:13:48Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-09-24T00:14:09Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team September 13, 2024 22:11

github-actions bot added the p2 label Sep 13, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 13, 2024

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort labels Sep 13, 2024

aws-cdk-automation previously requested changes Sep 13, 2024

View reviewed changes

michelle-wangg changed the title ~~fix(ec2): Instance resourceSignalTimeout overwrites initOptions.timeout~~ fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout Sep 16, 2024

GavinZZ reviewed Sep 16, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

michelle-wangg marked this pull request as ready for review September 16, 2024 21:03

michelle-wangg requested a review from GavinZZ September 16, 2024 21:06

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 16, 2024

GavinZZ suggested changes Sep 16, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 16, 2024

michelle-wangg requested review from GavinZZ and moelasmar September 16, 2024 22:24

kaizencc reviewed Sep 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

michelle-wangg requested a review from kaizencc September 17, 2024 22:48

moelasmar reviewed Sep 18, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

michelle-wangg force-pushed the overwriting-timeout branch 2 times, most recently from 563f7fe to 40dbdd2 Compare September 19, 2024 22:03

michelle-wangg requested a review from moelasmar September 19, 2024 23:08

moelasmar reviewed Sep 20, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

moelasmar reviewed Sep 20, 2024

View reviewed changes

...g/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/integ-init.template.json Show resolved Hide resolved

moelasmar previously requested changes Sep 20, 2024

View reviewed changes

michelle-wangg force-pushed the overwriting-timeout branch from 29e4ffc to 2ae6eb7 Compare September 20, 2024 17:52

michelle-wangg requested a review from moelasmar September 20, 2024 22:33

GavinZZ reviewed Sep 21, 2024

View reviewed changes

Michelle Wang added 2 commits September 22, 2024 07:43

fix(ec2): Instance resourceSignalTimeout overwrites initOptions.timeout

6ed7ede

update integration test and snapshots

0565d3c

Michelle Wang added 15 commits September 22, 2024 07:43

fix build

ccf3e1d

update

b58476b

fixes

d053085

move warning and update docs

43ffc80

update

094c980

update to use feature flag

189954c

feature flags

b1e1229

add integration test

4a08aa9

integration test

6d6ef2b

update comments

4f3502f

update feature flag naming

ad6c5a5

modify integration test

db72209

update integ tests and add count

505c96f

rerun integration test

84341cf

rerun integ

1d0dd62

michelle-wangg force-pushed the overwriting-timeout branch from 7cd9f6a to 1d0dd62 Compare September 22, 2024 14:58

michelle-wangg requested a review from GavinZZ September 23, 2024 17:13

GavinZZ reviewed Sep 23, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Show resolved Hide resolved

Michelle Wang and others added 2 commits September 23, 2024 13:10

add comment

7c29799

Merge branch 'main' into overwriting-timeout

dab5af4

michelle-wangg requested a review from GavinZZ September 23, 2024 20:11

GavinZZ approved these changes Sep 23, 2024

View reviewed changes

mergify bot merged commit a29bf19 into aws:main Sep 24, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2024

Conversation

michelle-wangg commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GavinZZ 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moelasmar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation commented Sep 23, 2024

AWS CodeBuild CI Report

Uh oh!

GavinZZ commented Sep 23, 2024

Uh oh!

mergify bot commented Sep 23, 2024

☑️ Nothing to do

Uh oh!

mergify bot commented Sep 24, 2024

Uh oh!

github-actions bot commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michelle-wangg commented Sep 13, 2024 •

edited

Loading

GavinZZ left a comment •

edited

Loading