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(stepfunctions): add service integrations #1646

Merged
merged 24 commits into from
May 8, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Jan 31, 2019

Add service integrations for SNS, SQS and ECS to StepFunctions.

Integration tests are done, but

  • Unit Tests still need to be added

Pull Request Checklist

  • Testing
    • Unit test added
    • 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
  • Title and Description
    • Change type: title prefixed with fix, feat 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.

@rix0rrr rix0rrr requested review from SoManyHs and a team as code owners January 31, 2019 14:18
/**
* A StepFunctions Task to run a Task on ECS or Fargate
*/
export class BaseRunTask extends stepfunctions.Task implements ec2.IConnectable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this called "Base"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it should be abstract.

@@ -192,7 +192,7 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
/**
* Return the subnets appropriate for the placement strategy
*/
public subnets(placement: VpcPlacementStrategy = {}): IVpcSubnet[] {
public subnets(placement: VpcPlacementStrategy = {}, required = SubnetQuery.Required): IVpcSubnet[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some more details in the PR description about the changes needed in the VPC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was a bad idea. It should default the other way.

/**
* Require at least one subnet
*/
Required = 'Required',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense to make this a boolean? (required)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose. Options with a boolean field is better, this is awkward.

import { TaskDefinition } from './task-definition';

/**
* Properties for SendMessageTask
Copy link
Contributor

Choose a reason for hiding this comment

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

SendMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to make it clear it's a Task in a stepfunctions workflow.

}
if (securityGroup === undefined) {
securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { vpc });
}
const subnets = vpc.subnets(vpcPlacement);
this.connections.addSecurityGroup(securityGroup);

if (assignPublicIp === undefined && subnets.every(s => vpc.isPublicSubnet(s))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a little comment explaining this logic

packages/@aws-cdk/aws-stepfunctions/README.md Show resolved Hide resolved
queue that you poll on a compute fleet you manage yourself)
* `sns.PublishTask` -- publish a message to an SNS topic
* `sqs.SendMessageTask` -- send a message to an SQS queue
* `ecs.FargateRunTask`/`ecs.Ec2RunTask` -- run a container task
Copy link
Contributor

Choose a reason for hiding this comment

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

I rather these would have a common prefix, so maybe (along with my comment on "Task" above): RunStepFunctionsTaskOnFargate and RunStepFunctionsTaskOnEc2

/**
* The resource that represents the work to be executed
*
* Can be either a Lambda Function or an Activity.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comment can be updated

*
* @default No parameters
*/
parameters?: { [name: string]: any };
Copy link
Contributor

Choose a reason for hiding this comment

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

any => string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be nested objects too.

/**
* A StepFunctions Task to send a message to an SQS Queue
*/
export class SendMessageTask extends stepfunctions.Task {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add queue.toStepFunction() as a convenience/discovery mechanism?

@skinny85, this is similar to what we have for pipelines (toCodePipeline()).

@eladb
Copy link
Contributor

eladb commented Feb 5, 2019

@rix0rrr let's land this. Can I help somehow?

@eladb
Copy link
Contributor

eladb commented Feb 6, 2019

@rix0rrr ping pong

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Feb 8, 2019

Yeah. I'm also not sure about the XxxTask suffix (in relation to SFN), but I really have nothing better.

StepFunctionsTaskRunOnFargate is horrible. I guess we could do SfnRunFargateTask, but "sfn" is not super official, and it's also inconsistent with tasks inside the aws-stepfunctions package.

Alternatively we put them in a different package so it's obvious they're "extended" tasks. @aws-cdk/aws-stepfunctions-tasks maybe.

There's actually a conflation here, because the step in the Workflow is called a Task, and a running instance of a piece of code on Fargate is also called a Task. The official name should be something like FargateRunTaskStepFunctionsTask. Let's not put 2 Tasks in the class name though.

@eladb
Copy link
Contributor

eladb commented Feb 11, 2019

I think we have a precedence with codepipeline actions and I think the current naming convention is CodePipelineActionSomething.

I know it’s ugly and long but there is a benefit in the common prefix and deconflation

@eladb
Copy link
Contributor

eladb commented Feb 11, 2019

It’s actually not that consistent with pipelines. Let’s coordinate with @skinny85 and come up with a convention?

@skinny85
Copy link
Contributor

So for CodePipeline, the convention is currently:

  • <servicename>.Pipeline<ActionType>Action (for example, s3.PipelineSourceAction) for actions outside the CodePipeline package
  • codepipeline.<Service><ActionType>Action (for example, codepipeline.GitHubSourceAction) for actions inside the CodePipeline package

However, as we've discussed earlier, this convention is:

a) inconsistent (different inside vs outside the CodePipeline package),
b) does not translate to languages without prefixed namespaces (like Java and C#), and
c) depends on the customer using the same prefix for their import as we recommend (codepipeline).

Given all of the above issues, I'm more than happy to iterate on it and come up with something better, that fits both CodePipeline and Step Functions.

@eladb
Copy link
Contributor

eladb commented Feb 12, 2019

@skinny85 that will be great if we can formalize this. @rix0rrr also had a few other naming conventions in mind that we wanted to formalize. How about kicking off a PR that adds a section to the design/aws-guidelines doc and we can iterate there?

@adjmaker
Copy link

Hello, what's going on with this PR?

Is @rix0rrr still working on this?

@rix0rrr rix0rrr force-pushed the huijbers/stepfunctions-service-integrations branch from 0455c51 to 7766d0e Compare April 27, 2019 11:48
@@ -115,7 +115,7 @@ export enum SubnetType {
* This can be good for subnets with RDS or
* Elasticache endpoints
*/
Isolated = 1,
Isolated = 'Isolated',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: usually we use lowercase for the values when they are string

@@ -86,6 +86,7 @@
"@aws-cdk/aws-secretsmanager": "^0.29.0",
"@aws-cdk/aws-servicediscovery": "^0.29.0",
"@aws-cdk/aws-sns": "^0.29.0",
"@aws-cdk/aws-stepfunctions": "^0.29.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

revert I presume?

@@ -533,6 +533,10 @@ export class PolicyStatement extends cdk.Token {
return undefined;
}

if (cdk.unresolved(values)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Token.isToken

import cdk = require('@aws-cdk/cdk');

/**
* Properties for SendMessageTask
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this doc needs a refresher

/**
* Properties for SendMessageTask
*/
export interface CommonRunTaskProps extends stepfunctions.BasicTaskProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of expected this to be under some ecs.ts and have some ECS context in the interface name

@rix0rrr rix0rrr merged commit e4ac767 into master May 8, 2019
@rix0rrr rix0rrr deleted the huijbers/stepfunctions-service-integrations branch May 8, 2019 14:40
SanderKnape pushed a commit to SanderKnape/aws-cdk that referenced this pull request May 14, 2019
Introducing the `@aws-cdk/aws-stepfunctions-tasks` package, with service integrations for ECS, SNS and SQS. Lambda and Activity integrations have been moved there as well.

Add jest bindings for `haveResource` and `haveResourceLike`.

Allow specifying `placementConstraints` and `placementStrategies` in the constructor of `EC2Service`.

Exposing parts of `TokenMap` so that the reverse mapping of string => Token can be used by non-core libraries, if they need to embed non-literal data representation facilities into complex data structures (used in the SFN tasks to represent data that is extracted from the state JSON via a JSONPath).

BREAKING CHANGES

* If your using Lambdas or Activities in StepFunctions workflows, you must now instantiate `InvokeFunction` or `InvokeActivity` from the `@aws-cdk/aws-stepfunctions-tasks` package around it.
* `PlacementConstraint` used in task definitions is now a union object, constructed using factory functions on the `PlacementConstraint` class.
* Empty placement constraints and strategies will no longer render in the CloudFormation template. This may appear as diffs showing an empty line.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants