-
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): run batch job #6396
Conversation
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 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 |
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.
Once batch has a more stable API, we can use the interfaces from batch instead of working with names and ARNs
Why wait? Why not start using them straight away?
On the same note, I had a doubt on naming this task. I was confused between calling it |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Let's switch to |
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 |
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 |
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.
Thanks for the updated revision. I have a few more comments based on a second iteration on this. It's starting to come together and looks close. 😊
// Resource-level access control is not supported by Batch | ||
// https://docs.aws.amazon.com/step-functions/latest/dg/batch-iam.html | ||
new iam.PolicyStatement({ | ||
resources: ['*'], |
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 found this - https://docs.aws.amazon.com/batch/latest/userguide/batch-supported-iam-actions-resources.html.
Is it possible that AWS batch has added supported for resource-level permissions and that the step functions page is stale?
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 am not sure how it would work out with the current batch implementation anyway. It does not expose the revision of a jobDefinition.
https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-batch.JobDefinition.html
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'm not sure I'm fully following your last comment.
I was thinking we could do -
resources: [ `${this.props.jobdefinition.jobDefinitionArn}/*` ]
This would be better than just *
.
packages/@aws-cdk/aws-stepfunctions-tasks/test/integ.run-batch-job.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/test/integ.run-batch-job.ts
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Mostly good. I'm still a bit worried about the *
permission, which I think we can do better, but not sure.
Some minor code clean up for readability.
This is very close to the line. Thanks for iterating on this!
// Resource-level access control is not supported by Batch | ||
// https://docs.aws.amazon.com/step-functions/latest/dg/batch-iam.html | ||
new iam.PolicyStatement({ | ||
resources: ['*'], |
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'm not sure I'm fully following your last comment.
I was thinking we could do -
resources: [ `${this.props.jobdefinition.jobDefinitionArn}/*` ]
This would be better than just *
.
packages/@aws-cdk/aws-stepfunctions-tasks/test/integ.run-batch-job.ts
Outdated
Show resolved
Hide resolved
Based on the permission configuration mentioned in this doc, we have to either provide the
So based on this, I am going ahead with these permission instead of new iam.PolicyStatement({
resources: [
Stack.of(_task).formatArn({
service: 'batch',
resource: 'job-definition',
resourceName: '*'
}),
this.props.jobQueue.jobQueueArn
],
actions: ['batch:SubmitJob']
}), That way we are limiting the permission to a specific queue but all the job-definitions. Once we have |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Oops, yes you're correct. I mis-read the doc. This is fine to start off with. |
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). |
Commit Message
feat(stepfunctions-tasks): run batch job (#6396)
Adding implementation to run batch job from step functions based on docs
closes #6467
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license