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

fix(sfn) Pass: support more types for result #2811

Merged
merged 11 commits into from
Jun 21, 2019

Conversation

spg
Copy link
Contributor

@spg spg commented Jun 10, 2019

Brings back compatibility for some types that were deleted by #2701

These types are supported, as exemplified by the following stack:

---
AWSTemplateFormatVersion: '2010-09-09'
Resources:
  MyStateMachine:
    Type: AWS::StepFunctions::StateMachine
    Properties:
      DefinitionString: |-
        {
          "StartAt": "State1",
          "States": {
            "State1": {
              "Type": "Pass",
              "Result": {
                "x-datum": 0.381018,
                "y-datum": 622.2269926397355
              },
              "ResultPath": "$.state1",
              "Next": "State2"
            },
            "State2": {
              "Type": "Pass",
              "Result": true,
              "ResultPath": "$.state2",
              "Next": "State3"
            },
            "State3": {
              "Type": "Pass",
              "Result": "something",
              "ResultPath": "$.state3",
              "Next": "State4"
            },
            "State4": {
              "Type": "Pass",
              "Result": 1,
              "ResultPath": "$.state4",
              "Next": "State5"
            },
            "State5": {
              "Type": "Pass",
              "Result": 1.01,
              "ResultPath": "$.state5",
              "Next": "State6"
            },
            "State6": {
              "Type": "Pass",
              "Result": null,
              "ResultPath": "$.state6",
              "Next": "State7"
            },
            "State7": {
              "Type": "Pass",
              "Result": [1, "something", true, 1.01],
              "ResultPath": "$.state7",
              "End": true
            }
          }
        }
      RoleArn: arn:aws:iam::XXXXXX:role/service-role/StatesExecutionRole-us-east-1

BREAKING CHANGE: Pass.result needs to be built using sfn.Result.fromXXX()


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@spg spg requested a review from a team as a code owner June 10, 2019 18:03
@spg
Copy link
Contributor Author

spg commented Jun 10, 2019

Not sure how I should interpret this error (from Travis build log):

@aws-cdk/aws-stepfunctions: [2019-06-10T18:20:57.037] [ERROR] jsii/compiler - Type model errors prevented the JSII assembly from being created
@aws-cdk/aws-stepfunctions: lib/states/pass.ts:54:5 - error TS9999: JSII: Non-primitive types must have a symbol
@aws-cdk/aws-stepfunctions: 54     readonly result?: {[key: string]: any} | string | number | boolean | null | any[];
@aws-cdk/aws-stepfunctions:        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@aws-cdk/aws-stepfunctions: Error: /home/travis/build/awslabs/aws-cdk/tools/cdk-build-tools/node_modules/jsii/bin/jsii --project-references exited with error code 1
@aws-cdk/aws-stepfunctions: Build failed. Total time (10.6s) | /home/travis/build/awslabs/aws-cdk/tools/cdk-build-tools/node_modules/jsii/bin/jsii (10.3s) | cfn2ts (0.3s)

@eladb
Copy link
Contributor

eladb commented Jun 11, 2019

Union types are not supported in jsii (which is the tech we use to package the CDK code to multiple programming languages), because it's impossible to express union types in most languages.

What we normally do in those cases is implement a "union-like class". For example see the lambda.Code class.

@@ -51,7 +51,7 @@ export interface PassProps {
*
* @default No injected result
*/
readonly result?: {[key: string]: any};
readonly result?: {[key: string]: any} | string | number | boolean | null | any[];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't use type unions, at least not like this.

Is this actually blocking you somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My previous state machine (0.33.0) was using result: true, which I had to modify to result: {something: true} when upgrading to 0.34.0.

My intuition was that if CFN supports a feature (in this case, result can be {[key: string]: any} | string | number | boolean | null | any[]), the CDK should allow it. Correct me if I'm wrong here :)

I can explore going down the "union-like class" path if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that will have to be the solution then.

@spg spg force-pushed the fix-sfn-more-types-for-pass branch from 8e9ec3d to b5794ad Compare June 15, 2019 00:00
@spg
Copy link
Contributor Author

spg commented Jun 17, 2019

@rix0rrr @eladb I implemented the "union-like" class. I can also add documentation if you wish.

@rix0rrr rix0rrr self-assigned this Jun 21, 2019
@rix0rrr rix0rrr added the p0 label Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants