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(stepfunctions-tasks): start a nested state machine execution as a construct #8178

Merged
merged 12 commits into from
Jun 3, 2020

Conversation

shivlaks
Copy link
Contributor

@shivlaks shivlaks commented May 24, 2020

This class is the replacement for the previous StartExecution class.

There are a few differences:

  1. the stateMachine parameter has been moved into props.
    Rationale: alignment with constructs.

  2. the resource ARN that's generated in the Amazon States
    language uses sync:2. This returns an output of JSON instead
    of a string.
    Rationale: alignment with Step Functions team recommendation.

  3. The input parameter has been changed to be of type sfn.TaskInput
    Rationale: previous type precluded the ability to assign state input to
    this parameter.


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

shivlaks added 6 commits May 24, 2020 12:58
This class is the replacement for the previous `StartExecution` class.

There are a few differences:
1. the `stateMachine` parameter has been moved into props.
Rationale: alignment with constructs.

2. the resource ARN that's generated in the Amazon States
language uses `sync:2`. This returns an output of JSON instead
of a string.
Rationale: alignment with Step Functions documentation.

3. The `input` parameter has been changed to be of type `sfn.TaskInput`
Rationale: previous type precluded the ability to assign state input to
this parameter.
@shivlaks shivlaks added the pr/do-not-merge This PR should not be merged at this time. label May 24, 2020
@shivlaks shivlaks requested review from rix0rrr and nija-at May 24, 2020 21:28
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 24, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 83eb1d2
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: aee6c47
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0601585
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0601585
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@ccfife ccfife mentioned this pull request May 27, 2020
24 tasks
@@ -39,6 +41,8 @@ export interface StartExecutionProps {
* A Step Functions Task to call StartExecution on another state machine.
*
* It supports three service integration patterns: FIRE_AND_FORGET, SYNC and WAIT_FOR_TASK_TOKEN.
*
* @deprecated - use 'StepFunctionsStartExecution'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as with batch.

Can we call new StepFunctionsStartExecution() in the constructor here and delete most of the code in this file?

Copy link
Contributor Author

@shivlaks shivlaks Jun 3, 2020

Choose a reason for hiding this comment

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

I think that's pretty natural and organic if we were replacing one API with another to create an adapter and call into the replacement. I don't think this is a scenario where we can just proxy into the replacement as the replacement contains and configures properties (i.e. inputPath, outputPath, ...) that the older

In this case, we are replacing a class that implemented a bind method to contribute a portion of the properties to represent a Task with a class that's a construct and configures all of the properties for a Task state.

The user experience with the current format will be to embed the call to start execution into the invocation of a new Task. I don't think proxying to invoke a new StepFunctionStartExeuction yields the right replacement.

new Task(this, 'my-task', {
  task: new tasks.ExecuteStateMachine(child, {...}),
...
}

Some of the other ones to come like DynamoDB and ECS, I've tried to reuse the shared types. I could perhaps move more into a central location here and DRY it up a bit.

Did you have something else in mind? If so, are you opposed to me making a subsequent PR to do some refactoring?

@shivlaks shivlaks requested a review from nija-at June 3, 2020 03:07
@shivlaks
Copy link
Contributor Author

shivlaks commented Jun 3, 2020

@nija-at would love if you could take another look when you get a chance!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 678bd34
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 33c6520
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@shivlaks shivlaks changed the title feat(stepfunctions-tasks): start a Step Functions execution as a task feat(stepfunctions-tasks): start a nested state machine execution as a construct Jun 3, 2020
@shivlaks shivlaks removed the pr/do-not-merge This PR should not be merged at this time. label Jun 3, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: a9fbe07
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 3000dd5 into master Jun 3, 2020
@mergify mergify bot deleted the shivlaks/sfn-tasks-sfn-integration branch June 3, 2020 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants