Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ecs): rename service logical id (under feature flag) #6348

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
import * as iam from '@aws-cdk/aws-iam';
import * as cloudmap from '@aws-cdk/aws-servicediscovery';
import { Construct, Duration, IResolvable, IResource, Lazy, Resource, Stack } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { LoadBalancerTargetOptions, NetworkMode, TaskDefinition } from '../base/task-definition';
import { ICluster } from '../cluster';
import { Protocol } from '../container-definition';
Expand Down Expand Up @@ -318,7 +319,11 @@ export abstract class BaseService extends Resource

this.taskDefinition = taskDefinition;

this.resource = new CfnService(this, "Service", {
const cfnServiceId = this.node.tryGetContext(cxapi.ENABLE_CFN_SERVICE_RESOURCE_RENAME)
? "Resource"
: "Service";

this.resource = new CfnService(this, cfnServiceId, {
desiredCount: props.desiredCount,
serviceName: this.physicalName,
loadBalancers: Lazy.anyValue({ produce: () => this.loadBalancers }, { omitEmptyArray: true }),
Expand Down
82 changes: 81 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,96 @@
import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import { beASupersetOfTemplate, expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import * as appscaling from '@aws-cdk/aws-applicationautoscaling';
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
import * as cloudmap from '@aws-cdk/aws-servicediscovery';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Test } from 'nodeunit';
import * as ecs from '../../lib';
import { LaunchType } from '../../lib/base/base-service';

export = {
"When creating a Fargate Service": {
"with service id renamed to resource (with feature flag enabled)"(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also have a test to test the old case? (With the long name?)

// GIVEN
class NestedService extends cdk.Construct {
constructor(scope: cdk.Construct, id: string, props: ecs.FargateServiceProps) {
super(scope, id);

new ecs.FargateService(this, "Resource", props);
}
}

const app = new cdk.App({
context: { [cxapi.ENABLE_CFN_SERVICE_RESOURCE_RENAME]: 'true' }
});
const stack = new cdk.Stack(app, 'FeatureStack');
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"),
});

new ecs.FargateService(stack, "Fargate", {
cluster,
taskDefinition,
});

new NestedService(stack, "Service", {
cluster,
taskDefinition
});

// THEN
expect(stack).to(beASupersetOfTemplate({
Resources: {
Fargate001516A4: {
Type: "AWS::ECS::Service",
Properties: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only care about the name, is it okay if you leave Properties out or empty? Don't want to accidentally assert things that aren't relevant to this particular test.

Might be that our current assertion doesn't allow it, but I'd like to try.

Cluster: {
Ref: "EcsCluster97242B84"
},
DeploymentConfiguration: {
MaximumPercent: 200,
MinimumHealthyPercent: 50
},
DesiredCount: 1,
EnableECSManagedTags: false,
LaunchType: "FARGATE",
NetworkConfiguration: {
AwsvpcConfiguration: {
AssignPublicIp: "DISABLED",
SecurityGroups: [
{
"Fn::GetAtt": [
"FargateSecurityGroup953082A8",
"GroupId"
]
}
],
Subnets: [
{
Ref: "MyVpcPrivateSubnet1Subnet5057CF7E"
},
{
Ref: "MyVpcPrivateSubnet2Subnet0040C983"
}
]
}
},
TaskDefinition: {
Ref: "FargateTaskDefC6FB60B4"
}
}
},
}
}));

test.done();
},
"with only required properties set, it correctly sets default properties"(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down
9 changes: 8 additions & 1 deletion packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,20 @@
export const ENABLE_STACK_NAME_DUPLICATES_CONTEXT = '@aws-cdk/core:enableStackNameDuplicates';

/**
* IF this is set, `cdk diff` will always exit with 0.
* If this is set, `cdk diff` will always exit with 0.
*
* Use `cdk diff --fail` to exit with 1 if there's a diff.
*/
export const ENABLE_DIFF_NO_FAIL_CONTEXT = 'aws-cdk:enableDiffNoFail';
/** @deprecated use `ENABLE_DIFF_NO_FAIL_CONTEXT` */
export const ENABLE_DIFF_NO_FAIL = ENABLE_DIFF_NO_FAIL_CONTEXT;

/**
* If this is set, any newly created ECS service will have the logical name end
* with `Resource` rather than `Service`.
*/
export const ENABLE_CFN_SERVICE_RESOURCE_RENAME = 'aws-cdk/aws-ecs:enableCfnServiceResourceRename';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to call this something snappier and more to the point, like shortLogicalId.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add _CONTEXT to the variable name to mirror the ENABLE_DIFF example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it to ENABLE_SHORTEN_SRVC_LOGICAL_ID_CONTEXT any objection?


/**
* This map includes context keys and values for feature flags that enable
* capabilities "from the future", which we could not introduce as the default
Expand All @@ -45,4 +51,5 @@ export const ENABLE_DIFF_NO_FAIL = ENABLE_DIFF_NO_FAIL_CONTEXT;
export const FUTURE_FLAGS = {
[ENABLE_STACK_NAME_DUPLICATES_CONTEXT]: 'true',
[ENABLE_DIFF_NO_FAIL_CONTEXT]: 'true',
[ENABLE_CFN_SERVICE_RESOURCE_RENAME]: 'true',
};