From 161db8de6d593c8174538126480619551ccbc499 Mon Sep 17 00:00:00 2001 From: Hsing-Hui Hsu Date: Mon, 14 Jun 2021 11:12:52 -0700 Subject: [PATCH] refactor(aws-ecs): DRY up propagateTaskTagsFrom (#15105) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../@aws-cdk/aws-ecs/lib/base/base-service.ts | 17 +++++++++++- .../aws-ecs/lib/base/task-definition.ts | 2 +- .../@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts | 18 +------------ .../aws-ecs/lib/fargate/fargate-service.ts | 20 +------------- .../aws-ecs/test/ec2/ec2-service.test.ts | 15 ++++++----- .../test/fargate/fargate-service.test.ts | 26 ++++++++++++++++++- 6 files changed, 52 insertions(+), 46 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts index d4023a4df6f9e..c33718905c5bb 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts @@ -159,6 +159,15 @@ export interface BaseServiceOptions { */ readonly propagateTags?: PropagatedTagSource; + /** + * Specifies whether to propagate the tags from the task definition or the service to the tasks in the service. + * Tags can only be propagated to the tasks within the service during service creation. + * + * @deprecated Use `propagateTags` instead. + * @default PropagatedTagSource.NONE + */ + readonly propagateTaskTagsFrom?: PropagatedTagSource; + /** * Specifies whether to enable Amazon ECS managed tags for the tasks within the service. For more information, see * [Tagging Your Amazon ECS Resources](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-using-tags.html) @@ -373,6 +382,10 @@ export abstract class BaseService extends Resource physicalName: props.serviceName, }); + if (props.propagateTags && props.propagateTaskTagsFrom) { + throw new Error('You can only specify either propagateTags or propagateTaskTagsFrom. Alternatively, you can leave both blank'); + } + this.taskDefinition = taskDefinition; // launchType will set to undefined if using external DeploymentController or capacityProviderStrategies @@ -380,6 +393,8 @@ export abstract class BaseService extends Resource props.capacityProviderStrategies !== undefined ? undefined : props.launchType; + const propagateTagsFromSource = props.propagateTaskTagsFrom ?? props.propagateTags ?? PropagatedTagSource.NONE; + this.resource = new CfnService(this, 'Service', { desiredCount: props.desiredCount, serviceName: this.physicalName, @@ -392,7 +407,7 @@ export abstract class BaseService extends Resource rollback: props.circuitBreaker.rollback ?? false, } : undefined, }, - propagateTags: props.propagateTags === PropagatedTagSource.NONE ? undefined : props.propagateTags, + propagateTags: propagateTagsFromSource === PropagatedTagSource.NONE ? undefined : props.propagateTags, enableEcsManagedTags: props.enableECSManagedTags ?? false, deploymentController: props.circuitBreaker ? { type: DeploymentControllerType.ECS, diff --git a/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts b/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts index 0cc89d633160f..2889c4d94df3d 100644 --- a/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts +++ b/packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts @@ -584,7 +584,7 @@ export class TaskDefinition extends TaskDefinitionBase { } /** - * Adds the specified extention to the task definition. + * Adds the specified extension to the task definition. * * Extension can be used to apply a packaged modification to * a task definition. diff --git a/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts b/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts index 4cf4de8a83292..df2ad7819543c 100644 --- a/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts @@ -1,7 +1,7 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import { Lazy, Resource, Stack } from '@aws-cdk/core'; import { Construct } from 'constructs'; -import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType, PropagatedTagSource } from '../base/base-service'; +import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType } from '../base/base-service'; import { fromServiceAtrributes } from '../base/from-service-attributes'; import { NetworkMode, TaskDefinition } from '../base/task-definition'; import { ICluster } from '../cluster'; @@ -82,15 +82,6 @@ export interface Ec2ServiceProps extends BaseServiceOptions { * @default false */ readonly daemon?: boolean; - - /** - * Specifies whether to propagate the tags from the task definition or the service to the tasks in the service. - * Tags can only be propagated to the tasks within the service during service creation. - * - * @deprecated Use `propagateTags` instead. - * @default PropagatedTagSource.NONE - */ - readonly propagateTaskTagsFrom?: PropagatedTagSource; } /** @@ -173,23 +164,16 @@ export class Ec2Service extends BaseService implements IEc2Service { throw new Error('Supplied TaskDefinition is not configured for compatibility with EC2'); } - if (props.propagateTags && props.propagateTaskTagsFrom) { - throw new Error('You can only specify either propagateTags or propagateTaskTagsFrom. Alternatively, you can leave both blank'); - } - if (props.securityGroup !== undefined && props.securityGroups !== undefined) { throw new Error('Only one of SecurityGroup or SecurityGroups can be populated.'); } - const propagateTagsFromSource = props.propagateTaskTagsFrom ?? props.propagateTags ?? PropagatedTagSource.NONE; - super(scope, id, { ...props, desiredCount: props.desiredCount, maxHealthyPercent: props.daemon && props.maxHealthyPercent === undefined ? 100 : props.maxHealthyPercent, minHealthyPercent: props.daemon && props.minHealthyPercent === undefined ? 0 : props.minHealthyPercent, launchType: LaunchType.EC2, - propagateTags: propagateTagsFromSource, enableECSManagedTags: props.enableECSManagedTags, }, { diff --git a/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts b/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts index 793fb633e83d0..7979b98f0b0fa 100644 --- a/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts +++ b/packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts @@ -1,7 +1,7 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import * as cdk from '@aws-cdk/core'; import { Construct } from 'constructs'; -import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType, PropagatedTagSource } from '../base/base-service'; +import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType } from '../base/base-service'; import { fromServiceAtrributes } from '../base/from-service-attributes'; import { TaskDefinition } from '../base/task-definition'; import { ICluster } from '../cluster'; @@ -58,16 +58,6 @@ export interface FargateServiceProps extends BaseServiceOptions { * @default Latest */ readonly platformVersion?: FargatePlatformVersion; - - /** - * Specifies whether to propagate the tags from the task definition or the service to the tasks in the service. - * Tags can only be propagated to the tasks within the service during service creation. - * - * @deprecated Use `propagateTags` instead. - * @default PropagatedTagSource.NONE - */ - readonly propagateTaskTagsFrom?: PropagatedTagSource; - } /** @@ -134,10 +124,6 @@ export class FargateService extends BaseService implements IFargateService { throw new Error('Supplied TaskDefinition is not configured for compatibility with Fargate'); } - if (props.propagateTags && props.propagateTaskTagsFrom) { - throw new Error('You can only specify either propagateTags or propagateTaskTagsFrom. Alternatively, you can leave both blank'); - } - if (props.securityGroup !== undefined && props.securityGroups !== undefined) { throw new Error('Only one of SecurityGroup or SecurityGroups can be populated.'); } @@ -147,15 +133,11 @@ export class FargateService extends BaseService implements IFargateService { && SECRET_JSON_FIELD_UNSUPPORTED_PLATFORM_VERSIONS.includes(props.platformVersion)) { throw new Error(`The task definition of this service uses at least one container that references a secret JSON field. This feature requires platform version ${FargatePlatformVersion.VERSION1_4} or later.`); } - - const propagateTagsFromSource = props.propagateTaskTagsFrom ?? props.propagateTags ?? PropagatedTagSource.NONE; - super(scope, id, { ...props, desiredCount: props.desiredCount, launchType: LaunchType.FARGATE, capacityProviderStrategies: props.capacityProviderStrategies, - propagateTags: propagateTagsFromSource, enableECSManagedTags: props.enableECSManagedTags, }, { cluster: props.cluster.clusterName, diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts b/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts index f53f66d8755ad..7e942c866b213 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts @@ -1859,13 +1859,14 @@ nodeunitShim({ memoryLimitMiB: 512, }); - test.throws(() => new ecs.Ec2Service(stack, 'Ec2Service', { - cluster, - taskDefinition, - propagateTags: PropagatedTagSource.SERVICE, - propagateTaskTagsFrom: PropagatedTagSource.SERVICE, - })); - + test.throws(() => { + new ecs.Ec2Service(stack, 'Ec2Service', { + cluster, + taskDefinition, + propagateTags: PropagatedTagSource.SERVICE, + propagateTaskTagsFrom: PropagatedTagSource.SERVICE, + }); + }, /You can only specify either propagateTags or propagateTaskTagsFrom. Alternatively, you can leave both blank/); test.done(); }, diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts b/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts index f1fdbedc01064..f7c33eb7b8af9 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts @@ -11,7 +11,7 @@ import * as cloudmap from '@aws-cdk/aws-servicediscovery'; import * as cdk from '@aws-cdk/core'; import { nodeunitShim, Test } from 'nodeunit-shim'; import * as ecs from '../../lib'; -import { DeploymentControllerType, LaunchType } from '../../lib/base/base-service'; +import { DeploymentControllerType, LaunchType, PropagatedTagSource } from '../../lib/base/base-service'; nodeunitShim({ 'When creating a Fargate Service': { @@ -2948,5 +2948,29 @@ nodeunitShim({ test.done(); }, + + 'with both propagateTags and propagateTaskTagsFrom defined'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const vpc = new ec2.Vpc(stack, 'MyVpc', {}); + const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc }); + const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef'); + + taskDefinition.addContainer('web', { + image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'), + memoryLimitMiB: 512, + }); + + // THEN + test.throws(() => { + new ecs.FargateService(stack, 'FargateService', { + cluster, + taskDefinition, + propagateTags: PropagatedTagSource.SERVICE, + propagateTaskTagsFrom: PropagatedTagSource.SERVICE, + }); + }, /You can only specify either propagateTags or propagateTaskTagsFrom. Alternatively, you can leave both blank/); + test.done(); + }, }, });