fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited#28793
Conversation
| ...this.renderRetryCatch(), | ||
| }; | ||
|
|
||
| // merge the Retry filed defined in the stateJson into the state |
There was a problem hiding this comment.
The Retry is configured in two ways(the addRetry method and defining it in the stateJson), so we need to merge them.
|
Nicely done, fix looks good to me. Thanks @sakurai-ryo for updating the documentation for clarification too. |
|
|
||
| The Retry and Catch fields are available for error handling. | ||
| You can configure the Retry field by defining it in the JSON object or by adding it using the `addRetry` method. | ||
| However, the Catch field cannot be configured by defining it in the JSON object, so it must be added using the `addCatch` method. |
There was a problem hiding this comment.
why not to do the same for the Catch field?
There was a problem hiding this comment.
@moelasmar
Thanks for clarifying.
As mentioned in the comments (#25798 (comment)), the method of defining the Catch directly in JSON for the CustomState did not render well and forced us to call _addCatch in the base State class.
The addCatch method was added to solve this in this PR (#28422), but now it overrides the Catch and Retry fields in the JSON.
So it will take some investigation and changes to be able to define the Catch directly in JSON.
…respect-state-json-catch-and-retry
|
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). |
|
@mergify update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
https://github.com/Mergifyio update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
Thanks @moelasmar @GavinZZ |
|
@Mergifyio update |
1 similar comment
|
@Mergifyio update |
for some reason mergify is not working, could you please do the update locally, and update your branch |
…respect-state-json-catch-and-retry
Pull request has been modified.
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). |
…is always overwrited (#28793) After #28422 was merged, the regression that overwrites the Retry field defined in the stateJson was introduced. The `this.renderRetryCatch()` method overwrites the Retry field in the stateJson. https://github.com/aws/aws-cdk/blob/45b8398bec9ba9c03f195c14f3b92188c9058a7b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts#L74 This PR fixes this regression and clarifies the current behavior for configuring the Retry and Catch field. Previously, I added the `addRetry` method to add the Retry field and did not render the Retry field in the stateJson in #28598, but this is initially a regression and should have been fixed. Closes #28769 Relates #28586 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…is always overwrited (#28793) After #28422 was merged, the regression that overwrites the Retry field defined in the stateJson was introduced. The `this.renderRetryCatch()` method overwrites the Retry field in the stateJson. https://github.com/aws/aws-cdk/blob/45b8398bec9ba9c03f195c14f3b92188c9058a7b/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts#L74 This PR fixes this regression and clarifies the current behavior for configuring the Retry and Catch field. Previously, I added the `addRetry` method to add the Retry field and did not render the Retry field in the stateJson in #28598, but this is initially a regression and should have been fixed. Closes #28769 Relates #28586 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
After #28422 was merged, the regression that overwrites the Retry field defined in the stateJson was introduced.
The
this.renderRetryCatch()method overwrites the Retry field in the stateJson.aws-cdk/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts
Line 74 in 45b8398
This PR fixes this regression and clarifies the current behavior for configuring the Retry and Catch field.
Previously, I added the
addRetrymethod to add the Retry field and did not render the Retry field in the stateJson in #28598, but this is initially a regression and should have been fixed.Closes #28769
Relates #28586
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license