feat(stepfunctions): add intrinsic functions #22431
Conversation
|
|
github-actions
bot
added
effort/small
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
| export function renderInExpression(x: string) { | ||
| const path = jsonPathString(x); | ||
| return path ?? singleQuotestring(x); | ||
| export function renderInExpression(x: any) { |
There was a problem hiding this comment.
Rather than throwing an error here for other types and accepting any, x should be typed to the specific valid types:
| export function renderInExpression(x: any) { | |
| export function renderInExpression(x: string | number) { |
There was a problem hiding this comment.
renderInExpression can receive string[] here, so it should at least accept string[]. (Note that listAt returns string[]. )
We could restrict the input type by creating another function like renderListInExpression, but I think it is not necessary because the function is a "private" function.
https://github.com/gkkachi/aws-cdk/blob/ab29124d26cf8a21a1399b0193122c7b8419d00d/packages/%40aws-cdk/aws-stepfunctions/lib/fields.ts#L113-L115
https://github.com/gkkachi/aws-cdk/blob/ab29124d26cf8a21a1399b0193122c7b8419d00d/packages/%40aws-cdk/aws-stepfunctions/test/fields.test.ts#L241
There was a problem hiding this comment.
If it can accept string[], the logic isn't quite right, then. You throw an error if it's not string or number.
There was a problem hiding this comment.
Also, please write tests against the various input types you'd expect here and at least one failure case.
There was a problem hiding this comment.
I wrote tests for renderInExpression . d32fbf3
| /* | ||
| * Stack verification steps: | ||
| * | ||
| * -- aws stepfunctions describe-state-machine --state-machine-arn <stack-output> has a status of `ACTIVE` |
There was a problem hiding this comment.
This should use an assertion in code to verify instead of being added here in text.
There was a problem hiding this comment.
I created this integration test based on this.
I think it is not necessary to execute describe-state-machine explicitly to confirm that the state machine is deployed successfully because successful deployment of cfn stack means that state machine deployments are also succeeded, so I deleted the comment by 926c938.
There was a problem hiding this comment.
Please add the assertion anyway. We didn't have that library when the older test was written but we do have that capability now.
There was a problem hiding this comment.
Ok, I added it. d2ff6e1
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
github-actions
bot
added
effort/medium
TheRealAmazonKendra
changed the title
|
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). |
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
|
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). |
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). |
| if (path) return path; | ||
| if (typeof x === 'number') return x.toString(10); | ||
| if (typeof x === 'string') return singleQuotestring(x); | ||
| throw new Error('Unxexpected value.'); |
Resolves #22068 and resolves #22629
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license