-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(aws-ec2): control over VPC AZs #20562
Conversation
packages/@aws-cdk/aws-ec2/lib/vpc.ts
Outdated
* | ||
* This option overrides `maxAzs`. | ||
* | ||
* @default (a subset of) AZs of the stack |
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.
* @default (a subset of) AZs of the stack | |
* @default - a subset of AZs of the stack |
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.
Changed
for (let i = 1; i < 2; i++) { | ||
Template.fromStack(stack).hasResourceProperties('AWS::EC2::Subnet', { | ||
AvailabilityZone: `${stack.region}b`, | ||
}); | ||
} |
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.
this loop doesn't do anything, should be:
for (let i = 1; i < 2; i++) { | |
Template.fromStack(stack).hasResourceProperties('AWS::EC2::Subnet', { | |
AvailabilityZone: `${stack.region}b`, | |
}); | |
} | |
Template.fromStack(stack).hasResourceProperties('AWS::EC2::Subnet', { | |
AvailabilityZone: `${stack.region}b`, | |
}); |
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.
Oops, that was a thoughtless copy-paste. Fixed.
@@ -1317,10 +1332,13 @@ export class Vpc extends VpcBase { | |||
|
|||
Tags.of(this).add(NAME_TAG, props.vpcName || this.node.path); | |||
|
|||
this.availabilityZones = stack.availabilityZones; | |||
if (props.availabilityZones) { | |||
this.availabilityZones = props.availabilityZones; |
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 suspect there is validation that should be done on this parameter, to ensure there are no deployment time issues.
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.
Good idea. Done.
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.
As this is a feature, please add an integ test for this feature.
Thanks for the review, @comcalvi . I made the changes you suggested. |
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 PR!
@comcalvi Thanks. I don't know if I messed it up, but the branch was out of date so I merged main into it. Now it needs another review from you. Would you please? |
With this change, the `Vpc` construct gains a new constructor prop, `availabilityZones`, which gives more control over AZs than the existing `maxAzs` prop. closes aws#5847
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). |
With this change, the `Vpc` construct gains a new constructor prop, `availabilityZones`, which gives more control over AZs than the existing `maxAzs` prop. closes aws#5847 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [x] 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*
With this change, the
Vpc
construct gains a new constructor prop,availabilityZones
, which gives more control over AZs than the existingmaxAzs
prop.closes #5847
All Submissions:
Adding new Unconventional Dependencies:
New Features
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