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(stepfunctions-tasks): add 'Emr on Eks' tasks #17103

Merged
merged 47 commits into from
Nov 24, 2021
Merged

Conversation

sanava2010
Copy link
Contributor

@sanava2010 sanava2010 commented Oct 21, 2021

This CDK feature adds support for Emr on Eks by implementing API service integrations for the following three APIs.

This PR adds three tasks which support Emr on Eks:

  1. Create Virtual Cluster
  2. Start a job run
  3. Delete virtual cluster

Continuation of #15262 by @matthewsvu and @BenChaimberg:

Closes #15234.


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 Oct 21, 2021

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Hi @sanava2010! Looks good so far. I have only looked at start-job-run so far. I'm realizing that this is a gargantuan PR and it is tough for me to get context on all 3 tasks at the same time. It's still nearly 3k lines of code outside of the expected.json files. Can you please separate this into a PR for each task? It doesn't look like the tasks really rely on each other and this would help me focus on reviewing one section at a time. We can then merge after we've approved all 3 PRs so we support all 3 tasks together. (I know I told you to go about this PR in this manner, sorry for changing it up...)

If you haven't done the above, I think I can just power through this PR as is. I'll just try to look at things separately.

I also want to see if we can migrate the unit tests to the new Assertions library. This will help us slim the tests down to exactly what we are testing for -- lots of helper functions like Match.objectLike(), Match.arrayWith(), etc. But I will let you know if we can do this.

Great work!

@mergify mergify bot dismissed kaizencc’s stale review October 29, 2021 00:37

Pull request has been modified.

sanava2010 and others added 7 commits October 29, 2021 12:04
…tart-job-run.test.ts

Co-authored-by: kaizen3031593 <36202692+kaizen3031593@users.noreply.github.com>
…tart-job-run.test.ts

Co-authored-by: kaizen3031593 <36202692+kaizen3031593@users.noreply.github.com>
Co-authored-by: kaizen3031593 <36202692+kaizen3031593@users.noreply.github.com>
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Took a look at a few more files today. Mostly comments on documentation and refactoring some checks into validation functions.

…lete-virtual-cluster.ts

Co-authored-by: kaizen3031593 <36202692+kaizen3031593@users.noreply.github.com>
@mergify mergify bot dismissed kaizencc’s stale review October 29, 2021 23:19

Pull request has been modified.

@kaizencc kaizencc changed the title feat(stepfunctions-tasks): add emr-containers support for calling CreateVirtualCluster, DeleteVirtualCluster, and StartJobRun (continued)#15262 feat(stepfunctions-tasks): add 'Emr on Eks' tasks Nov 11, 2021
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Just a few lines of code that should have been deleted, and I want you to confirm that you have integ tested the job execution role policy update and that the custom resource is successful. Aim to approve this PR tomorrow. Great work!

@matthewsvu
Copy link

Thanks for going through the PR guys, I've been observing the progress on the PR and you guys fixed a lot of the issues that were plaguing the original PR and refactored them out.

Awesome work!

@mergify mergify bot dismissed kaizencc’s stale review November 17, 2021 02:11

Pull request has been modified.

kaizencc
kaizencc previously approved these changes Nov 18, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 18, 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).

@sanava2010
Copy link
Contributor Author

The build is failing because expected json of some stacks has changed, running yarn integ is not working and is throwing an issue while creating an EKS cluster and is throwing a CloudFormation did not receive a response from your Custom Resource exception. Working on debugging it.

Screen Shot 2021-11-18 at 4 52 22 PM

@mergify mergify bot dismissed kaizencc’s stale review November 22, 2021 18:59

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Nov 24, 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: 075a812
  • 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 f2bf322 into aws:master Nov 24, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 24, 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).

beezly pushed a commit to beezly/aws-cdk that referenced this pull request Nov 29, 2021
This CDK feature adds support for Emr on Eks by implementing API service integrations for the following three APIs.

This PR adds three tasks which support Emr on Eks:
1) [Create Virtual Cluster](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_CreateVirtualCluster.html)
2) [ Start a job run](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_StartJobRun.html)
3) [Delete virtual cluster ](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_DeleteVirtualCluster.html)


Continuation of aws#15262 by @matthewsvu and @BenChaimberg:

Closes aws#15234.

----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
This CDK feature adds support for Emr on Eks by implementing API service integrations for the following three APIs.

This PR adds three tasks which support Emr on Eks:
1) [Create Virtual Cluster](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_CreateVirtualCluster.html)
2) [ Start a job run](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_StartJobRun.html)
3) [Delete virtual cluster ](https://docs.aws.amazon.com/emr-on-eks/latest/APIReference/API_DeleteVirtualCluster.html)


Continuation of aws#15262 by @matthewsvu and @BenChaimberg:

Closes aws#15234.

----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-stepfunctions-tasks effort/large Large work item – several weeks of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(stepfunctions-tasks): emr-containers CreateVirtualCluster, DeleteVirtualCluster, and StartJobRun task
6 participants