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

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

Merged
merged 1 commit into from
May 14, 2024

Conversation

WtfJoke
Copy link
Contributor

@WtfJoke WtfJoke commented Apr 27, 2024

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


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

@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Apr 27, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 27, 2024 23:17
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Apr 27, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@WtfJoke WtfJoke changed the title feat(pipes-targets): Add step function target feat(pipes-targets): add step function target Apr 27, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review April 27, 2024 23:21

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 27, 2024
@WtfJoke WtfJoke force-pushed the pipesStepFunctionTarget branch 2 times, most recently from 141dbf5 to 08d94b7 Compare April 28, 2024 16:10
This was referenced Apr 29, 2024
@WtfJoke
Copy link
Contributor Author

WtfJoke commented May 3, 2024

@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
Copy link
Contributor

mrgrain commented May 3, 2024

@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
Copy link
Contributor Author

WtfJoke commented May 3, 2024

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

@WtfJoke
Copy link
Contributor Author

WtfJoke commented May 13, 2024

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

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mrgrain mrgrain self-assigned this May 13, 2024
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 13, 2024
Copy link
Contributor

@RaphaelManke RaphaelManke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mergify mergify bot dismissed mrgrain’s stale review May 14, 2024 15:53

Pull request has been modified.

@WtfJoke WtfJoke force-pushed the pipesStepFunctionTarget branch 2 times, most recently from daeeba8 to f4b434b Compare May 14, 2024 15:55
@WtfJoke
Copy link
Contributor Author

WtfJoke commented May 14, 2024

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
Copy link
Contributor

mrgrain commented May 14, 2024

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

@WtfJoke
Copy link
Contributor Author

WtfJoke commented May 14, 2024

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
Copy link
Collaborator

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 14, 2024
Copy link
Contributor

mergify bot commented May 14, 2024

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

@mrgrain mrgrain changed the title feat(pipes-targets): add step function target feat(pipes-targets): add step functions state machine target May 14, 2024
@mergify mergify bot merged commit b0975e4 into aws:main May 14, 2024
13 checks passed
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 14, 2024
@WtfJoke
Copy link
Contributor Author

WtfJoke commented May 14, 2024

Thanks a bunch!

@WtfJoke WtfJoke deleted the pipesStepFunctionTarget branch May 14, 2024 20:36
@aws-cdk-automation
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(pipes-targets): Add Step functions target for EventBridge pipes
4 participants