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(codepipeline): add support for a StepFunctions invoke action #8931

Merged
merged 9 commits into from
Jul 18, 2020
Merged

feat(codepipeline): add support for a StepFunctions invoke action #8931

merged 9 commits into from
Jul 18, 2020

Conversation

madhumitranrl
Copy link
Contributor

this feature enables developers using the cdk
to create stepfunction invoke action in their stacks.

verified the changes using unit tests.

Reviewed by: sainsbm


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

this feature enables developers using the cdk
to create stepfunction invoke action in their stacks.

verified the changes using unit tests.

Reviewed by: sainsbm
Fixed lint issues and verified the changes using unit tests
@skinny85 skinny85 changed the title feat: allow stepfunctions invoke action feat(codepipeline): add support for a StepFunctions invoke action Jul 8, 2020
Added validations for input and output artifacts.
Stringified StateMachineInput when the input type is literal

Tested the changes using unit tests. Modified StateMachineInputType FilePath
test to verify based on exception messages. This specific test was previously
verifying only if an exception was thrown.
Added Integration tests and verified with yarn integ.

README file has also been added which explains how to invoke a step function
from a pipeline.

Reviewed by: sainsbm
@madhumitranrl madhumitranrl marked this pull request as ready for review July 10, 2020 22:25
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @madhumitranrl !

I like it, but I'd like to have one major change done before I give my approval 🙂.

Comment on lines 808 to 809
definition: startState,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation:

Suggested change
definition: startState,
});
definition: startState,
});

Comment on lines 833 to 834
definition: startState,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation:

Suggested change
definition: startState,
});
definition: startState,
});

@@ -794,3 +794,56 @@ new codepipeline_actions.CodeBuildAction({

See [the AWS documentation](https://docs.aws.amazon.com/codepipeline/latest/userguide/actions-invoke-lambda-function.html)
on how to write a Lambda function invoked from CodePipeline.

#### AWS Step Functions
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been changed to ### on current master, if you could update the PR with the latest.

* If InputType is set to FilePath, this artifact is required
* and is used to source the input for the state machine execution.
*
* @default the Action will not have any inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @default the Action will not have any inputs
* @default - the Action will not have any inputs

*
* @default - action execution ID
*/
readonly executionPrefixName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to executionNamePrefix?

*
* @default - none
*/
readonly stateMachineInput?: string | object;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice try, but we don't allow using union types in L2s, because they don't translate to other languages well 🙂.

Since this value is actually closely tied to the input property as well, can I offer an alternative design here?

export interface StepFunctionsInvokeActionProps extends codepipeline.CommonAwsActionProps {
  readonly output?: codepipeline.Artifact;
  readonly stateMachine: stepfunction.IStateMachine;
  readonly executionPrefixName?: string;
  readonly stateMachineInput?: StateMachineInput;
}

export class StateMachineInput {
  public static filePath(filePath: string, inputArtifact: codepipeline.Artifact): StateMachineInput { ... }

  public static literal(object: object): StateMachineInput { ... }

  private constructor(...) { ... }

  // all 3 are read in bind()
  public readonly inputArtifact?: codepipeline.Artifact;
  public readonly inputType?: string;
  public readonly input: any;
}

Updated the design to include inputArtifact, inputType and input
as a single StateMachineInput unit.

Modified unit and integration tests based on the design change.
Updated README as well.
@mergify mergify bot dismissed skinny85’s stale review July 16, 2020 22:00

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

@madhumitranrl don't get mad at me, but I figured out an even nicer API for this Action. Sorry that I'm making you go through this, but I just forgot about the ArtifactPath class, which is a perfect fit for this use case.

If you had enough of my comments, let me know, and I'll try to take over the PR, and get it over the line 🤪.

Thanks!

const pipeline = new codepipeline.Pipeline(this, 'MyPipeline');
const startState = new stepfunction.Pass(stack, 'StartState');
const simpleStateMachine = new stepfunction.StateMachine(stack, 'SimpleStateMachine', {
definition: startState,
Copy link
Contributor

Choose a reason for hiding this comment

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

2 spaces instead of 4:

Suggested change
definition: startState,
definition: startState,

const stepFunctionAction = new codepipeline_actions.StepFunctionsInvokeAction({
actionName: 'Invoke',
stateMachine: simpleStateMachine,
stateMachineInput: codepipeline_actions.StateMachineInput.literal({IsHelloWorldExample: true}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stateMachineInput: codepipeline_actions.StateMachineInput.literal({IsHelloWorldExample: true}),
stateMachineInput: codepipeline_actions.StateMachineInput.literal({ IsHelloWorldExample: true }),

Comment on lines 821 to 823
The StateMachineInputType can be a Literal or FilePath.
Input artifact and StateMachineInputField are required when
the StateMachineInputType is set as FilePath.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The StateMachineInputType can be a Literal or FilePath.
Input artifact and StateMachineInputField are required when
the StateMachineInputType is set as FilePath.
The `StateMachineInput` can be created with one of 2 static factory methods:
`literal`, which takes an arbitrary map as its only argument, or `filePath`:

const inputArtifact = new codepipeline.Artifact();
const startState = new stepfunction.Pass(stack, 'StartState');
const simpleStateMachine = new stepfunction.StateMachine(stack, 'SimpleStateMachine', {
definition: startState,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
definition: startState,
definition: startState,

* When the input type is FilePath, input artifact and
* filepath must be specified.
*/
public static filePath(filePath: string, inputArtifact: codepipeline.Artifact): StateMachineInput {
Copy link
Contributor

@skinny85 skinny85 Jul 16, 2020

Choose a reason for hiding this comment

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

Apologies @madhumitranrl , but I came up with an even better API :). We have a class in the CodePipeline library exactly for this (file in an artifact) purpose: ArtifactPath. So, you can change this function to:

  public static filePath(inuputFile: codepipeline.ArtifactPath): StateMachineInput {
    return new StateMachineInput(inuputFile.location, inuputFile.artifact, 'FilePath');
  }

And the customer uses it like this:

StateMachineInput .filePath(buildArtifact.atPath('path/to/my/file.json'));

This will be perfect!

Now, with this design, what do you think about getting rid of StateMachineInput class? We can change StepFunctionsInvokeActionProps to be just:

export interface StepFunctionInvokeActionProps {
  // these 3 same as now
  readonly output?: codepipeline.Artifact;
  readonly stateMachine: stepfunction.IStateMachine;
  readonly executionNamePrefix?: string;
 
  // only one of inputFile and inputObject can be provided - we check that in the  StepFunctionsInvokeAction class
  readonly inputFile?: codepipeline.ArtifactPath;
  readonly inputObject?: object;
}

Let me know which one you like better!

/**
* StepFunction invoke Action that is provided by an AWS CodePipeline.
*/
export class StepFunctionsInvokeAction extends Action {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of calling it StepFunctionInvokeAction (not Functions)? It does invoke a single StepFunction...

// allow state machine executions to be inspected
options.role.addToPolicy(new iam.PolicyStatement({
actions: ['states:DescribeExecution'],
resources: [`arn:aws:states:*:*:execution:${this.props.stateMachine.stateMachineArn}:${this.props.executionNamePrefix ?? ''}*`],
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have a method for that in our standard library. This should be:

      resources: [Stack.of(this.props.stateMachine).formatArn({
        service: 'states',
        resource: 'execution',
        resourceName: `${this.props.stateMachine.stateMachineArn}:${this.props.executionNamePrefix ?? ''}*`,
        sep: ':',
      })],

new cpactions.StepFunctionsInvokeAction({
actionName: 'Invoke',
stateMachine: simpleStateMachine,
stateMachineInput: cpactions.StateMachineInput.literal({IsHelloWorldExample: true}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stateMachineInput: cpactions.StateMachineInput.literal({IsHelloWorldExample: true}),
stateMachineInput: cpactions.StateMachineInput.literal({ IsHelloWorldExample: true }),

Updated the design to include ArtifactPath class

Modified unit, integration tests and README accordingly
@mergify mergify bot dismissed skinny85’s stale review July 17, 2020 17:56

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

I have one more question before I ship this @madhumitranrl !

return {
configuration: {
StateMachineArn: this.props.stateMachine.stateMachineArn,
Input: (this.props.stateMachineInput) ? this.props.stateMachineInput.input : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to fill this out if stateMachineInput is undefined? We can't make it undefined as well?

configuration: {
StateMachineArn: this.props.stateMachine.stateMachineArn,
Input: (this.props.stateMachineInput) ? this.props.stateMachineInput.input : [],
InputType: (this.props.stateMachineInput) ? this.props.stateMachineInput.inputType : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks to TypeScript's safe navigation operator, this can be simplified to:

Suggested change
InputType: (this.props.stateMachineInput) ? this.props.stateMachineInput.inputType : undefined,
InputType: this.props.stateMachineInput?.inputType,

Updated the ternary operator with typescript safe navigation operator
Included the action statement

Unit and integration tests pass
@mergify mergify bot dismissed skinny85’s stale review July 17, 2020 20:16

Pull request has been modified.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution, and for putting up with me @madhumitranrl 😃

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 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
Copy link
Contributor

mergify bot commented Jul 18, 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 499776d into aws:master Jul 18, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@madhumitranrl
Copy link
Contributor Author

Looks great! Thanks for the contribution, and for putting up with me @madhumitranrl 😃

Thank you!

curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this pull request Aug 11, 2020
…s#8931)

this feature enables developers using the cdk
to create stepfunction invoke action in their stacks.

verified the changes using unit tests.

Reviewed by: sainsbm


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants