fix(stepfunctions): map state validation fix #4382

apalumbo · 2019-10-05T07:36:30Z

Fixed validation for map state
Also added validation for maxConcurremcy

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

…e, added synth unit tests

mergify · 2019-10-05T07:37:07Z

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
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-05T07:42:26Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

rhboyd · 2019-10-05T13:25:24Z

Why is maxConcurrency mandatory? It should be optional with a default to like 10 or 100 if the user doesn’t supply it.

apalumbo · 2019-10-05T14:38:14Z

It is not mandatory, is number | undefined , in this way you have to provide a number or not set a value. null is not permitted , in this way the check if the value is set is easier

…at has to be a positive integer

aws-cdk-automation · 2019-10-05T17:09:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-05T17:57:00Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

apalumbo · 2019-10-06T14:21:02Z

@rhboyd The default value for maxConcurrency (if not provided is 0 from the step functions)

You can configure an upper bound on how many concurrent sub-workflows Map executes by adding the MaxConcurrency field. The default value is 0, which places no limit on parallelism and iterations are invoked as concurrently as possible. A MaxConcurrency value of 1 has the effect to invoke the Iterator one element at a time, in the order of their appearance in the input state, and will not start an iteration until the previous iteration has completed execution.

https://aws.amazon.com/blogs/aws/new-step-functions-support-for-dynamic-parallelism/

rhboyd · 2019-10-06T14:26:46Z

Awesome. Thanks for clarifying it. I was trying to read the PR on mobile and I confused myself. +1

rhboyd · 2019-10-06T14:28:01Z

Also, tagging @rix0rrr since he shepherded the original release into production.

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts

aws-cdk-automation · 2019-10-06T16:04:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-08T16:04:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

yyolk · 2019-10-08T21:42:08Z

I tried sync'ing a graph using this branch (only updating node-aws-cdk with npm link), I ended up getting an error about Iterator not being filled, copying this branch's test I also tried something like the following:

m = sfn.Map(
    self,
    logical_name,
    input_path=input_path,
    output_path=output_path,
    max_concurrency=10,
    **kwargs,
)
m.iterator(sfn.Pass(self, "Pass in Map"))

This error'd for the same reason. I figure this is likely because I didn't also upgrade the python packages I import (cdk is only at runtime).

I'm going through that now, running the pack.sh script after installing maven, python-site-packages, and dotnet. I'm also about to rap up for the day, so I'm hoping this gets merged and published before then ;)

rix0rrr · 2019-10-10T08:55:14Z

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts

@@ -150,10 +150,30 @@ export class Map extends State implements INextable {
     * Validate this state
     */
    protected validate(): string[] {
-        if (!!this.iterator) {
-            return ['Map state must have a non-empty iterator'];
+        const validateMaxConcurrency = () => {


Not sure why this needs to be an inner function? I'd rather you extract the logic to test that a number is a positive integer to a function isPositiveInteger() and call that inline in this function.

Ok i will move the function outside Map and export it for the test, I will add tests for the function (the samecases I added) but I would like to leave the tests on synth to take care about regressions

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts

…ded tests

apalumbo · 2019-10-10T18:59:26Z

@rix0rrr It should be ok now

aws-cdk-automation · 2019-10-10T19:00:36Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-10T19:04:27Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-10T22:47:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

apalumbo · 2019-10-14T12:20:36Z

@rix0rrr Do you think that, is the review it's ok, there is any chance that this will go in the next release? We are waiting for this to complete a feature in one of our projects and it would be great

aws-cdk-automation · 2019-10-16T19:23:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-10-21T09:44:47Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-21T10:23:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sarveshbhagat · 2019-10-31T15:34:54Z

I tried sync'ing a graph using this branch (only updating node-aws-cdk with npm link), I ended up getting an error about Iterator not being filled, copying this branch's test I also tried something like the following:
m = sfn.Map(
    self,
    logical_name,
    input_path=input_path,
    output_path=output_path,
    max_concurrency=10,
    **kwargs,
)
m.iterator(sfn.Pass(self, "Pass in Map"))
This error'd for the same reason. I figure this is likely because I didn't also upgrade the python packages I import (cdk is only at runtime).

I'm going through that now, running the pack.sh script after installing maven, python-site-packages, and dotnet. I'm also about to rap up for the day, so I'm hoping this gets merged and published before then ;)

Is this working for you?

Stvad · 2019-11-09T00:51:30Z

Should iterator be added to MapProps ?

fix(stepfunctions) Fixed validation for missing iterator for Map stat…

91eb65f

…e, added synth unit tests

apalumbo requested a review from eladb as a code owner October 5, 2019 07:36

feat(stepfunctions): Added test and validation for maxConcurrency, th…

3eaf502

…at has to be a positive integer

apalumbo force-pushed the stepFunctionsMapImplementationFix branch from be89962 to 3eaf502 Compare October 5, 2019 16:28

Merge branch 'master' into stepFunctionsMapImplementationFix

6316b83

refactor(stepfunctions): Refactored Map test to reduce code duplication

d3774e7

apalumbo commented Oct 6, 2019

View reviewed changes

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts Show resolved Hide resolved

NGL321 assigned eladb Oct 7, 2019

apalumbo mentioned this pull request Oct 8, 2019

Step function Map throw error on synth #4417

Closed

Merge branch 'master' into stepFunctionsMapImplementationFix

ae9e033

NGL321 assigned rix0rrr and unassigned eladb Oct 8, 2019

rix0rrr reviewed Oct 10, 2019

View reviewed changes

refactor(stepfunctions): extracted integer validation outside map, ad…

18c6906

…ded tests

apalumbo requested a review from rix0rrr October 10, 2019 18:58

Merge branch 'master' into stepFunctionsMapImplementationFix

572a2d0

fix(stepfunctions) Fixed map test and isPositiveInteger

b9c2fa5

Merge branch 'master' into stepFunctionsMapImplementationFix

bd8c8dd

rix0rrr approved these changes Oct 21, 2019

View reviewed changes

Merge branch 'master' into stepFunctionsMapImplementationFix

d67ec09

mergify bot merged commit bbe0380 into aws:master Oct 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions): map state validation fix #4382

fix(stepfunctions): map state validation fix #4382

apalumbo commented Oct 5, 2019 •

edited

Loading

mergify bot commented Oct 5, 2019

aws-cdk-automation commented Oct 5, 2019

rhboyd commented Oct 5, 2019

apalumbo commented Oct 5, 2019

aws-cdk-automation commented Oct 5, 2019

aws-cdk-automation commented Oct 5, 2019

apalumbo commented Oct 6, 2019

rhboyd commented Oct 6, 2019

rhboyd commented Oct 6, 2019

aws-cdk-automation commented Oct 6, 2019

aws-cdk-automation commented Oct 8, 2019

yyolk commented Oct 8, 2019

rix0rrr Oct 10, 2019

apalumbo Oct 10, 2019

apalumbo commented Oct 10, 2019

aws-cdk-automation commented Oct 10, 2019

aws-cdk-automation commented Oct 10, 2019

aws-cdk-automation commented Oct 10, 2019

apalumbo commented Oct 14, 2019 •

edited

Loading

aws-cdk-automation commented Oct 16, 2019

mergify bot commented Oct 21, 2019

aws-cdk-automation commented Oct 21, 2019

sarveshbhagat commented Oct 31, 2019

Stvad commented Nov 9, 2019

fix(stepfunctions): map state validation fix #4382

fix(stepfunctions): map state validation fix #4382

Conversation

apalumbo commented Oct 5, 2019 • edited Loading

mergify bot commented Oct 5, 2019

aws-cdk-automation commented Oct 5, 2019

AWS CodeBuild CI Report

rhboyd commented Oct 5, 2019

apalumbo commented Oct 5, 2019

aws-cdk-automation commented Oct 5, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 5, 2019

AWS CodeBuild CI Report

apalumbo commented Oct 6, 2019

rhboyd commented Oct 6, 2019

rhboyd commented Oct 6, 2019

aws-cdk-automation commented Oct 6, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 8, 2019

AWS CodeBuild CI Report

yyolk commented Oct 8, 2019

rix0rrr Oct 10, 2019

Choose a reason for hiding this comment

apalumbo Oct 10, 2019

Choose a reason for hiding this comment

apalumbo commented Oct 10, 2019

aws-cdk-automation commented Oct 10, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 10, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 10, 2019

AWS CodeBuild CI Report

apalumbo commented Oct 14, 2019 • edited Loading

aws-cdk-automation commented Oct 16, 2019

AWS CodeBuild CI Report

mergify bot commented Oct 21, 2019

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

sarveshbhagat commented Oct 31, 2019

Stvad commented Nov 9, 2019

apalumbo commented Oct 5, 2019 •

edited

Loading

apalumbo commented Oct 14, 2019 •

edited

Loading