feat(pipes-targets): add step functions state machine target

[WtfJoke]

Issue #29665

Closes #29665

Reason for this change

Step Function target is not supported yet by pipes-targets.

Description of changes

• Added step function as a pipes target.
• I've decided to make the invocationType a required parameter, since this made the code clearer and make users aware of how they want the step function to be invoked.
  The AWS::Pipes::Pipe PipeTargetStateMachineParameters has this as an optional parameter (defaulting to Request-Response), which can lead the user unknowingly in a broken pipe, because cdk's StateMachines default to Standard Workflow, which is not compatible with Request-Response Invocation Type.
• Currently there seems no way to prevent users from creating a pipe with an imported Standard StateMachine and InvocationType Request-Response as the stateMachineType cant be read (or I dont know how :D)

Description of how you validated changes

• Added unit tests
• Added integration test

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

I've talked with @RaphaelManke and he was fine for me opening up a PR (put him as a co-author nevertheless) :)

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

[aws-cdk-automation]

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.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[WtfJoke]

@mrgrain just wanted to ask if there is something else I should change or does an additional person needs to review my PR (or I just need to wait a bit more)?

[mrgrain]

@mrgrain just wanted to ask if there is something else I should change or does an additional person needs to review my PR (or I just need to wait a bit more)?

Someone else should get around to this eventually. I'm not actually working on the CDK anymore 🙈
But ping me if you don't get a review next week!

[WtfJoke]

Thanks, appreciate it! Lets hope its not necessary :D

[WtfJoke]

@mrgrain unfortunately I need to come back to your offer as the PR hasnt been merged and a week passed. So I hope pinging you helps :D Thanks!

[mrgrain]

Looks great! Got a couple more minor renames and let's set a default for InvocationType so that most users won't have to specify parameters at all.

Also trying to get @RaphaelManke to review as well

[RaphaelManke]

Overall great job @WtfJoke.
I have only one thing that I am not 100% happy about.
Can we define a common name for Step function that is used all over the place?
Currently we have :

• AWS Step Functions
• StepFunction
• StateMachine
• stepFunctionStateMachine
• sfn

The EventBridge targets constructs calls the target
SfnStateMachine

Maybe we should align to the naming of that because its a name from an already stable package.
Whats your thought in that @mrgrain ?

My suggestion is, name the target SfnStateMachine

Pull request has been modified.

[WtfJoke]

Can we define a common name for Step function that is used all over the place?

This is something I am not happy about it as well.
I tried to Stick mostly to the term "Step Function", but the terms Step Function and State Machine are mostly used interchangeable. I'm open for either name.

Please keep in mind its not only the Targets name (StepFunctionTarket vs StateMachineTarget vs SfnStateMachineTarget (@RaphaelManke you meant to include Target right?)) but also the in the corresponding docs.

Currently in the docs:

StateMachine StepFunction target properties.

or

InvocationType for invoking StepFunction.
Invoke StepFunction asynchronously (StartExecution). See https://docs.aws.amazon.com/step-functions/latest/apireference/API_StartExecution.html for more details.

or in the README

AWS Step Functions

I am not so eager on writing SfnStateMachine everywhere in the docs. I would favor choosing StepFunction or StateMachine there.
What do you think @RaphaelManke @mrgrain?

P.S: I've rebased my changes on latest main and incorporated the review feedback from @mrgrain.

[mrgrain]

• AWS Step Functions

This should be used in docs, when referring to the service.
"AWS Step Functions State Machine" can be used to refer to the resource in the docs, but when used multiple times "State Machine" is enough.

• StepFunction

This should never be used, given that it is missing an s ;)

• StateMachine

This the correct way to refer to just the resource.

• stepFunctionStateMachine

Ignoring the case, this Would be correct way to name the combination of service & resource

• sfn

The EventBridge targets constructs calls the target SfnStateMachine

I'm inclined to agree. sfn is the short version used by the SDK as well.

Maybe we should align to the naming of that because its a name from an already stable package. Whats your thought in that @mrgrain ?

Probably. I think we named the queue target wrong: SqsTarget when it really should be SqsQueueTarget.
But then all classes in this module are targets, so maybe SqsQueue would be enough.

My suggestion is, name the target SfnStateMachine

I come to the same conclusion, in particular about dropping the Target suffix. Which also means dropping the Target from TargetParameters I think.

There could be an argument about calling it StepFunctionsStateMachine (I kind of like it better) but given the precedence of EventBridge targets and that sfn is a semi official abbreviation, I'm okay with it.

[mrgrain]

This is something I am not happy about it as well. I tried to Stick mostly to the term "Step Function", but the terms Step Function and State Machine are mostly used interchangeable. I'm open for either name.

They should not be used interchangeably. We should make sure to explicitly call out the resource we are integrating with, which is the AWS Step Functions State Machine. Or shorter State Machine.

When grouping things, we tend to use the service. So that's why the file is named stepfunctions.ts or in the docs we might have a heading "AWS Step Functions". The idea being that if there were a second resource in the same service/module they would go into the same group. Of course in practice this rarely happens.

Please keep in mind its not only the Targets name (StepFunctionTarket vs StateMachineTarget vs SfnStateMachineTarget (@RaphaelManke you meant to include Target right?)) but also the in the corresponding docs.

Yeah I think we should drop Target from the name. We can still refer to it in the docs as "SfnStateMachine target" when we want to make it explicit we are talking about a target.

InvocationType for invoking StepFunction.
Invoke StepFunction asynchronously (StartExecution). See https://docs.aws.amazon.com/step-functions/latest/apireference/API_StartExecution.html for more details.

Just because you called it out here, I think this is a nice example for why it's important to differentiate between the service and the resource. StepFunction (without s) doesn't exist. And Step Functions (the service) doesn't have an InvocationType. Only a State Machine can be invoked with an InvocationType.

I am not so eager on writing SfnStateMachine everywhere in the docs. I would favor choosing StepFunction or StateMachine there. What do you think @RaphaelManke @mrgrain?

I can go over the docs again to give some concrete notes. In example code it would be SfnStateMachine. In copy text, it would likely be "State Machine" or "the State Machine target" or "parameters of the State Machine target", etc.

[WtfJoke]

I kinda prefer StepFunctionsStateMachine over SfnStateMachine, but in the end its up to you to decide.

I can go over the docs again to give some concrete notes. In example code it would be SfnStateMachine. In copy text, it would likely be "State Machine" or "the State Machine target" or "parameters of the State Machine target", etc.

Once we set on a name, I can go through the code+docs again and afterwards it would be great to have a review again. IMHO no need for docs review in advance (but as you like).

[WtfJoke]

So now everything should match. Thank you, that you took time for repeated reviews @mrgrain.
Lets hope the build goes through and we can get this merged 🤞

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 2712ba1
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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

[WtfJoke]

Thanks a bunch!