-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
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.
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!
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/start-job-run.test.ts
Show resolved
Hide resolved
Pull request has been modified.
…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>
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.
Took a look at a few more files today. Mostly comments on documentation and refactoring some checks into validation functions.
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/create-virtual-cluster.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/create-virtual-cluster.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/create-virtual-cluster.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/delete-virtual-cluster.ts
Outdated
Show resolved
Hide resolved
…lete-virtual-cluster.ts Co-authored-by: kaizen3031593 <36202692+kaizen3031593@users.noreply.github.com>
Pull request has been modified.
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/utils/role-policy/index.py
Show resolved
Hide resolved
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.
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!
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/emrcontainers/integ.start-job-run.ts
Show resolved
Hide resolved
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! |
Pull request has been modified.
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emrcontainers/utils/role-policy/index.py
Show resolved
Hide resolved
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). |
Pull request has been modified.
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 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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*
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*
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:
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