Repository: pulumi/pulumi-awsx
Pull request #1043 (2023-07-17 08:47:42)

Correctly set single ECS container name
Fixes https://github.com/pulumi/pulumi-awsx/issues/882

Using `container.name`, which is an `Output`, required output-ifying that code path.

thomas11: > This is a big change to not have any tests. Would it be possible to add some?
> 
> I'm not super familiar with aws itself, so I'm hesitant to approve something that I can't check. Others may feel differently.

@iwahbe I did add tests for the functionality I fixed, in `awsx/ecs/container.test.ts`. Granted, coverage for other code paths is still low to none.
iwahbe: > > This is a big change to not have any tests. Would it be possible to add some?
> > I'm not super familiar with aws itself, so I'm hesitant to approve something that I can't check. Others may feel differently.
> 
> @iwahbe I did add tests for the functionality I fixed, in `awsx/ecs/container.test.ts`. Granted, coverage for other code paths is still low to none.

My bad, I totally missed that. I was looking for files that had test in the name, not the extension.

Patch:
diff --git a/awsx/ecs/containers.ts b/awsx/ecs/containers.ts
index 1bf055ab..7cdb3680 100644
--- a/awsx/ecs/containers.ts
+++ b/awsx/ecs/containers.ts
@@ -18,21 +18,42 @@ import { LogGroupId } from "../cloudwatch/logGroup";
 import * as schema from "../schema-types";
 import * as utils from "../utils";
 
+/** @internal */
+export function normalizeTaskDefinitionContainers(
+  args: schema.FargateTaskDefinitionArgs | schema.EC2TaskDefinitionArgs,
+): pulumi.Output<Record<string, schema.TaskDefinitionContainerDefinitionInputs>> {
+  const { container, containers } = args;
+  if (containers !== undefined && container === undefined) {
+    // Wrapping in Output is not necessary here but it is in the following case, so we do it here,
+    // too, to simplify the return type.
+    return pulumi.output(containers);
+  } else if (container !== undefined && containers === undefined) {
+    return pulumi.output(container.name).apply((n) => {
+      const name = n ?? "container";
+      const rec: Record<string, schema.TaskDefinitionContainerDefinitionInputs> = {
+        [name]: container,
+      };
+      return rec;
+    });
+  } else {
+    throw new Error("Exactly one of [container] or [containers] must be provided");
+  }
+}
+
 /** @internal */
 export function computeContainerDefinitions(
   parent: pulumi.Resource,
-  containers: Record<string, schema.TaskDefinitionContainerDefinitionInputs>,
+  containers: pulumi.Output<Record<string, schema.TaskDefinitionContainerDefinitionInputs>>,
   logGroupId: pulumi.Input<LogGroupId> | undefined,
 ): pulumi.Output<schema.TaskDefinitionContainerDefinitionInputs[]> {
-  const result: pulumi.Output<schema.TaskDefinitionContainerDefinitionInputs>[] = [];
-
-  for (const containerName of Object.keys(containers)) {
-    const container = containers[containerName];
-
-    result.push(computeContainerDefinition(parent, containerName, container, logGroupId));
-  }
-
-  return pulumi.all(result);
+  const computed = containers.apply((c) => {
+    return pulumi.all(
+      Object.entries(c).map(([containerName, container]) =>
+        computeContainerDefinition(parent, containerName, container, logGroupId),
+      ),
+    );
+  });
+  return computed;
 }
 
 function computeContainerDefinition(
@@ -43,7 +64,7 @@ function computeContainerDefinition(
 ): pulumi.Output<schema.TaskDefinitionContainerDefinitionInputs> {
   const resolvedMappings = container.portMappings
     ? pulumi.output(container.portMappings).apply((portMappings) =>
-        portMappings.map((mappingInput) => {
+        portMappings?.map((mappingInput) => {
           return pulumi
             .output(mappingInput.targetGroup?.port)
             .apply((tgPort): schema.TaskDefinitionPortMappingInputs => {
@@ -52,10 +73,10 @@ function computeContainerDefinition(
         }),
       )
     : undefined;
-  const region = utils.getRegion(parent);
+
   return pulumi
-    .all([container, resolvedMappings, region, logGroupId])
-    .apply(([container, portMappings, region, logGroupId]) => {
+    .all([container, resolvedMappings, logGroupId])
+    .apply(([container, portMappings, logGroupId]) => {
       const containerDefinition = {
         ...container,
         portMappings,
@@ -88,10 +109,10 @@ export function getMappingInputs(
 
 /** @internal */
 export function computeLoadBalancers(
-  containers: Record<string, schema.TaskDefinitionContainerDefinitionInputs>,
+  containers: pulumi.Output<Record<string, schema.TaskDefinitionContainerDefinitionInputs>>,
 ): pulumi.Output<aws.types.output.ecs.ServiceLoadBalancer[]> {
-  const mappedContainers = Object.entries(containers).map(
-    ([containerName, containerDefinition]) => {
+  const mappedContainers = containers.apply((conts) => {
+    return Object.entries(conts).map(([containerName, containerDefinition]) => {
       const portMappings:
         | pulumi.Input<pulumi.Input<schema.TaskDefinitionPortMappingInputs>[]>
         | undefined = containerDefinition.portMappings;
@@ -109,30 +130,32 @@ export function computeLoadBalancers(
         });
       });
       return mappedMappings;
-    },
-  );
-  return pulumi.all(mappedContainers).apply((containerGroups) =>
-    utils.collect(containerGroups, (cg) => {
-      if (cg === undefined) {
-        return [];
-      }
-      return utils.choose(
-        cg,
-        ({
-          containerName,
-          tgArn,
-          tgPort,
-        }): aws.types.output.ecs.ServiceLoadBalancer | undefined => {
-          if (tgArn === undefined || tgPort === undefined) {
-            return undefined;
-          }
-          return {
+    });
+  });
+  return mappedContainers.apply((mapped) => {
+    return pulumi.all(mapped).apply((containerGroups) =>
+      utils.collect(containerGroups, (cg) => {
+        if (cg === undefined) {
+          return [];
+        }
+        return utils.choose(
+          cg,
+          ({
             containerName,
-            containerPort: tgPort,
-            targetGroupArn: tgArn,
-          };
-        },
-      );
-    }),
-  );
+            tgArn,
+            tgPort,
+          }): aws.types.output.ecs.ServiceLoadBalancer | undefined => {
+            if (tgArn === undefined || tgPort === undefined) {
+              return undefined;
+            }
+            return {
+              containerName,
+              containerPort: tgPort,
+              targetGroupArn: tgArn,
+            };
+          },
+        );
+      }),
+    );
+  });
 }
diff --git a/awsx/ecs/ec2TaskDefinition.ts b/awsx/ecs/ec2TaskDefinition.ts
index 6ff00d93..f8961e7d 100644
--- a/awsx/ecs/ec2TaskDefinition.ts
+++ b/awsx/ecs/ec2TaskDefinition.ts
@@ -18,7 +18,11 @@ import { defaultLogGroup } from "../cloudwatch/logGroup";
 import * as role from "../role";
 import * as schema from "../schema-types";
 import * as utils from "../utils";
-import { computeContainerDefinitions, computeLoadBalancers } from "./containers";
+import {
+  normalizeTaskDefinitionContainers,
+  computeContainerDefinitions,
+  computeLoadBalancers,
+} from "./containers";
 
 /**
  * Create a TaskDefinition resource with the given unique name, arguments, and options.
@@ -51,7 +55,7 @@ export class EC2TaskDefinition extends schema.EC2TaskDefinition {
       },
     );
 
-    const containers = normalizeEC2TaskDefinitionContainers(args);
+    const containers = normalizeTaskDefinitionContainers(args);
 
     const { logGroup, logGroupId } = defaultLogGroup(name, args.logGroup, {}, { parent: this });
     this.logGroup = logGroup;
@@ -95,17 +99,6 @@ export class EC2TaskDefinition extends schema.EC2TaskDefinition {
   }
 }
 
-function normalizeEC2TaskDefinitionContainers(args: schema.EC2TaskDefinitionArgs) {
-  const { container, containers } = args;
-  if (containers !== undefined && container === undefined) {
-    return containers;
-  } else if (container !== undefined && containers === undefined) {
-    return { container: container };
-  } else {
-    throw new Error("Exactly one of [container] or [containers] must be provided");
-  }
-}
-
 function buildTaskDefinitionArgs(
   name: string,
   args: schema.EC2TaskDefinitionArgs,
diff --git a/awsx/ecs/fargateTaskDefinition.ts b/awsx/ecs/fargateTaskDefinition.ts
index 17e6a2f7..bf721c1d 100644
--- a/awsx/ecs/fargateTaskDefinition.ts
+++ b/awsx/ecs/fargateTaskDefinition.ts
@@ -18,7 +18,11 @@ import { defaultLogGroup } from "../cloudwatch/logGroup";
 import * as role from "../role";
 import * as schema from "../schema-types";
 import * as utils from "../utils";
-import { computeContainerDefinitions, computeLoadBalancers } from "./containers";
+import {
+  normalizeTaskDefinitionContainers,
+  computeContainerDefinitions,
+  computeLoadBalancers,
+} from "./containers";
 import { calculateFargateMemoryAndCPU } from "./fargateMemoryAndCpu";
 
 /**
@@ -52,7 +56,7 @@ export class FargateTaskDefinition extends schema.FargateTaskDefinition {
       },
     );
 
-    const containers = normalizeFargateTaskDefinitionContainers(args);
+    const containers = normalizeTaskDefinitionContainers(args);
 
     const { logGroup, logGroupId } = defaultLogGroup(name, args.logGroup, {}, { parent: this });
     this.logGroup = logGroup;
@@ -96,12 +100,20 @@ export class FargateTaskDefinition extends schema.FargateTaskDefinition {
   }
 }
 
-function normalizeFargateTaskDefinitionContainers(args: schema.FargateTaskDefinitionArgs) {
+export function normalizeFargateTaskDefinitionContainers(args: schema.FargateTaskDefinitionArgs) {
   const { container, containers } = args;
   if (containers !== undefined && container === undefined) {
-    return containers;
+    // Wrapping in Output is not necessary here but it is in the following case, so we do it here,
+    // too, to simplify the return type.
+    return pulumi.output(containers);
   } else if (container !== undefined && containers === undefined) {
-    return { container: container };
+    const name = container.name ?? "container";
+    return pulumi.output(name).apply((n) => {
+      const rec: Record<string, schema.TaskDefinitionContainerDefinitionInputs> = {
+        [n]: container,
+      };
+      return rec;
+    });
   } else {
     throw new Error("Exactly one of [container] or [containers] must be provided");
   }
diff --git a/examples/ecs/capacity-provider-service-with-cluster-default-strategies/index.ts b/examples/ecs/capacity-provider-service-with-cluster-default-strategies/index.ts
index 76f0dae2..8853f7f6 100644
--- a/examples/ecs/capacity-provider-service-with-cluster-default-strategies/index.ts
+++ b/examples/ecs/capacity-provider-service-with-cluster-default-strategies/index.ts
@@ -14,7 +14,7 @@ const cluster = new classic.ecs.Cluster("cluster", {
     ],
 });
 
-// // Create a load balancer on port 80 and spin up two instances of Nginx.
+// Create a load balancer on port 80 and spin up two instances of Nginx.
 const lb = new classic.lb.ApplicationListener("nginx-lb", { port: 80 });
 const targetGroup = lb.defaultTargetGroup!.targetGroup;
 
diff --git a/examples/ecs/capacity-provider-service-with-cluster-default-strategies/package.json b/examples/ecs/capacity-provider-service-with-cluster-default-strategies/package.json
index a2deaf72..32fc8a82 100644
--- a/examples/ecs/capacity-provider-service-with-cluster-default-strategies/package.json
+++ b/examples/ecs/capacity-provider-service-with-cluster-default-strategies/package.json
@@ -5,6 +5,6 @@
     },
     "dependencies": {
         "@pulumi/pulumi": "^3.0.0",
-        "@pulumi/aws": "^4.0.0"
+        "@pulumi/aws": "^5.0.0"
     }
 }


Test patch:
diff --git a/awsx/ecs/container.test.ts b/awsx/ecs/container.test.ts
index 2bc71fab..793665c7 100644
--- a/awsx/ecs/container.test.ts
+++ b/awsx/ecs/container.test.ts
@@ -11,7 +11,9 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-import { getMappingInputs } from "./containers";
+import * as pulumi from "@pulumi/pulumi";
+
+import { normalizeTaskDefinitionContainers, getMappingInputs } from "./containers";
 
 describe("port mappings", () => {
   it.each`
@@ -31,3 +33,34 @@ describe("port mappings", () => {
     },
   );
 });
+
+function promiseOf<T>(output: pulumi.Output<T>): Promise<T> {
+  return new Promise((resolve) => output.apply(resolve));
+}
+
+describe("container naming for single container", () => {
+  it("single container with explicit name", async () => {
+    const args = {
+      container: {
+        name: "myTestName",
+        cpu: 16,
+      },
+    };
+    const normalized = normalizeTaskDefinitionContainers(args);
+    const n = await promiseOf(normalized);
+    expect(n.myTestName).toBeDefined();
+    expect(n.container).toBeUndefined();
+  });
+
+  it("single container without name", async () => {
+    const args = {
+      container: {
+        cpu: 16,
+      },
+    };
+    const normalized = normalizeTaskDefinitionContainers(args);
+    const n = await promiseOf(normalized);
+    // "container" is the default name
+    expect(n.container).toBeDefined();
+  });
+});