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.

Sign up for GitHub

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

Jump to bottom

feat(pipes-targets): add step function target #29987

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(pipes-targets): add step function target #29987

wants to merge 1 commit into from

Conversation

WtfJoke
Copy link
Contributor

@WtfJoke WtfJoke commented Apr 27, 2024 •
edited

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
aws-cdk-automation
aws-cdk-automation previously requested changes Apr 27, 2024
View reviewed changes
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
mrgrain
mrgrain reviewed Apr 28, 2024
View reviewed changes
packages/@aws-cdk/aws-pipes-targets-alpha/lib/index.ts Outdated
@@ -1 +1,2 @@
export * from './sqs';
export * from './step-function';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure these are named consistently

Suggested change
export * from './step-function';
export * from './stepfunctions';

Copy link
Contributor Author

@WtfJoke WtfJoke Apr 28, 2024 •
edited

Choose a reason for hiding this comment

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

Thanks for taking a look already :)

Currently its consistent using step-function everywhere (as far as I can tell). Should I change it to the plural version of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you mean "everywhere"? I was referring to the module name which is aws-stepfunctions.

(You might totally be right about this by the way)

Copy link
Contributor Author

@WtfJoke WtfJoke Apr 28, 2024 •
edited

Choose a reason for hiding this comment

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

How do you mean "everywhere"?

I meant all the filenames inside this PR.

I was referring to the module name which is aws-stepfunctions.

Ah I think I know now what you meant. The package name/cdk module name is aws-stepfunctions and I should keep that name consistent also in this pipes-targets module, changed it to stepfunctions :)

I kept the name StepFunctionTarget if you think I should change that as well let me know.

mrgrain
mrgrain reviewed Apr 28, 2024
View reviewed changes
packages/@aws-cdk/aws-pipes-targets-alpha/README.md Outdated
@@ -63,3 +68,42 @@ const pipe = new pipes.Pipe(this, 'Pipe', {
target: pipeTarget
});
```

### AWS Step Function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### AWS Step Function
### AWS Step Functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Changed it

@WtfJoke @RaphaelManke
feat(pipes-targets): add step function target
08d94b7 
Co-authored-by: RaphaelManke <RaphaelManke@users.noreply.github.com>
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

This was referenced Apr 29, 2024
Closed
Closed
@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrgrain mrgrain mrgrain left review comments

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(pipes-targets): Add Step functions target for EventBridge pipes
3 participants
@WtfJoke @aws-cdk-automation @mrgrain