Skip to content

Conversation

@brikis98
Copy link
Contributor

Partially fixes #43. This adds support for calling ECS Tasks from step functions state machines:

  • Refactored task.js so that it's an "abstract" class with an invoke method that must be overridden by subclasses representing each different type of task (Lambda, Activity, ECS).
  • The existing Lambda code was moved to task-lambda.js and the existing Activity code was moved to task-activity.js.
  • Added a new task-ecs.js for this ECS work.
  • Added support for specifying the ECS endpoint and region.
  • Added a couple basic, mocked-out tests. Not sure if it's possible to do a good integration test for this, as using ECS requires either (a) an ECS cluster, which is a fair amount to spin up at test time or (b) if you're using Fargate, you need a VPC with private subnets and a NAT gateway, which is also a lot to spin up at test time.

@codecov
Copy link

codecov bot commented Jan 25, 2019

Codecov Report

Merging #44 into master will decrease coverage by 0.24%.
The diff coverage is 94.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
- Coverage   97.22%   96.98%   -0.25%     
==========================================
  Files          46       51       +5     
  Lines        1009     1094      +85     
==========================================
+ Hits          981     1061      +80     
- Misses         28       33       +5
Impacted Files Coverage Δ
src/params.js 100% <ø> (ø) ⬆️
src/server.js 93.02% <ø> (ø) ⬆️
src/constants.js 100% <ø> (ø) ⬆️
src/lib/states/state-machine.js 100% <100%> (ø) ⬆️
src/lib/states/task-lambda.js 100% <100%> (ø)
src/lib/tools/case.js 100% <100%> (ø)
src/lib/tools/sleep.js 100% <100%> (ø)
src/lib/states/task-ecs.js 87.5% <87.5%> (ø)
src/lib/states/task-activity.js 94.64% <94.64%> (ø)
src/lib/states/task.js 96.96% <94.73%> (+1.31%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14586f8...d5bb18a. Read the comment docs.

Copy link
Owner

@ChristopheBougere ChristopheBougere left a comment

Choose a reason for hiding this comment

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

Awesome work 👏👏👏
I'm just wondering if the output of a sync task shouldn't be the result of describeTask, so that it can be used later. Any guess on that?

}

async waitForEcsTaskToFinish(ecs, parameters, taskArn) {
// TODO: I've seen a history event called TaskSubmitted...

Choose a reason for hiding this comment

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

Not much documentation about events history, sadly :/
Best way to check is to have a look to a real execution in the stepfunctions console to see which one has been added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I ran it and that's where I got the list of events I have. I did see TaskSubmitted, but there was no info on when that's supposed to fire... It's probably safer to omit it and leave the TODO until we have docs on these events rather than fire it at the wrong time, but I'm open to other suggestions.

@ChristopheBougere ChristopheBougere merged commit be2aa14 into ChristopheBougere:master Jan 28, 2019
@ChristopheBougere
Copy link
Owner

Perfect!
I'll publish a new version on npm.

@brikis98
Copy link
Contributor Author

Thanks! I'll try out the new release and let you know how it works for us.

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.

Are the new Task types (e.g., ECS, SNS, etc) supported?

2 participants