Skip to content
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

stepfunctions: Retry block in CustomState is always empty. #28586

Closed
troter opened this issue Jan 5, 2024 · 2 comments · Fixed by #28598 · May be fixed by stack-spot/app-handler-functions-template#2, stack-spot/eks-env-ts-template#2 or stack-spot/web-react-deploy#4
Labels
@aws-cdk/aws-stepfunctions Related to AWS StepFunctions bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@troter
Copy link

troter commented Jan 5, 2024

Describe the bug

After this(#28422) changes, Retry block in CustomState is always empty.

because Retry is always overwritten here.

https://github.com/graydenshand/aws-cdk/blob/3e7c777f28d3375acdb6bf1446c2cd159eadce6d/packages/aws-cdk-lib/aws-stepfunctions/lib/states/custom-state.ts#L56-L65

Expected Behavior

  • The Retry block passed to the constructor will be used as is.
  • or add addRetry method.

Current Behavior

Retry is always overwrite by empty array.

Reproduction Steps

Pass stateJson that has Retry lock to CustomState.

const stateJson = {
    Type: "Task",
    Resource: "arn:aws:states:::ecs:runTask.sync",
    ResultPath: "$.results.ECSRunTask",
    TimeoutSeconds: defaultTimeoutHours * 60 * 60,
    Retry: [
        {
            ErrorEquals: ["ECS.AmazonECSException", "ECS.ServerException"],
            IntervalSeconds: 60,
            MaxAttempts: 3,
        },
    ],
    Parameters: {
        LaunchType: "FARGATE",
        Group: ecsServiceName,
        Cluster: clusterArn,
        TaskDefinition: taskDefinitionArn,
    },
};

const RunTaskState = new sfn.CustomState(this, "ECS RunTask.", {
    stateJson: stateJson,
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.116.0

Framework Version

No response

Node.js Version

18.15.0

OS

macOS Sonoma 14.2.1

Language

TypeScript

Language Version

No response

Other information

No response

@troter troter added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 5, 2024
@github-actions github-actions bot added the @aws-cdk/aws-stepfunctions Related to AWS StepFunctions label Jan 5, 2024
@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jan 5, 2024
@mergify mergify bot closed this as completed in #28598 Jan 8, 2024
mergify bot pushed a commit that referenced this issue Jan 8, 2024
Currently it is not possible to set Retry for the Task when using CustomState construct.
There are two ways to add Retry: by adding the `addRetry` method or by rendering the `Retry` property of `stateJson`.
The `addRetry` method was added in this PR because rendering the `Retry` property of `stateJson`, which was not rendered before, could result in an unexpected StateMachine for the user.

Closes #28586

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

github-actions bot commented Jan 8, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@jacklin213
Copy link
Member

jacklin213 commented Jan 18, 2024

Hey @pahud,

Can the issue be re-opened. #28598 only satisfies bullet 2 in the expected behavior section in the original issue post.

I don't believe the actual regression (bullet 1) has been addressed.

Users upgrading from v2.115.0 and before, that had Retry in their stateJson passed as CustomStateProps will find that even after they upgrade to v2.119.0, the state machine definition will be missing the Retry entry.
(Tested on latest v2.121.1 version)

EDIT: Creating separate issue as suggested by the bot

mergify bot pushed a commit that referenced this issue Feb 1, 2024
…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*
SankyRed pushed a commit that referenced this issue Feb 8, 2024
…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*
TheRealAmazonKendra pushed a commit that referenced this issue Feb 9, 2024
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment