-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(custom-resources): correctly convert values to Date type #28398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -332,6 +338,10 @@ function isNumber(shape: SmithyShape): boolean { | |||
isShape('byte')(shape); | |||
} | |||
|
|||
function isDate(shape: SmithyShape): boolean { | |||
return isShape('timestamp')(shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return isShape('timestamp')(shape) | |
return isShape('timestamp')(shape); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be coming from running the update-sdkv3-parameters-model.sh
script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as these updates...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Date
type as a parameter, such asGetMetricData
API, from a Custom Resource Lambda function, where the parameter is passed asstring
type 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
string
type must be properly converted toDate
type 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
number
andUint8Array
types.Uint8Array
: #27034number
: #27112Major changes
update-sdkv3-parameters-model.ts
scriptIf the type is
timestamp
in thesmithy
specification, writed
to 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.sh
script 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-adapter
moduleI added code to convert value marked
d
in state machine toDate
type.If the conversion to
Date
type fails, theDate
class 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-alpha
moduleAdded integ-test to verify that errors reported in the related issue have been resolved.
The IAM Policy added internally by the call to
adPolicyStatementFromSdkCall
looks like the following and does not callGetMetricData
correctly, so theaddToRolePolicy
method was used to explicitly add a new Policy is added explicitly with theaddToRolePolicy
method.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