feat(batch): ComputeEnvironment implements IConnectable#21458
feat(batch): ComputeEnvironment implements IConnectable#21458mergify[bot] merged 10 commits intoaws:mainfrom tcutts:connectableComputeEnvironment
Conversation
|
|
|
Hi @tcutts and thanks for the contribution! This looks exciting and I think you've found the right approach to implement this. Before I go ahead with a more thorough review, can you please make sure the build passes and with this kind of logic change, we will definitely need unit tests in addition to the integration test you've already added. |
|
There you go @mrgrain - builds properly now. My example in the README needed fixing... Thanks for the encouraging words. |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Looks like we still need unit tests on this change.
mrgrain
left a comment
There was a problem hiding this comment.
A few minor changes and since we are changing the logical, I'd like to see a unit test.
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
Pull request has been modified.
|
I've added a couple of unit tests, but it's not a particular area of expertise for me (not being an SDE!); I've borrowed some of the connections test logic from aws-ec2, and re-purposed it here to check that the right ingress rules are created, for both fargate and EC2 managed compute environments. Is that sufficient? |
mrgrain
left a comment
There was a problem hiding this comment.
Yes, these look great! Thank you for your work here. 
|
Thank you for contributing! Your pull request will be updated from main 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
closes #20983
ComputeEnvironments have embedded security groups in them. These are currently difficult to reach and modify in CDK stacks, which forces users into undesirable practices when integrating batch queues with other services such as as EFS filesystems or RDS instances. Ideally, it should be possible to use compute environments as a target:
but this isn't currently possible, so the user may try to work around it by allowing from any IPv4, or by having to use an escape hatch, which is not simple.
This pull request adds ec2.IConnectable to batch.ComputeEnvironment. It still seems to pass all the existing tests and integration tests, so I don't think it's a breaking change, but I suspect it could be done in a better way than I've done it, to make things even simpler for the user.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license