feat(batch): ComputeEnvironment implements IConnectable #21458
Conversation
|
|
github-actions
bot
added
feature-request
tcutts
changed the title
mrgrain
changed the title
|
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. |
Co-authored-by: Momo Kornher <[email protected]>
mergify
bot
dismissed stale reviews from TheRealAmazonKendra and mrgrain
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? |
|
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 aws#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: ```ts something.connections.allowDefaultPortFrom(computeEnvironment); ``` 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: * [yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [no] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [yes] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [yes] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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