-
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(stepfunctions-tasks): add step functions task to run glue job #6258
Conversation
* add new task RunGlueJobTask and associated unit tests * since Job construct does not yet exist, uses job name as required parameter closes #5266
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.
Apologies about the late response on this.
How hard would it be to add an integration test that sets up a simple glue job and a simple state machine that calls it? Also, to add a couple of statements as described here on how to verify that it actually worked.
packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-glue-job-task.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-glue-job-task.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-glue-job-task.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-glue-job-task.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-glue-job-task.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
"@aws-cdk/aws-iam": "999.0.0", | ||
"@aws-cdk/aws-kms": "999.0.0", | ||
"@aws-cdk/aws-lambda": "999.0.0", | ||
"@aws-cdk/aws-s3": "999.0.0", | ||
"@aws-cdk/aws-s3-assets": "999.0.0", |
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.
can we make this a devdependency?
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 think so...I'm still a node noob but since it's only used for integration testing, that would make sense. I'll also remove it from "dependencies", but let me know if that's incorrect.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
packages/@aws-cdk/aws-stepfunctions-tasks/test/integ.glue-task.ts
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@BenChaimberg - I've gone ahead and cleaned up the note about cold start. Hope you don't mind. Approving momentarily. From this point, on avoid merging from master, i.e., clicking 'Update branch'. mergify will take this forward automatically. |
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
resources: ["*"], | ||
actions: [ | ||
"glue:StartJobRun", | ||
"glue:GetJobRun", | ||
"glue:GetJobRuns", | ||
"glue:BatchStopJobRun" | ||
], |
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.
Sorry, there's something I missed here.
Can we reduce the scope of this to just the glue job's ARN?
Also, why does this task need BatchStopJobRun
access?
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 chose these permissions because they are the same as those created by Step Functions when a state machine with a glue task is created in the console. The BatchStopJobRun
is required for stopping the glue job if the state machine execution is cancelled.
I didn't think it was possible to specify a resource for these permissions because the Glue docs state that "Glue doesn't allow ARNs for [...] job runs". But I changed the permissions and it seems to work fine for starting the run, realizing the run has finished, and stopping the run.
Since the permissions are different for the different service integration patterns (fire and forget vs. sync), I also added a conditional that sets the appropriate set based on the specified pattern.
…nto glue_job_sfn_task
Pull request has been modified.
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). |
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). |
…6258) * feat(stepfunctions-tasks): add step functions task to run glue job * add new task RunGlueJobTask and associated unit tests * since Job construct does not yet exist, uses job name as required parameter closes #5266 * cleanup constructor properties, add integration test * remove job run ID from props, update default prop descriptions * add s3 assets package to module * fix linting errors * clean up documentation, add links to docs and glue task example * add verification step to integration step, ensure job succeeds * update expected integration test stack (asset names) * add integ test verification comment about glue cold start * cleaned up the note around cold start * specify glue job ARN in state machine role permissions * change state machine role permissions based on service integration pattern Co-authored-by: Niranjan Jayakar <16217941+nija-at@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
closes #5266
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license