-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(batch): add fargate Runtime Platform properties to ECS Fargate C… #28841
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments are mostly for my understanding of the change. Otherwise, seems like a straight forward change.
@@ -1616,7 +1620,7 @@ | |||
} | |||
}, | |||
"constructInfo": { | |||
"fqn": "@aws-cdk/aws-batch-alpha.EcsJobDefinition", | |||
"fqn": "aws-cdk-lib.aws_batch.EcsJobDefinition", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last change of this file was to stabilizing (convert alpha to regular module). So the snapshot would be before stabilizing which was still using aws-batch-alpha
/** | ||
* The vCPU architecture of Fargate Runtime. | ||
* | ||
* @default - X86_64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, why set defaults for this and fargateOperatingSystemFamily
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is the default value should be same as the default value in CFN resource.
fargateCpuArchitecture: ecs.CpuArchitecture.ARM64, | ||
fargateOperatingSystemFamily: ecs.OperatingSystemFamily.LINUX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that the batch
EcsFaragetContainerDefintion
will always match ecs
? I am wondering if there is a case where ECS needs some update and then batch needs to follow. Meaning there would be a time where batch doesn't support all types ECS supports.
I might be looking at this in the wrong way or have an miss understanding on how things connect together within CDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw it was already using enum values from ECS definition so I didn't really think about it... Looking at the current values they're the same but I'm not sure if that's guaranteed or it's documented somewhere.
If we can generate all these values from the schema in the future, then we can be confident about the values here.
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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
#28841) The property [RuntimePlatform](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-batch-jobdefinition-containerproperties.html#cfn-batch-jobdefinition-containerproperties-runtimeplatform) is not present in the AWS Batch ECS Fargate Job Definition. This PR adds flatten properties fargateCpuArchitecture and fargateOperatingSystemFamily to the ECS Fargate Job Definition in AWS Batch. Closes #26484. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The property RuntimePlatform is not present in the AWS Batch ECS Fargate Job Definition. This PR adds flatten properties fargateCpuArchitecture and fargateOperatingSystemFamily to the ECS Fargate Job Definition in AWS Batch.
Closes #26484.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license