feat(batch-alpha): add RuntimePlatform prop #26506

khushail · 2023-07-26T00:32:21Z

Closes #26484.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

packages/@aws-cdk/aws-batch-alpha/lib/runtime-platform.ts

aws-cdk-automation · 2023-08-15T00:07:42Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

pahud · 2023-08-15T20:31:06Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

+    if (props.runtimePlatform) {
+      this.runtimePlatform = props.runtimePlatform;
+    }
+


I think you should modify the _renderContainerDefinition() function to add the new runtimePlatform property like

runtimePlatform: this.runtimePlatform

So it will synth a new container definition for ContainerProperties

pahud · 2023-08-15T21:53:15Z

OK looks like the RuntimePlatform support was just added into cloudformation in Aug 2023

AWS CDK at this moment uses CFN spec 130.1.0 which does not include this property yet.

aws-cdk/packages/@aws-cdk/cfnspec/spec-source/specification/000_cfn/000_official/000_AWS_Batch.json

Line 2 in ed9b537

"$version": "130.1.0",

cfnspec 130.1.0 was PR merged on July 14
#26362

The latest CFN spec is 135.0.0 which DOES have runtimePlatform property.
https://d1uauaxba7bl26.cloudfront.net/latest/gzip/CloudFormationResourceSpecification.json

We will need to wait another PR from the core team to support 135.0.0 or above before we are allowed to continue this PR.

aws-cdk-automation · 2023-08-19T00:07:08Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-20T00:08:08Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-21T00:07:46Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-22T00:07:50Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-23T00:07:45Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-24T00:07:36Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-25T00:07:41Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

pahud · 2023-08-25T17:46:42Z

I was wrong. CDK now uses awscdk-service-spec as the source of the truth for the L1 and the latest definition can be found here:
https://github.com/cdklabs/awscdk-service-spec/blob/main/sources/CloudFormationSchema/us-east-1/aws-batch-jobdefinition.json

And I can confirm ContainerProperties.RuntimePlatform is already in the definition so I think it should be available in the L1 props.

    "ContainerProperties" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "User" : {
          "type" : "string"
        },
        "Secrets" : {
          "type" : "array",
          "uniqueItems" : false,
          "items" : {
            "$ref" : "#/definitions/Secret"
          }
        },
        "Memory" : {
          "type" : "integer"
        },
        "Privileged" : {
          "type" : "boolean"
        },
        "LinuxParameters" : {
          "$ref" : "#/definitions/LinuxParameters"
        },
        "FargatePlatformConfiguration" : {
          "$ref" : "#/definitions/FargatePlatformConfiguration"
        },
        "JobRoleArn" : {
          "type" : "string"
        },
        "ReadonlyRootFilesystem" : {
          "type" : "boolean"
        },
        "Vcpus" : {
          "type" : "integer"
        },
        "Image" : {
          "type" : "string"
        },
        "ResourceRequirements" : {
          "type" : "array",
          "uniqueItems" : false,
          "items" : {
            "$ref" : "#/definitions/ResourceRequirement"
          }
        },
        "LogConfiguration" : {
          "$ref" : "#/definitions/LogConfiguration"
        },
        "MountPoints" : {
          "type" : "array",
          "uniqueItems" : false,
          "items" : {
            "$ref" : "#/definitions/MountPoints"
          }
        },
        "ExecutionRoleArn" : {
          "type" : "string"
        },
        "RuntimePlatform" : {
          "$ref" : "#/definitions/RuntimePlatform"
        },

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Pull request has been modified.

aws-cdk-automation · 2023-08-25T22:01:28Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 579cfe2
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2023-08-26T00:07:25Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

rix0rrr · 2023-08-29T11:38:28Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

+   *
+   * A runtimePlatform is supported only for tasks using the Fargate launch type.
+   *
+   * @default - Undefined.


Please don't write "undefined". We all know that the JavaScript value of a missing variable is undefined.

Describe what the behavior is if the value is left undefined.

Does this property need to be on the interface? IEcsContainerDefinition ?

It only needs to be here if downstream consumers of this class need to make decisions based on runtimePlatform. If so, it also needs to be added to EcsContainerDefinition.fromContainerDefinitionAttributes().

rix0rrr · 2023-08-29T11:38:45Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

+   *
+   * A runtimePlatform is supported only for tasks using the Fargate launch type.
+   *
+   * @default - Undefined.


rix0rrr · 2023-08-29T11:39:32Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

@@ -643,6 +667,10 @@ abstract class EcsContainerDefinitionBase extends Construct implements IEcsConta
      logConfiguration: this.logDriverConfig,
      readonlyRootFilesystem: this.readonlyRootFilesystem,
      resourceRequirements: this._renderResourceRequirements(),
+      runtimePlatform: {


Please make it so the entire subobject is not rendered if this.runtimePlatform is undefined, otherwise this will lead to unnecessary changes to existing templates.

rix0rrr · 2023-08-29T11:44:07Z

packages/@aws-cdk/aws-batch-alpha/lib/ecs-container-definition.ts

+   *
+   * @default - Undefined.
+   */
+  readonly runtimePlatform?: RuntimePlatform;


Instead of having this subproperty, how about we inline the 2 fields from RuntimePlatform here? It will be easier to work with for users.

Prefer flat properties, CDK Design Guidelines, section Props.

rix0rrr · 2023-08-29T11:44:54Z

Also this is not a chore, feat or fix are better choices.

aws-cdk-automation · 2023-09-10T00:08:10Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-09-11T00:07:34Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

gmarchand · 2023-09-11T05:58:58Z

Hello @pahud, Sorry I can not help you on this one, I don't have the skills to contribute. But the need is always here.

mrgrain · 2023-09-11T10:10:09Z

@comcalvi FYI

aws-cdk-automation · 2023-09-12T00:07:37Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-09-13T00:07:30Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-09-14T00:07:35Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-09-15T00:07:37Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-09-16T00:07:22Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

aws-cdk-automation · 2023-09-26T17:48:00Z

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2023-09-27T00:07:42Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

added RuntimePlatform

bd2801d

github-actions bot added the p2 label Jul 26, 2023

aws-cdk-automation requested a review from a team July 26, 2023 00:32

aws-cdk-automation previously requested changes Jul 26, 2023

View reviewed changes

khushail changed the title ~~(aws-batch-alpha): Construct Props RuntimePlatform~~ feat(aws-batch-alpha): add RuntimePlatform prop Jul 26, 2023

pahud requested changes Jul 27, 2023

View reviewed changes

packages/@aws-cdk/aws-batch-alpha/lib/runtime-platform.ts Show resolved Hide resolved

khushail added 2 commits August 1, 2023 00:04

added implementation of runtimePlatform

a782b2f

added integ test

e19717d

khushail added 2 commits August 15, 2023 01:03

updated unit test

c1d4d95

resolving conflicts

10fdd1e

khushail changed the title ~~feat(aws-batch-alpha): add RuntimePlatform prop~~ feat(batch-alpha): add RuntimePlatform prop Aug 15, 2023

pahud previously requested changes Aug 15, 2023

View reviewed changes

khushail added 2 commits August 25, 2023 11:19

Merge branch 'main' into khushail/aws-batch-alpha-construct-26484

ca4904d

Merge branch 'main' into khushail/aws-batch-alpha-construct-26484

7d7e770

khushail changed the title ~~feat(batch-alpha): add RuntimePlatform prop~~ chore(batch-alpha): add RuntimePlatform prop Aug 25, 2023

khushail marked this pull request as ready for review August 25, 2023 19:45

Merge branch 'main' into khushail/aws-batch-alpha-construct-26484

dc3e002

updated renderContainerDefinition with props

579cfe2

gmarchand mentioned this pull request Aug 28, 2023

AWS Fargate supports Linux ARM64 aws-samples/aws-batch-with-ffmpeg#11

Open

rix0rrr requested changes Aug 29, 2023

View reviewed changes

khushail marked this pull request as draft September 15, 2023 23:07

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Sep 16, 2023

aws-cdk-automation closed this Sep 16, 2023

gmarchand mentioned this pull request Sep 25, 2023

(aws-batch-alpha): Construct Props RuntimePlatform #26484

Closed

2 tasks

khushail changed the title ~~chore(batch-alpha): add RuntimePlatform prop~~ feat(batch-alpha): add RuntimePlatform prop Sep 26, 2023

khushail reopened this Sep 26, 2023

aws-cdk-automation requested changes Sep 26, 2023

View reviewed changes

aws-cdk-automation closed this Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(batch-alpha): add RuntimePlatform prop #26506

feat(batch-alpha): add RuntimePlatform prop #26506

khushail commented Jul 26, 2023 •

edited by rix0rrr

Loading

aws-cdk-automation left a comment

aws-cdk-automation commented Aug 15, 2023

pahud Aug 15, 2023

pahud commented Aug 15, 2023 •

edited

Loading

aws-cdk-automation commented Aug 19, 2023

aws-cdk-automation commented Aug 20, 2023

aws-cdk-automation commented Aug 21, 2023

aws-cdk-automation commented Aug 22, 2023

aws-cdk-automation commented Aug 23, 2023

aws-cdk-automation commented Aug 24, 2023

aws-cdk-automation commented Aug 25, 2023

pahud commented Aug 25, 2023

aws-cdk-automation commented Aug 25, 2023

aws-cdk-automation commented Aug 26, 2023

rix0rrr Aug 29, 2023

rix0rrr Aug 29, 2023

rix0rrr Aug 29, 2023

rix0rrr Aug 29, 2023

rix0rrr Aug 29, 2023

rix0rrr commented Aug 29, 2023

aws-cdk-automation commented Sep 10, 2023

aws-cdk-automation commented Sep 11, 2023

gmarchand commented Sep 11, 2023

mrgrain commented Sep 11, 2023

aws-cdk-automation commented Sep 12, 2023

aws-cdk-automation commented Sep 13, 2023

aws-cdk-automation commented Sep 14, 2023

aws-cdk-automation commented Sep 15, 2023

aws-cdk-automation commented Sep 16, 2023

aws-cdk-automation left a comment

aws-cdk-automation commented Sep 26, 2023

aws-cdk-automation commented Sep 27, 2023

feat(batch-alpha): add RuntimePlatform prop #26506

feat(batch-alpha): add RuntimePlatform prop #26506

Conversation

khushail commented Jul 26, 2023 • edited by rix0rrr Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 15, 2023

pahud Aug 15, 2023

Choose a reason for hiding this comment

pahud commented Aug 15, 2023 • edited Loading

aws-cdk-automation commented Aug 19, 2023

aws-cdk-automation commented Aug 20, 2023

aws-cdk-automation commented Aug 21, 2023

aws-cdk-automation commented Aug 22, 2023

aws-cdk-automation commented Aug 23, 2023

aws-cdk-automation commented Aug 24, 2023

aws-cdk-automation commented Aug 25, 2023

pahud commented Aug 25, 2023

aws-cdk-automation commented Aug 25, 2023

AWS CodeBuild CI Report

aws-cdk-automation commented Aug 26, 2023

rix0rrr Aug 29, 2023

Choose a reason for hiding this comment

rix0rrr Aug 29, 2023

Choose a reason for hiding this comment

rix0rrr Aug 29, 2023

Choose a reason for hiding this comment

rix0rrr Aug 29, 2023

Choose a reason for hiding this comment

rix0rrr Aug 29, 2023

Choose a reason for hiding this comment

rix0rrr commented Aug 29, 2023

aws-cdk-automation commented Sep 10, 2023

aws-cdk-automation commented Sep 11, 2023

gmarchand commented Sep 11, 2023

mrgrain commented Sep 11, 2023

aws-cdk-automation commented Sep 12, 2023

aws-cdk-automation commented Sep 13, 2023

aws-cdk-automation commented Sep 14, 2023

aws-cdk-automation commented Sep 15, 2023

aws-cdk-automation commented Sep 16, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 26, 2023

aws-cdk-automation commented Sep 27, 2023

khushail commented Jul 26, 2023 •

edited by rix0rrr

Loading

pahud commented Aug 15, 2023 •

edited

Loading