-
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(stepfunctions-tasks): CallAwsServiceCrossRegion
doesn't work with WAIT_FOR_TASK_TOKEN
#32807
Conversation
… WAIT_FOR_TASK_TOKEN
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.
CallAwsServiceCrossRegion
doesn't work with WAIT_FOR_TASK_TOKEN
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32807 +/- ##
=======================================
Coverage 80.92% 80.92%
=======================================
Files 236 236
Lines 14253 14253
Branches 2490 2490
=======================================
Hits 11534 11534
Misses 2434 2434
Partials 285 285
Flags with carried forward coverage won't be shown. Click here to find out more.
|
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/lambda/call-aws-service-cross-region.ts
Outdated
Show resolved
Hide resolved
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.
See comment on default value set for payloadResponseOnly
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
Pull request has been modified.
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.
Please see the proposed change.
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/lambda/call-aws-service-cross-region.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
@@ -265,6 +265,8 @@ const getObject = new tasks.CallAwsServiceCrossRegion(this, 'GetObject', { | |||
|
|||
Other properties such as `additionalIamStatements` can be used in the same way as the `CallAwsService` task. | |||
|
|||
Note that when you use `integrationPattern.WAIT_FOR_TASK_TOKEN`, the output path changes under `Payload` property. |
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.
Nice. Thanks for adding.
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). |
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). |
@Mergifyio dequeue |
This pull request has been removed from the queue for the following reason: Pull request #32807 has been dequeued by a You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
✅ The pull request has been removed from the queue
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
This pull request has been removed from the queue for the following reason: The pull request #32807 has been manually updated You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
Pull request has been modified.
@Mergifyio requeue |
❌ This pull request head commit has not been previously disembarked from queue. |
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). |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #32746
Reason for this change
The Step Functions task
CallAwsServiceCrossRegion
did not work withintegrationPattern.WAIT_FOR_TASK_TOKEN
. This PR fixes the problem.Description of changes
The root cause of this problem is that we used an alterntive form of calling Lambda Invoke, which does not support waitForTaskToken integration pattern.
https://docs.aws.amazon.com/step-functions/latest/dg/connect-lambda.html#connect-lambda-api-examples
This PR fixes the code to align with the implementation in
LambdaInvoke
task, which supports both integration pattern.aws-cdk/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/lambda/invoke.ts
Lines 146 to 165 in bbdd42c
To fix this without breaking changes, we introduce
payloadResponseOnly
property (true by default), and fix the_renderTask
method as above. Users who need other integration patterns can setpayloadResponseOnly: false
.Additionally we throw an error if
payloadResponseOnly == true && integrationPattern != REQUEST_RESPONSE
to prevent misconfiguration. This is not a breaking change because the construct previously did not work at all whenintegrationPattern != REQUEST_RESPONSE
.Describe any new or updated permissions being added
No.
Description of how you validated changes
Added unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license