-
Notifications
You must be signed in to change notification settings - Fork 40
Generate two CFN templates for create VPC and import VPC for bootstrap #1113
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
Generate two CFN templates for create VPC and import VPC for bootstrap #1113
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1113 +/- ##
=========================================
Coverage 80.55% 80.55%
Complexity 2930 2930
=========================================
Files 390 390
Lines 14462 14462
Branches 998 998
=========================================
Hits 11650 11650
Misses 2209 2209
Partials 603 603
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tanner Lewis <[email protected]>
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 working on this, I'll do another pass on the text-descriptions, but these are a huge improvement!
Signed-off-by: Tanner Lewis <[email protected]>
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 those updates, a shame about the conditions being not very useful. Going into more nitpicky UX. Could you also see about reviewing the UX with product? - Happy to any additional changes happen out of band of this change.
availabilityZones: [Fn.select(0, availabilityZones)], | ||
privateSubnetIds: [Fn.select(0, privateSubnetIds)] |
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.
Why aren't we including all AZ's/subnets pairs specified by the customer?
Curious, if we don't need them to find the VPC, does this mean that we could accidentally allocate resources under subnets other than the ones specified by the user?
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.
My understanding is that since we are not doing lookup and instead essentially crafting parts of an existing VPC from attributes, we will only use attributes that we have specified and no lookup is actually happening. This CFN itself only needs one subnet and one AZ for our EC2 instance
Co-authored-by: Peter Nied <[email protected]> Signed-off-by: Tanner Lewis <[email protected]>
Co-authored-by: Peter Nied <[email protected]> Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Description
Changes functionality to generate two CFN templates for create VPC and import VPC for bootstrap. As well as introduces VPC Id and Subnet intake from CFN parameters that will later be used when deploying the Migration Assistant CDK
Import VPC v1
Import VPC v2
Create VPC
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-2166
Is this a backport? If so, please add backport PR # and/or commits #
Testing
Manual CFN deployment testing
CDK tests
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.