fix(custom-resources): correctly convert values to Date type#28398
fix(custom-resources): correctly convert values to Date type#28398mergify[bot] merged 30 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
|
Exemption Request: Since this PR is an internal behavior fix, I don't think it is necessary to change the README. |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
kaizencc
left a comment
There was a problem hiding this comment.
Thank you for the very detailed PR @sakurai-ryo! This looks mostly reasonable to me, but because I'm not too familiar with this part of our repo I'm going to toss it over to @mrgrain or @MrArnoldPalmer for a second look.
| } | ||
|
|
||
| function isDate(shape: SmithyShape): boolean { | ||
| return isShape('timestamp')(shape) |
There was a problem hiding this comment.
| return isShape('timestamp')(shape) | |
| return isShape('timestamp')(shape); |
There was a problem hiding this comment.
Oops, I fixed it.
There was a problem hiding this comment.
Mostly because I'm not too familiar with this section of the repo, can you explain how you arrived at these updates?
There was a problem hiding this comment.
Should be coming from running the update-sdkv3-parameters-model.sh script.
There was a problem hiding this comment.
As well as these updates...
There was a problem hiding this comment.
Should be coming from running the update-sdkv3-parameters-model.sh script.
|
@sakurai-ryo There are some merge conflicts that need to be addressed. The changes look good to me, @kaizencc @mrgrain are there any other questions or changes to be made? |
…fix-integ-tests-date-type
…fix-integ-tests-date-type
|
Thanks @paulhcsun! Fixed conflicts. |
…fix-integ-tests-date-type
|
hey @sakurai-ryo, Apologies for not getting around to this. It doesn't seem like there are any other major concerns and I'm good to merge this in. It seems like the integ snapshots are outdated. Could you rerun with latest changes to update those? I'll keep an eye on this and approve when the build is happy. |
…fix-integ-tests-date-type
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
|
Hi @paulhcsun |
paulhcsun
left a comment
There was a problem hiding this comment.
Thanks for this fix @sakurai-ryo! And thanks again for your patience and understanding.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Description
The following issue reports an error that occurs when calling an API that takes the
Datetype as a parameter, such asGetMetricDataAPI, from a Custom Resource Lambda function, where the parameter is passed asstringtype to the AWS SDK.#27962
https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/cloudwatch/command/GetMetricDataCommand/#:~:text=Description-,EndTime,-Required
To resolve this error, the
stringtype must be properly converted toDatetype when calling the AWS SDK from Lambda.In this PR, I added the conversion to Date type in the same way as the existing conversion to
numberandUint8Arraytypes.Uint8Array: #27034number: #27112Major changes
update-sdkv3-parameters-model.tsscriptIf the type is
timestampin thesmithyspecification, writedto the state machine so that it can be converted to a Date type later.https://smithy.io/2.0/spec/simple-types.html#timestamp
update-sdkv3-parameters-model.shscript was not called from anywhere, so I called it manually and updated the JSON file.Please let me know if there is a problem.
sdk-v2-to-v3-adaptermoduleI added code to convert value marked
din state machine toDatetype.If the conversion to
Datetype fails, theDateclass does not throw an exception, so the error is handled in a slightly tricky way.Also added a unit test for this process.
integ-tests-alphamoduleAdded integ-test to verify that errors reported in the related issue have been resolved.
The IAM Policy added internally by the call to
adPolicyStatementFromSdkCalllooks like the following and does not callGetMetricDatacorrectly, so theaddToRolePolicymethod was used to explicitly add a new Policy is added explicitly with theaddToRolePolicymethod.{ "Version": "2012-10-17", "Statement": [ { "Action": [ "monitoring:GetMetricData" ], "Resource": [ "*" ], "Effect": "Allow" } ] }aws-cdk/packages/aws-cdk-lib/custom-resources/lib/helpers-internal/sdk-v3-metadata.json
Line 174 in 1a9c30e
fixes #27962
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license