Skip to content

Commit

Permalink
fix(ecs): get rid of EFS casing warnings (#19681)
Browse files Browse the repository at this point in the history
About a year ago, ECS TaskDefinition handler changed the casing of some
EFS-related properties:

* `EfsVolumeConfiguration` -> `EFSVolumeConfiguration`
* `FileSystemId` -> `FilesystemId`

They continue to accept both casings, but emit a warning when the
deprecated casing is used. When the new casing was introduced, we
reverted to the old casing in order to not cause resource replacements.

However:

- The old casings emit warnings; when the service/task creation fails
  due to unrelated reasons, users see the warnings, interpret them as
  errors, then stop looking and come and tell us that there is a bug
  in CDK.
- Task definition replacement isn't actually a problem. Task definitions
  can be replaced for something as trivial as changing CPU count or
  memory size. Replacing them for a change that is effectively a no-op
  shouldn't matter. Yes, this will restart `Service`s based on these
  Task Definitions, but if you are only running 1 copy of the Task
  you have made the decision not to care about potential downtime of
  your service.

Maintaining the patch does not seem worth the cost/benefit ratio.

Reverts #10483, closes #15025.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr committed Apr 6, 2022
1 parent b0834fe commit eafc11a
Show file tree
Hide file tree
Showing 6 changed files with 524 additions and 32 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ export class TaskDefinition extends TaskDefinitionBase {
scope: spec.dockerVolumeConfiguration.scope,
},
efsVolumeConfiguration: spec.efsVolumeConfiguration && {
fileSystemId: spec.efsVolumeConfiguration.fileSystemId,
filesystemId: spec.efsVolumeConfiguration.fileSystemId,
authorizationConfig: spec.efsVolumeConfiguration.authorizationConfig,
rootDirectory: spec.efsVolumeConfiguration.rootDirectory,
transitEncryption: spec.efsVolumeConfiguration.transitEncryption,
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-ecs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"devDependencies": {
"@aws-cdk/assertions": "0.0.0",
"@aws-cdk/aws-s3-deployment": "0.0.0",
"@aws-cdk/aws-efs": "0.0.0",
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/cdk-integ-tools": "0.0.0",
"@aws-cdk/cfn2ts": "0.0.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1154,8 +1154,8 @@ describe('ec2 task definition', () => {
Family: 'Ec2TaskDef',
Volumes: [{
Name: 'scratch',
EfsVolumeConfiguration: {
FileSystemId: 'local',
EFSVolumeConfiguration: {
FilesystemId: 'local',
},
}],
});
Expand Down

0 comments on commit eafc11a

Please sign in to comment.