-
Notifications
You must be signed in to change notification settings - Fork 4k
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(aws-stepfunctions): support StateMachineType #5398
Conversation
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -98,8 +130,11 @@ export class StateMachine extends StateMachineBase { | |||
const graph = new StateGraph(props.definition.startState, `State Machine ${id} definition`); | |||
graph.timeout = props.timeout; | |||
|
|||
this.stateMachineType = props.stateMachineType ? props.stateMachineType : StateMachineType.STANDARD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the default in CloudFormation perhaps STANDARD
? I think so, no? If so, I'd rather we default to undefined
here, so that the churn on the existing CloudFormation templates is minimal.
The @default
annotation on the stateMachineType
property should STILL indicate that the default behavior is STANDARD
. @default
annotations concern themselves with behavior of the stack as a whole, not with the details of what is rendered to a CFN temlate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback.
In my latest commit, the member variable stateMachineType
is set to STANDARD
by default (line 133), while the parameter stateMachineType
in the cloudformation resource keeps undefined
if users do not specify this optional field.
This solution helps avoid breaking the existing integration tests. :)
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
import { Test } from 'nodeunit'; | ||
import stepfunctions = require('../lib'); | ||
|
||
export = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added unit tests to cover different scenarios regarding stateMachineType
.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request is now being automatically merged. |
A new optional parameter
StateMachineType
has been introduced to the module ofaws-stepfunctions
by this commit.enum
because onlyEXPRESS
andSTANDARD
are acceptable.STANDARD
if users do not assign value to this field.The corresponding comments and tests have been created as well.
Closes #5397
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license