feat(aws-ec2): Add SubnetFilter for Id and CIDR netmask#15373
feat(aws-ec2): Add SubnetFilter for Id and CIDR netmask#15373mergify[bot] merged 12 commits intoaws:masterfrom
Conversation
|
|
|
|
||
| constructor(mask: number) { | ||
| super(); | ||
| if (mask < 16 || mask > 28 ) { |
There was a problem hiding this comment.
Outsider comment: this check isn't necessary. The Subnet class should already prevent creation of a subnet that isn't like 28 >= cidr mask >= 16.
If AWS supports a different range of subnets in the future, this would need to update as well.
There was a problem hiding this comment.
Agreed. I think this check is redundant.
njlynch
left a comment
There was a problem hiding this comment.
Looks good!
All really minor stuff. The docs I think need an update; the two code changes are optional (but recommended).
|
|
||
| constructor(mask: number) { | ||
| super(); | ||
| if (mask < 16 || mask > 28 ) { |
There was a problem hiding this comment.
Agreed. I think this check is redundant.
packages/@aws-cdk/aws-ec2/README.md
Outdated
| * `SubnetIdSubnetFilter`: chooses subnets from a list of ids | ||
| * `AvailabilityZoneSubnetFilter`: chooses subnets in the provided list of availability zones | ||
| * `OnePerAZSubnetFilter`: chooses at most one subnet per availability zone | ||
| * `ContainsIpAddressesSubnetFilter`: chooses a subnet which contains *any* of the listed ip addresses | ||
| * `CidrMaskSubnetFilter`: chooses subnets that have the provided CIDR netmask |
There was a problem hiding this comment.
This is a good enhancement to the documentation. However, since the various classes are all private (not exported) it probably makes more sense to replace the class names here with the static factory method names.
| return this.retainByCidrMask(subnets, this.mask); | ||
| } | ||
|
|
||
| private retainByCidrMask(subnets: ISubnet[], netmask: number): ISubnet[] { |
There was a problem hiding this comment.
~ (Nit) I would probably just inline this; I don't know that making it its own method adds anything here.
|
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 PR adds a couple of basic SubnetFilters into the CDK directly so they don't have to be needlessly reimplemented. The `SubnetIdSubnetFilter` filters subnets by ID and the `CidrMaskSubnetFilter` filters subnets based on the CIDR netmask. closes aws#15228 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR adds a couple of basic SubnetFilters into the CDK directly so they don't have to be needlessly reimplemented. The `SubnetIdSubnetFilter` filters subnets by ID and the `CidrMaskSubnetFilter` filters subnets based on the CIDR netmask. closes aws#15228 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR adds a couple of basic SubnetFilters into the CDK directly so they don't have to be needlessly reimplemented. The
SubnetIdSubnetFilterfilters subnets by ID and theCidrMaskSubnetFilterfilters subnets based on the CIDR netmask.closes #15228
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license