Skip to content
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): fargate support for jobs #15848

Merged
merged 6 commits into from
Sep 12, 2021
Merged

feat(batch): fargate support for jobs #15848

merged 6 commits into from
Sep 12, 2021

Conversation

DDynamic
Copy link
Contributor

@DDynamic DDynamic commented Aug 1, 2021

Added Fargate support for Batch jobs.

Note: this is not entirely my work - most of it was done by @kokachev. It is an updated version of Fargate support for batch jobs based on the feedback left in #13591.

  • Doc fixes
  • Integration test addition
  • Network configuration for Fargate
  • Support ResourceRequirements for Fargate jobs
  • Other minor fixes revealed by integration test

closes: #13590, #13591

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

@gitpod-io
Copy link

gitpod-io bot commented Aug 1, 2021

const isFargate = ComputeResourceType.FARGATE === props.computeResources.type
|| ComputeResourceType.FARGATE_SPOT === props.computeResources.type;

if (!isFargate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to avoid code that bypasses the type-system if we can. You can just compute the values before initializing the computeEnvironment and use them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

};

if (!isFargate) {
Object.assign(containerProperties, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same - just compute before needing to use it, and declare the types it uses so its easy to navigate and deep dive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/**
* Whether or not to assign a public IP to the job
*/
export enum AssignPublicIp {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems funky, can we not just convert all of this to a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@iliapolo
Copy link
Contributor

iliapolo commented Aug 2, 2021

@DDynamic Thanks for reviving this!

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 2, 2021
@mergify mergify bot dismissed iliapolo’s stale review August 2, 2021 23:27

Pull request has been modified.

@DDynamic
Copy link
Contributor Author

DDynamic commented Aug 3, 2021

@iliapolo should be ready for a final pass.

@DDynamic DDynamic requested a review from iliapolo August 4, 2021 02:44
packages/@aws-cdk/aws-batch/lib/compute-environment.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-batch/lib/job-definition.ts Outdated Show resolved Hide resolved
@iliapolo
Copy link
Contributor

iliapolo commented Aug 5, 2021

@DDynamic found a couple of more stuff that caught my eye, but I feel good we can merge this soon enough. Thanks!

@mergify mergify bot dismissed iliapolo’s stale review August 6, 2021 01:57

Pull request has been modified.

@DDynamic
Copy link
Contributor Author

DDynamic commented Aug 6, 2021

@iliapolo ready for another pass. Thanks for the through review!

@DDynamic DDynamic requested a review from iliapolo August 6, 2021 05:04
@robertd
Copy link
Contributor

robertd commented Aug 11, 2021

Thanks for working on this @DDynamic. I hope we can squeeze this in 1.118.0 😄

Poke @iliapolo 😉

@damian-bisignano
Copy link

Any chance this is going to be merged in soon? hanging out for this feature and it looks complete.

@DDynamic
Copy link
Contributor Author

@madeline-k 👋 I see you're now the assignee for aws-batch PRs and issues. Would you mind helping to bring this to completition?

@mergify
Copy link
Contributor

mergify bot commented Sep 12, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 04a1a3a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 066bcb1 into aws:master Sep 12, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 12, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-batch): Fargate support for batch jobs
6 participants