fix(ecs): get rid of EFS casing warnings #19681
Conversation
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.
|
|
|
I'd like @iliapolo to confirm that I didn't overlook anything about the task replacement (given he originally introduced this patch). Also @MrArnoldPalmer mentions in this comment that the change was breaking because users would need to change their code. As far as I can tell, all the changes are hidden in our code? |
rix0rrr
added
the
pr/requires-two-approvers
I don't think you overlooked anything.
The L2 code won't have to change, but if people are using the L1's, it will break. However given the mess and confusion this creates, I think its worth just taking the hit here 👍 |
rix0rrr
removed
the
pr/requires-two-approvers
|
@iliapolo mind following that approval up with an actual approval? 😉 |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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 aws#10483, closes aws#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*
About a year ago, ECS TaskDefinition handler changed the casing of some
EFS-related properties:
EfsVolumeConfiguration->EFSVolumeConfigurationFileSystemId->FilesystemIdThey 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:
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.
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
Services based on theseTask 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:
Adding new Unconventional Dependencies:
New Features
cdk-integto deploy the infrastructure and generate the snapshot (i.e.cdk-integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license