feat(stepfunctions-tasks): task construct to call RunJob on ECS

[shivlaks]

Replacement for the current implementation of ECS service integration.
Merges state and service integration properties and represents them as a
construct.

The notable differences from the current implementation are:

• single implementation classrunTask with an added launchTarget required
  property where the properties specific to the launch type are specified
• linter disable of ref-via-interface as we need to use TaskDefinition
  because properties that are not known for imported task definitions are required
• older implementations have been marked deprecated with direction to the
  replacement.

Left all of the unit tests (except fargate platform version which was not
previously honored) and integ test expectations verbatim to ensure that
we have not lost fidelity.

BREAKING CHANGE:

• containerName is not supported as an override anymore and has been replaced by containerDefinition

Closes #8610

---

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

[MrArnoldPalmer]

Couple of small nits but the APIs feel right to me. As far as bind vs base class I would lean towards bind as well.

addressed changes

[MrArnoldPalmer]

Changes lgtm. Will leave approval to @nija-at so it doesn’t auto merge before he gets a second look.

[nija-at]

Take a look at the comment around bind() signature and couple of comments around security groups. These two need to be addressed, a few around code simplification, the rest are just slight adjustments.

[nija-at]

All this can be dropped in favour of -

taskDefinition.containers.includes(override.containerDefinition)

[shivlaks]

I thought so too, but containers is a protected property and not public

[mb-dev]

Wait, launchType is now optional. and if omitted the default capacity provider is used:
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RunTask.html#ECS-RunTask-request-launchType

capacity providers are coming to cloudformation soon. should this change be compatible and not assume launch type is EC2 or Fargate?

[shivlaks]

@mb-dev - going from required to optional is not a breaking change. My initial instinct is that we can introduce that change when it's more fleshed out or in a follow-on PR.

Is there a use case this would preclude today? In that case, I'd push for implementing it (probably in a follow-up PR as well)

[shivlaks]

@mb-dev - just saw that you'd already created #7967 a while back. I'll create a PR to introduce that change. I'd like to keep the scope of this PR contained so we can iterate and keep things moving.

I don't believe capacityProviderStrategy is currently modeled in runTask and we'd need to do that ahead of making launchType optional

[mb-dev]

@shivlaks what alarmed me is that the PR makes the definition depend more on launch type than before. launchTarget sounds like launchType, and it's tied to either EC2/Fargate. I am not sure if it will be clear how to express a undefined launchType while still specifying parameters to be used at creation.

Currently no launchType might look like:

const runTask = new tasks.EcsRunTask(stack, 'Run', {
    integrationPattern: sfn.IntegrationPattern.RUN_JOB,
    cluster,
This conversation was marked as resolved by shivlaks
    taskDefinition,
    launchTarget: new tasks.EcsEc2LaunchTarget({
      launchType: undefined,
    }),
  });

If launchTarget would be named something like clusterConfiguration it might be easier to parse.

Then again, I realize I am late in commenting - this PR is pretty far along. If you have an idea for a clear syntax that can be introduced in a follow up PR, that will be great. Thanks!

[mergify]

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).

[shivlaks]

If launchTarget would be named something like clusterConfiguration it might be easier to parse.

Then again, I realize I am late in commenting - this PR is pretty far along. If you have an idea for a clear syntax that can be introduced in a follow up PR, that will be great. Thanks!

That's a fair point, let's continue this conversation in #7967 and get ahead of the potential usability issue / user confusion

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

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).