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

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

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

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.

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

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

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

Thanks a bunch!

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.