feat(ecs): container dependencies #3032

Don-CA · 2019-06-24T12:32:19Z

Add new addContainerDependencies method to allow for container dependencies
Fixes #2490

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
Docs
- jsdocs: All public APIs documented
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
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"

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

cohalz · 2019-07-09T02:37:57Z

👍

piradeepk

Thanks for taking the time to make these changes and I'm sorry for the delay in reviewing the change. Overall everything looks great, just had a couple comments!

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-task-definition.ts

packages/@aws-cdk/aws-apigateway/package-lock.json

piradeepk

@Don-CA, Thanks for the hard work and for addressing this issue, it's really appreciated!

LGTM! @rix0rrr please review and merge

rix0rrr · 2019-07-18T14:50:24Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+  /**
+   * This method adds one or more container dependencies to the container.
+   */
+  public addContainerDependencies(...containerDependencies: ContainerDependency[]) {


Please also add an optional array property to the Props object to initialize these.

See my comment below, I'm keeping it consistent with similar properties but open to refactoring all the properties to output cleaner cloudformation.

rix0rrr · 2019-07-18T14:50:39Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

@@ -418,6 +430,7 @@ export class ContainerDefinition extends cdk.Construct {
      command: this.props.command,
      cpu: this.props.cpu,
      disableNetworking: this.props.disableNetworking,
+      dependsOn: this.containerDependencies.map(renderContainerDependency),


Please don't render the property at all if the list is empty.

I was unsure about this. Rendering an empty list keeps in consistent with Links, MountPoints and VolumesFrom however it results in a noisy change to the tests.
I'm happy to make it different to the other three and then do further PR to make the other three not output if their list is empty, would you be open to that?

Or, leave it as is, and refactor all four as a separate PR?

We prefer not to render empty arrays/objects. Would be great to fix the others as well...

Ok, cool. I can fix the others as well as part of this PR.

eladb

A few notes

eladb · 2019-07-21T10:48:38Z

packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecs-deploy.expected.json

@@ -230,6 +230,7 @@
      "Properties": {
        "ContainerDefinitions": [
          {
+            "DependsOn": [],


Preferably don't render if the array is empty

eladb · 2019-07-21T10:49:51Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+  /**
+   * An array dependencies defined for container startup and shutdown.
+   */
+  public readonly containerDependencies = Array<ContainerDependency>();


I am only now realizing that all of these are exposing mutable arrays to users... Not ideal. @rix0rrr what do you think?

eladb · 2019-07-21T10:50:32Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

@@ -418,6 +430,7 @@ export class ContainerDefinition extends cdk.Construct {
      command: this.props.command,
      cpu: this.props.cpu,
      disableNetworking: this.props.disableNetworking,
+      dependsOn: this.containerDependencies.map(renderContainerDependency),


We prefer not to render empty arrays/objects. Would be great to fix the others as well...

eladb · 2019-07-21T10:50:49Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+  /**
+   * The container to depend on.
+   */
+  readonly container: ContainerDefinition,


Use ; for EOL

eladb · 2019-07-21T10:51:24Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+   * The dependency condition of the container. Valid values are ContainerDependencyCondition.START, ContainerDependencyCondition.COMPLETE,
+   * ContainerDependencyCondition.SUCCESS and ContainerDependencyCondition.HEALTHY.
+   *
+   * @default - ContainerDependencyCondition.HEALTHY


We only use - when it's not a "real" value. In your case you are actually providing a value...

eladb · 2019-07-21T10:51:38Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+   */
+  readonly container: ContainerDefinition,
+  /**
+   * The dependency condition of the container. Valid values are ContainerDependencyCondition.START, ContainerDependencyCondition.COMPLETE,


What is the dependency condition? Can you copy & paste from AWS docs?

This is a copy & paste from the docs :) Added some more of my own context.

eladb · 2019-07-23T11:30:02Z

See some review comments

piradeepk

Can you add tests to ensure that MountPoints, Links, etc still continue to render correctly if populated?

Don-CA · 2019-07-30T02:14:22Z

Can you add tests to ensure that MountPoints, Links, etc still continue to render correctly if populated?

Added tests for VolumesFrom and Links.
There are already existing tests for the rest:

piradeepk

Thanks for adding the tests! @eladb please review

eladb

I really don’t like it that we expose all these public arrays. That’s an anti pattern because it exposes an internal mutable structure

eladb · 2019-07-31T09:33:27Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

@@ -469,15 +482,15 @@ export class ContainerDefinition extends cdk.Construct {
      image: this.imageConfig.imageName,
      memory: this.props.memoryLimitMiB,
      memoryReservation: this.props.memoryReservationMiB,


Shouldn’t these be Lazy? What happens if you add something to the array after initialization?

eladb · 2019-07-31T09:35:20Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+
+function renderContainerDependency(containerDependency: ContainerDependency): CfnTaskDefinition.ContainerDependencyProperty {
+  return {
+    containerName: containerDependency.container.node.id,


If the name of the container is similar to the node ID that’s a detail that needs to be encapsulated behind a container.containerName property

Would this also apply to the reference to container.node.id in addLink and in validate() in TaskDefinition?

Yes please!

Don-CA · 2019-07-31T23:52:05Z

I really don’t like it that we expose all these public arrays. That’s an anti pattern because it exposes an internal mutable structure

I took all those arrays private and all the tests still ran, what do you think of doing that? It would create some breaking changes, would that be acceptable?

eladb · 2019-08-01T07:55:17Z

I don't think it will be acceptable to break this public API sadly. What we can do is mark them as @deprecated so people don't rely on them and we will get rid of it in v2.0 sometime in the future.

eladb · 2019-08-01T12:31:12Z

Apologies for the confusion... I was just venting ;-)

Add new addContainerDependency method to allow for container dependencies Fixes #2490

Don-CA · 2019-08-03T05:37:01Z

Apologies for the confusion... I was just venting ;-)

No worries, reverted those changes :)

Add new addContainerDependencies method to allow for container dependencies Fixes #2490

* chore: update package-lock.json * feat(eks): define kubernetes resources This change allows defining arbitrary Kubernetes resources within an EKS cluster. * nice! * update readme * Update README.md * feat(events): ability to add cross-account targets (#3323) This adds the capability of adding a target to an event rule that belongs to a different account than the rule itself. Required for things like cross-account CodePipelines with source actions triggered by events. * chore(ci): add mergify config file (#3502) * chore: update jsii to 0.14.3 (#3513) * fix(iam): correctly limit the default PolicyName to 128 characters (#3487) Our logic for trimming the length of the default IAM policy name was not working, as it wasn't updated when logicalId became a Token rather than a literate string, and so it was never actually triggered (it just checked that the display name of the Token was less than 128 characters, which it always is). The fix is to resolve the logical ID Token before applying the trimming logic. Fixes #3402 * v1.3.0 (#3516) See CHANGELOG * fix: typo in restapi.ts (#3530) * feat(ecs): container dependencies (#3032) Add new addContainerDependencies method to allow for container dependencies Fixes #2490 * feat(s3-deployment): CloudFront invalidation (#3213) see #3106 * docs(core): findChild gets direct child only (#3512) * doc(iam): update references to addManagedPolicy (#3511) * fix(sqs): do not emit grants to the AWS-managed encryption key (#3169) Grants on the `alias/aws/sqs` KMS key alias are not necessary since the key will implicitly allow for it's intended usage to be fulfilled (as opposed to how you have to manage grants yourself when using a user-managed key instead). This removes the statement that was generated using an invalid resource entry. Fixes #2794 * fix(lambda): allow ArnPrincipal in grantInvoke (#3501) Fixes #3264 I'm trying to allow a lambda function in another account to be able to invoke my CDK generated lambda function. This works through the CLI like so: aws lambda add-permission --function-name=myFunction --statement-id=ABoldStatement --action=lambda:InvokeFunction --principal=arn:aws:iam::{account_id}:role/a_lambda_execution_role But CDK doesn't seem to allow me to add an ArnPrincipal doing something like this: myFunction.grantInvoke(new iam.ArnPrincipal(props.myARN)) With the error: Invalid principal type for Lambda permission statement: ArnPrincipal. Supported: AccountPrincipal, ServicePrincipal This PR allows ArnPrincipal to be passed to lambda.grantInvoke. There might be some additional validation required on the exact ARN as I believe only some ARNs are supported by lambda add-permission * chore(contrib): remove API stabilization disclaimer * fix(ssm): add GetParameters action to grantRead() (#3546) * misc * rename `KubernetesManifest` to `KubernetesResource` and `addResource` * move AWS Auth APIs to `cluster.awsAuth` and expose `AwsAuth` * remove the yaml library (we can just use a JSON stream) * add support for adding accounts to aws-auth * fix cluster deletion bug * move kubctl app info to constants * addManifest => addResource * update test expectations * add unit test for customresrouce.ref * fix sample link

Don-CA requested review from SoManyHs and a team as code owners June 24, 2019 12:32

Don-CA requested review from RomainMuller and skinny85 as code owners June 25, 2019 04:22

piradeepk requested changes Jul 12, 2019

View reviewed changes

Don-CA requested a review from a team as a code owner July 17, 2019 12:39

piradeepk approved these changes Jul 18, 2019

View reviewed changes

rix0rrr approved these changes Jul 18, 2019

View reviewed changes

rix0rrr reviewed Jul 18, 2019

View reviewed changes

eladb reviewed Jul 21, 2019

View reviewed changes

piradeepk requested changes Jul 29, 2019

View reviewed changes

piradeepk approved these changes Jul 30, 2019

View reviewed changes

eladb reviewed Jul 31, 2019

View reviewed changes

NGL321 added the pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason. label Aug 1, 2019

NGL321 assigned piradeepk and SoManyHs Aug 1, 2019

Don-CA and others added 6 commits August 2, 2019 09:07

feat(ecs): add support for container dependencies

ec3e385

Add new addContainerDependency method to allow for container dependencies Fixes #2490

add tests for Links and VolumesFrom

c5178c0

fix tests after rebase

2867ce1

Lazy evals, containerNames and making lists private

0d3d73a

reference Lazy through core

f65a6ee

revert making the lists private

b870d57

eladb changed the title ~~feat(ecs): add support for container dependencies~~ feat(ecs): container dependencies Aug 5, 2019

eladb merged commit 56656e0 into aws:master Aug 5, 2019

eladb pushed a commit that referenced this pull request Aug 6, 2019

feat(ecs): container dependencies (#3032)

1ae4c13

Add new addContainerDependencies method to allow for container dependencies Fixes #2490

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): container dependencies #3032

feat(ecs): container dependencies #3032

Don-CA commented Jun 24, 2019 •

edited

Loading

cohalz commented Jul 9, 2019

piradeepk left a comment •

edited

Loading

piradeepk left a comment

rix0rrr Jul 18, 2019

Don-CA Jul 18, 2019 •

edited

Loading

rix0rrr Jul 18, 2019

Don-CA Jul 18, 2019 •

edited

Loading

Don-CA Jul 18, 2019 •

edited

Loading

eladb Jul 21, 2019

Don-CA Jul 21, 2019

eladb left a comment

eladb Jul 21, 2019

eladb Jul 21, 2019

eladb Jul 21, 2019

eladb Jul 21, 2019

eladb Jul 21, 2019

eladb Jul 21, 2019

Don-CA Jul 24, 2019

eladb commented Jul 23, 2019

piradeepk left a comment

Don-CA commented Jul 30, 2019 •

edited

Loading

piradeepk left a comment

eladb left a comment

eladb Jul 31, 2019

eladb Jul 31, 2019

Don-CA Jul 31, 2019 •

edited

Loading

eladb Jul 31, 2019

Don-CA commented Jul 31, 2019

eladb commented Aug 1, 2019

eladb commented Aug 1, 2019

Don-CA commented Aug 3, 2019

feat(ecs): container dependencies #3032

feat(ecs): container dependencies #3032

Conversation

Don-CA commented Jun 24, 2019 • edited Loading

cohalz commented Jul 9, 2019

piradeepk left a comment • edited Loading

Choose a reason for hiding this comment

piradeepk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-CA Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-CA Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Don-CA Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Jul 23, 2019

piradeepk left a comment

Choose a reason for hiding this comment

Don-CA commented Jul 30, 2019 • edited Loading

piradeepk left a comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-CA Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Don-CA commented Jul 31, 2019

eladb commented Aug 1, 2019

eladb commented Aug 1, 2019

Don-CA commented Aug 3, 2019

Don-CA commented Jun 24, 2019 •

edited

Loading

piradeepk left a comment •

edited

Loading

Don-CA Jul 18, 2019 •

edited

Loading

Don-CA Jul 18, 2019 •

edited

Loading

Don-CA Jul 18, 2019 •

edited

Loading

Don-CA commented Jul 30, 2019 •

edited

Loading

Don-CA Jul 31, 2019 •

edited

Loading