fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited #28793

sakurai-ryo · 2024-01-20T14:47:34Z

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.renderRetryCatch(),

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

sakurai-ryo · 2024-01-20T14:56:32Z

packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts

      ...this.renderNextEnd(),
      ...this.stateJson,
      ...this.renderRetryCatch(),
    };
+
+    // merge the Retry filed defined in the stateJson into the state


The Retry is configured in two ways(the addRetry method and defining it in the stateJson), so we need to merge them.

jacklin213 · 2024-01-21T22:58:56Z

Nicely done, fix looks good to me. Thanks @sakurai-ryo for updating the documentation for clarification too.
Will wait for someone from aws-cdk team to sign off :)

moelasmar · 2024-01-23T20:19:47Z

packages/aws-cdk-lib/aws-stepfunctions/README.md

@@ -562,6 +562,10 @@ Custom states can be chained together with any of the other states to create you
 definition. You will also need to provide any permissions that are required to the `role` that
 the State Machine uses.

+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.


why not to do the same for the Catch field?

@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.

thanks @sakurai-ryo for the clarifying.

…respect-state-json-catch-and-retry

mergify · 2024-01-30T22:58:47Z

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 · 2024-01-30T23:02:35Z

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).

GavinZZ · 2024-01-31T00:00:39Z

@mergify update

mergify · 2024-01-31T00:00:43Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics.yml without workflows permission

moelasmar · 2024-01-31T01:19:12Z

https://github.com/Mergifyio update

mergify · 2024-01-31T01:19:17Z

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics.yml without workflows permission

sakurai-ryo · 2024-01-31T04:00:39Z

Thanks @moelasmar @GavinZZ
Do I need to do the update locally?

moelasmar · 2024-01-31T17:09:49Z

@Mergifyio update

moelasmar · 2024-01-31T22:03:17Z

@Mergifyio update

moelasmar · 2024-01-31T22:04:42Z

Thanks @moelasmar @GavinZZ Do I need to do the update locally?

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-cdk-automation · 2024-02-01T01:34:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 65885ad
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-02-01T01:34:29Z

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*

fix: regression

63cdb72

aws-cdk-automation requested a review from a team January 20, 2024 14:47

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Jan 20, 2024

sakurai-ryo commented Jan 20, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 20, 2024

sakurai-ryo changed the title ~~fix(stepfunctions): the Retry field in the statesJson is always overwrited~~ fix(stepfunctions): the Retry field in the statesJson of CustomState is always overwrited Jan 22, 2024

sakurai-ryo changed the title ~~fix(stepfunctions): the Retry field in the statesJson of CustomState is always overwrited~~ fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited Jan 22, 2024

moelasmar self-requested a review January 22, 2024 22:55

moelasmar reviewed Jan 23, 2024

View reviewed changes

Merge branch 'master' of https://github.com/sakurai-ryo/aws-cdk into …

d0012e8

…respect-state-json-catch-and-retry

moelasmar previously approved these changes Jan 30, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 30, 2024

Merge branch 'master' of https://github.com/sakurai-ryo/aws-cdk into …

65885ad

…respect-state-json-catch-and-retry

moelasmar approved these changes Feb 1, 2024

View reviewed changes

mergify bot merged commit 3c33e2c into aws:main Feb 1, 2024
11 checks passed

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

abdelnn mentioned this pull request Feb 26, 2024

aws-stepfunctions: state.Retry is not iterable #29274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited #28793

fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited #28793

sakurai-ryo commented Jan 20, 2024 •

edited

Loading

sakurai-ryo Jan 20, 2024

jacklin213 commented Jan 21, 2024

moelasmar Jan 23, 2024

sakurai-ryo Jan 24, 2024 •

edited

Loading

moelasmar Jan 30, 2024

mergify bot commented Jan 30, 2024

mergify bot commented Jan 30, 2024

GavinZZ commented Jan 31, 2024

mergify bot commented Jan 31, 2024

moelasmar commented Jan 31, 2024

mergify bot commented Jan 31, 2024

sakurai-ryo commented Jan 31, 2024

moelasmar commented Jan 31, 2024

moelasmar commented Jan 31, 2024

moelasmar commented Jan 31, 2024

aws-cdk-automation commented Feb 1, 2024

mergify bot commented Feb 1, 2024

fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited #28793

fix(stepfunctions): the Retry field in the statesJson in CustomState is always overwrited #28793

Conversation

sakurai-ryo commented Jan 20, 2024 • edited Loading

sakurai-ryo Jan 20, 2024

Choose a reason for hiding this comment

jacklin213 commented Jan 21, 2024

moelasmar Jan 23, 2024

Choose a reason for hiding this comment

sakurai-ryo Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

moelasmar Jan 30, 2024

Choose a reason for hiding this comment

mergify bot commented Jan 30, 2024

mergify bot commented Jan 30, 2024

GavinZZ commented Jan 31, 2024

mergify bot commented Jan 31, 2024

❌ Mergify doesn't have permission to update

moelasmar commented Jan 31, 2024

mergify bot commented Jan 31, 2024

❌ Mergify doesn't have permission to update

sakurai-ryo commented Jan 31, 2024

moelasmar commented Jan 31, 2024

moelasmar commented Jan 31, 2024

moelasmar commented Jan 31, 2024

aws-cdk-automation commented Feb 1, 2024

AWS CodeBuild CI Report

mergify bot commented Feb 1, 2024

sakurai-ryo commented Jan 20, 2024 •

edited

Loading

sakurai-ryo Jan 24, 2024 •

edited

Loading