feat: add support for Step Functions #827

rix0rrr · 2018-10-02T08:13:24Z

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

…ions

ALSO: - Add support for policies - Add support for CloudWatch event rule targets

ALSO - Add Activity test

…ions

eladb

This looks great dude. Beautiful work.

My only "big" concern is the need to remember to use prefixStates. Feels a bit error-prone... I remember we discussed a way to make this automatic by utilizing the uniqueIds in the construct tree.

packages/@aws-cdk/aws-stepfunctions/README.md

eladb · 2018-10-02T16:35:45Z

packages/@aws-cdk/aws-stepfunctions/README.md

+    .next(waitX)
+    .next(getStatus)
+    .next(new stepfunctions.Choice(this, 'Job Complete?')
+        .on(stepfunctions.Condition.stringEquals('$.status', 'FAILED'), jobFailed)


Is "on" the term used by the service? I was expecting something like "case" or "when"

case is bound to be a keyword, so is if. I suppose when can be used, that is only a keyword in Ruby afaik.

Maybe just addChoice? I know you are not a fan of the add, but in this case it's actually adding to an array of choices, no?

Yes, but at the same time it's a fluent API. I will ask the Step Functions team what they prefer.

.on() doesn't give me any heartburn; there's no equivalent keyword in ASL, so no harm. .otherwise() is standing in for the operation ASL calls "Default":, so why not say .default()? If we were designing ASL today we would probably have used "else" instead.

packages/@aws-cdk/aws-stepfunctions/README.md

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

packages/@aws-cdk/aws-stepfunctions/lib/state-transition-metrics.ts

…ions

…ties

rix0rrr · 2018-10-09T11:23:40Z

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at Amazon.JSII.Generator.MethodExtensions.GetParametersJsonSyntaxToken(Method method) in /codebuild/output/src706358202/src/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/MethodExtensions.cs:line 47
   at Amazon.JSII.Generator.Class.ClassGenerator.<CreateType>g__CreateAttributes|1_0() in /codebuild/output/src706358202/src/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/Class/ClassGenerator.cs:line 33

Needs to wait for private constructor support to hit the .NET generator: aws/jsii#256

timbray

Generally looks pretty good to me.

timbray · 2018-10-10T03:43:58Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

+- Interface we need from State:
+
+    interface IState {
+        next(chainable): Chainable;


Why not an explicit task.endMachine() to model "End": true? In the future State Machines are going to need to be event-driven, which means multiple threads of execution. So being able to explicitly signal terminating the state machine is distinct and useful.

Oh, this whole file shouldn't have been here anymore, sorry. It's mostly outdated too.

Right now, absence of a Next is assumed to be an End state, and it doesn't necessarily end the whole machine (might also end a branch in a Parallel).

Can you elaborate a bit on the upcoming change, I don't quite get what it is.

timbray · 2018-10-10T03:45:41Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

+
+- State machine definition should be frozen (and complete) after being chained to.
+
+Nextable: Pass, Task, Wait, Parallel


All of these can be terminating.

timbray · 2018-10-10T03:45:59Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

+Use as:
+
+    new StateMachine(..., {
+        definintion: new Blah()


timbray · 2018-10-10T03:46:54Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

+- Can SMDef be mutable?
+
+QUESTION
+- Do branches in Parallel allow Timeout/Version?


Not on the branch itself, but obvs there can be task states in a branch with timeouts.

timbray · 2018-10-10T03:50:18Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

+PROBLEMS ENCOUNTERED
+--------------------
+
+    task1.catch(handler).then(task2)


Well, the option (b) doesn't make sense, since the Handler has its own "Next", there's no reason to think that it'd want to go to task2.

So I think that if you're going to put a .catch() in one of these fluent expressions, you just have to accept that the catch() might fling you off somewhere else in the state machine if an error happens.

timbray · 2018-10-10T04:02:50Z

packages/@aws-cdk/aws-stepfunctions/README.md

+});
+```
+
+If you don't like the visual look of starting a chain directly off the first


I really like it the fluent idiom fwiw. I worry a bit about the issue discussed above about what .catch() might mean, but it does look cool.

timbray · 2018-10-10T04:07:49Z

packages/@aws-cdk/aws-stepfunctions/README.md

+
+Contributions welcome:
+
+- [ ] A single `LambdaTask` class that is both a `Lambda` and a `Task` in one


Makes me a little nervous because I can see wanting to substitute some other kind of thing for a Lambda. One of the nice things about the Task state is that the "Resource" URI is opaque and allows switching around the kind of thing that deals with the input just by changing the Resource value. Also, not obvious what such a thing would buy you because all the other fields are the same; is there some extra value you could add if you knew that the intent was that the Task was specifically Lambda?

First off, let me say that adding this construct which is both a Lambda and a Task in one does not remove the existing classes, which can still be pieced together to treat any other thing as a Task.

I see a use case for it in a couple of ways:

Generally just cutting down on objects I need to create and piece together. Right now, in my state machine definitions I'll always use a Lambda and Task together, so why isn't that made easier for me? In other words, what I want to express is "place this piece of code at this point on the state machine". That is my intent, but instead I'm having to define a Lambda and then a Task that points to the Lambda, which feels like implementation details beyond my intent.

At least the same Timeout parameter needs to be plugged correctly into both constructs, the new construct takes a single timeout and passes it onto both Lambda and Task.

I think people are going to want a SingletonLambda most of the time while defining reusable state machines. That way, they can define the Lambda and its code as part of the reusable piece, and then instantiate it arbitrarily many times (while still only getting one copy of the Lambda in their stack). The concern of switching between a per-instantiation and Singleton Lambda could nicely be hidden in that construct I'm talking about.

timbray · 2018-10-10T04:12:50Z

packages/@aws-cdk/aws-stepfunctions/lib/chain.ts

+
+/**
+ * A collection of states to chain onto
+ *


An example would help illustrate what this is for. I've written a few state machines and the idiom

new Chain(startState, endStates, state)

baffles me.

That constructor is private though. It's an implementation detail and you shouldn't be able to call it.

packages/@aws-cdk/aws-stepfunctions/lib/state-graph.ts

timbray · 2018-10-10T04:19:36Z

packages/@aws-cdk/aws-stepfunctions/DESIGN_NOTES.md

@@ -0,0 +1,216 @@
+- Goal: should be possible to define structures such as IfThenElse() by users.


I can see why, but every time you add the ability to express a construct that's not in ASL, this creates the risk that if ASL subsequently wants to add the same thing, probably with subtly varying semantics, things get awkward. I'd think there's a case for, in V1.0, sticking pretty close to what ASL can do.

rix0rrr · 2018-10-10T07:56:23Z

.otherwise() is standing in for the operation ASL calls "Default":, so why not say .default()?

Because that's a reserved keyword in most languages we're targeting (by way of switch/case/default). Same for else.

I think it'd be nice if the thing you use to catch errors at least had "catch" somewhere in its name. As you point out, "catch" is a keyword in lots of languages,

Best I can think of then is addCatch, to go along with addRetry. Better?

eladb · 2018-10-10T08:11:39Z

Because that's a reserved keyword in most languages we're targeting (by way of switch/case/default). Same for else.

How about nextState, catchState, defaultState?

…ions

…into huijbers/step-functions

eladb

Looks good!

### Highlights - __A new construct library for AWS Step Functions__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-stepfunctions/README.md)). The library provides rich APIs for modeling state machines by exposing a programmatic interface for [Amazon State Language](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html). - __A new construct library for Amazon S3 bucket deployments__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-deployment/README.md)). You can use now automatically populate an S3 Bucket from a .zip file or a local directory. This is a building block for end-to-end support for static websites in the AWS CDK. ### Bug Fixes * **aws-apigateway:** make LambdaRestApi proxy by default ([#963](#963)) ([a5f5e2c](a5f5e2c)), closes [#959](#959) * **aws-cdk:** Allow use of assumed roles behind a proxy ([#898](#898)) ([f2b1048](f2b1048)) * **aws-cdk:** Auto-delete stacks that failed creating before new attempt ([#917](#917)) ([2af8309](2af8309)) * **aws-cloudfront:** expose distributionId ([#938](#938)) ([f58d98c](f58d98c)) * **aws-dynamodb:** don't emit empty array properties ([#909](#909)) ([841975a](841975a)) * **docs:** use ..code to display file structure in "writing constructs" ([#935](#935)) ([b743362](b743362)) ### Features * **assets:** isZipArchive indicates if this is a zip asset ([#944](#944)) ([65190f9](65190f9)) * **aws-cdk:** deploy supports CloudFormation Role ([#940](#940)) ([393be6f](393be6f)), closes [#735](#735) * **aws-cloudformation:** allow specifying custom resource type ([#943](#943)) ([9de3a84](9de3a84)) * **aws-cloudformation:** correctly handle the templateConfiguration property in the CreateUpdateStack Pipeline Action. ([#923](#923)) ([d251a46](d251a46)) * **aws-cloudfront:** add support for "webAclId" ([#969](#969)) ([3ec9d76](3ec9d76)) * **aws-codedeploy:** add auto rollback configuration to server Deployment Group. ([#925](#925)) ([7ee91cf](7ee91cf)) * **aws-codedeploy:** add instance tag filter support for server Deployment Groups. ([#824](#824)) ([e6e8c51](e6e8c51)) * **aws-codedeploy:** add support for setting CloudWatch alarms on a server Deployment Group. ([#926](#926)) ([27b26b1](27b26b1)) * add support for Step Functions ([#827](#827)) ([81b533c](81b533c)) * **aws-lambda:** add grantInvoke() method ([#962](#962)) ([1ee8135](1ee8135)), closes [#961](#961) * **aws-lambda:** improvements to the code and runtime APIs ([#945](#945)) ([36f29b6](36f29b6)), closes [#902](#902) [#188](#188) [#947](#947) [#947](#947) [#664](#664) * **aws-logs:** extractMetric() returns Metric object ([#939](#939)) ([5558fff](5558fff)), closes [#850](#850) * **aws-s3:** initial support for website hosting ([#946](#946)) ([2d3661c](2d3661c)) * **aws-s3-deployment:** bucket deployments ([#971](#971)) ([84d6876](84d6876)), closes [#952](#952) [#953](#953) [#954](#954) * **docs:** added link to CloudFormation concepts ([#934](#934)) ([666bbba](666bbba)) ### BREAKING CHANGES * **aws-apigateway:** specifying a path no longer works. If you used to provide a '/', remove it. Otherwise, you will have to supply `proxy: false` and construct more complex resource paths yourself. * **aws-lambda:** The construct `lambda.InlineJavaScriptLambda` is no longer supported. Use `lambda.Code.inline` instead; `lambda.Runtime.NodeJS43Edge` runtime is removed. CloudFront docs [stipulate](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-requirements-limits.html#lambda-requirements-lambda-function-configuration) that you should use node6.10 or node8.10. It is always possible to use any value by instantiating a `lambda.Runtime` object.

adilosa and others added 27 commits July 26, 2018 17:50

Model the Amazon States Language

6a76727

Change to classes to make jsii happy

d74e4c8

Added ChoiceState, validations, and basic serialization tests

61f310b

Add method and class documentation

1fbdee3

Add unit tests

b4bad6b

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

f33cd7b

…ions

WIP

a7deb69

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

a5dd0b3

…ions

First iteration of object model and builder

813f4a4

WIP

385ae1f

Make an activity be possible to be the resource for a Task

4a1172c

Making all tests pass with mutable interface

9e54761

ALSO: - Add support for policies - Add support for CloudWatch event rule targets

Add TODO

c4e849f

Some more tests

0c32d6d

Add defaultRetry, get rid of internal ugliness in public API

2b402f9

ALSO - Add Activity test

Add integration test

9a80554

Add metrics

766d0c8

Metric tests for activities

006f98e

Add comments to all states

abb85ef

Add resultPath to error handler

dc74909

Add some more supported input/output/resultpaths

b23fe58

Support "Result" field on "Pass"

bd4f1da

Small fixes to deal with oddities of mutable state

9645ade

Shorten generated name if necessary

c53238d

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

d46454d

…ions

Update to latest API on 'master'

f8d534e

Finish first iteration

9a84c20

rix0rrr requested review from RomainMuller and eladb October 2, 2018 08:13

eladb approved these changes Oct 8, 2018

View reviewed changes

Rico Huijbers added 5 commits October 9, 2018 11:18

Review comments

beebb71

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

a9a5fa4

…ions

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

6c979a0

…ions

Update to new API

8307cd3

Work around jsii bug that does not register implicitly created proper…

584645d

…ties

timbray reviewed Oct 10, 2018

View reviewed changes

Removing obsolete DESIGN_NOTES

fe43fbd

Rico Huijbers added 5 commits October 11, 2018 10:27

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

ffe3533

…ions

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

4f792c8

…ions

Merge remote-tracking branch 'origin/master' into huijbers/step-funct…

b88b2ff

…ions

Method renames

ae693ab

Merge branch 'huijbers/step-functions' of github.com:awslabs/aws-cdk …

0e8cc0e

…into huijbers/step-functions

eladb approved these changes Oct 16, 2018

View reviewed changes

rix0rrr merged commit 81b533c into master Oct 16, 2018

rix0rrr deleted the huijbers/step-functions branch October 16, 2018 14:36

eladb mentioned this pull request Oct 19, 2018

v0.13.0 #975

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Step Functions #827

feat: add support for Step Functions #827

rix0rrr commented Oct 2, 2018

eladb left a comment

eladb Oct 2, 2018

rix0rrr Oct 8, 2018

eladb Oct 8, 2018

rix0rrr Oct 9, 2018

timbray Oct 10, 2018

rix0rrr commented Oct 9, 2018

timbray left a comment

timbray Oct 10, 2018

rix0rrr Oct 10, 2018

timbray Oct 10, 2018

timbray Oct 10, 2018

timbray Oct 10, 2018

timbray Oct 10, 2018

timbray Oct 10, 2018

timbray Oct 10, 2018

rix0rrr Oct 10, 2018

timbray Oct 10, 2018

rix0rrr Oct 10, 2018

timbray Oct 10, 2018

rix0rrr commented Oct 10, 2018

eladb commented Oct 10, 2018

eladb left a comment


		- State machine definition should be frozen (and complete) after being chained to.

		Nextable: Pass, Task, Wait, Parallel


		Contributions welcome:

		- [ ] A single `LambdaTask` class that is both a `Lambda` and a `Task` in one

		@@ -0,0 +1,216 @@
		- Goal: should be possible to define structures such as IfThenElse() by users.

feat: add support for Step Functions #827

feat: add support for Step Functions #827

Conversation

rix0rrr commented Oct 2, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Oct 9, 2018

timbray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Oct 10, 2018

eladb commented Oct 10, 2018

eladb left a comment

Choose a reason for hiding this comment