-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
(aws-ec2): Instance resourceSignalTimeout overwrites initOptions.timeout #30052
(aws-ec2): Instance resourceSignalTimeout overwrites initOptions.timeout #30052
Comments
Thanks @Xfel for pointing this and sharing the repro code. I can see the value is being overwritte.
Marking the issue as appropriate. |
I can take this. Do we want to choose the maximum of the two or throw an error if both are supplied? |
Comments on closed issues and PRs are hard for our team to see. |
1 similar comment
Comments on closed issues and PRs are hard for our team to see. |
…ut (#31446) ### Issue # (if applicable) Closes #30052 ### 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](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Describe the bug
When specifiying both
resourceSignalTimeout
andinitOptions.timeout
in the options for creating an EC2 Instance, only the value fromresourceSignalTimeout
is used.Expected Behavior
A timeout consisting of the sum of the values, or a clear error that specifying both fields is not supported.
Current Behavior
resourceSignalTimeout
overridesinitOptions.timeout
completely.Reproduction Steps
In a new CDK project:
The resulting template lists for the instance:
Possible Solution
In https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/instance.ts,
initOptions.timeout
is applied beforeresourceSignalTimeout
. The code to applyinitOptions.timeout
adds it to any preexisting timeout. Swapping the order of setup here should fix the issue.Additional Information/Context
No response
CDK CLI Version
2.136.0 (build 94fd33b)
Framework Version
No response
Node.js Version
v18.18.0
OS
Windows
Language
TypeScript
Language Version
5.0.4
Other information
No response
The text was updated successfully, but these errors were encountered: