feat(batch): Stabilize Batch #27059
feat(batch): Stabilize Batch #27059
Conversation
There was a problem hiding this comment.
I didn't look at the specifics of the src or tests since these are just ported over but I do have a few comments about files that ended up in the wrong spot. Additionally. I think we should be calling this a feat instead of a chore and not include the breaking change in the text. batch didn't have anything in it so it can't be breaking.
There was a problem hiding this comment.
We don't want this file moved.
There was a problem hiding this comment.
This file shouldn't be within batch. It should be within the rosetta folder that then contains a folder for each module.
| * Note: the CDK will never set this value by default, `false` will set by CFN. | ||
| * This is to avoid a deployment failure that occurs when this value is set. | ||
| * | ||
| * @see https://github.com/aws/aws-cdk/issues/27054 | ||
| * | ||
| * @default false |
There was a problem hiding this comment.
I might be missing something here, but the linked issue makes it sound like setting this prop to true can break the deployment.
Can we fix the underlying issue before stabilization?
There was a problem hiding this comment.
The underlying issue is in CFN, and this is the fix. The problem is that whenever a ComputeEnvironment is created, the CFN handler first calls CreateCE, and then UpdateCE. The issue with this is that CreateCE does not set certain properties (in this case, this prop). UpdateCE is given these ignored properties, but UpdateCE sees that this property has changed (since it was not set by CreateCE); and, as the error indicates, these properties are not allowed to be changed.
CDK's default value is causing this issue by passing it in as true by default. The same issue would occur if it was false. This only occurs with Fargate, but the service team suggested the value not be set by default anywhere, so that is what this does.
The issue for this is really a needs-cfn, I'll update the issue.
|
@TheRealAmazonKendra true but it's breaking from the alpha module it is also breaking the alpha module to move all of this code out |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main 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 |
graduates the alpha module. All users of any `ManagedComputeEnvironment` who left `updateToLatestImageVersion` unspecified will see it default to `undefined` instead of `true`. *If you use `FargateComputeEnvironment`, this upgrade may cause deployment errors; destroy the Fargate CE and recreate it to resolve this, if encountered.* Related: #27054. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
graduates the alpha module.
All users of any
ManagedComputeEnvironmentwho leftupdateToLatestImageVersionunspecified will see it default toundefinedinstead oftrue. If you useFargateComputeEnvironment, this upgrade may cause deployment errors; destroy the Fargate CE and recreate it to resolve this, if encountered.Related: #27054.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license