fix(ecs): rename service logical id (under feature flag) #6348
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Looks like the tests are passing irrespective of the content within it. Added the label until I can figure out why. |
|
Test updated. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * 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'; |
There was a problem hiding this comment.
Would prefer to call this something snappier and more to the point, like shortLogicalId.
There was a problem hiding this comment.
Add _CONTEXT to the variable name to mirror the ENABLE_DIFF example.
There was a problem hiding this comment.
Renamed it to ENABLE_SHORTEN_SRVC_LOGICAL_ID_CONTEXT any objection?
| Resources: { | ||
| Fargate001516A4: { | ||
| Type: "AWS::ECS::Service", | ||
| Properties: { |
There was a problem hiding this comment.
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.
| 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) { |
There was a problem hiding this comment.
Do you also have a test to test the old case? (With the long name?)
|
Also, please name the PR with the customer in mind. The customer will read the following bullet in our CHANGELOG: FIXES:
Will they know what that means for them? |
|
Please follow the guidelines on how to author the BREAKING CHANGE section for features under flags |
Because of the flag this won't be a breaking change though, right? |
|
No, it won’t, but are utilizing this section to announce the feature flag: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md |
|
More specific section: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#feature-flags |
piradeepk
changed the title
|
Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review. |
|
Closing for the time being |
Using the feature flag, update the logical id created by the baseService to append Resource rather than Service.
BREAKING CHANGE: ECS services (irrespective of whether they're ec2 or fargate services) created using the BaseService construct (and extensions) will have their logical ids removing the suffix Service. If the aws-cdk/aws-ecs:enableCfnServiceResourceRename flag is enabled, a customer who created a stack prior to this change will be forced to recreate their entire service.
Fixes: #3763
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license