Enable use of existing subnets #305
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
errordeveloper
force-pushed
the
subnets
branch
2 times, most recently
from
9632225
to
3e4c6d6
Compare
- refactor conditional logic for readability - add more flag compatibility validation - print warning to ensure that configuring the VPC and subnets is user's responsibility
errordeveloper
force-pushed
the
subnets
branch
from
3e4c6d6
to
d825a04
Compare
This was referenced Nov 6, 2018
errordeveloper
commented
Nov 7, 2018
|
|
||
| for _, subnet := range output.Subnets { | ||
| if c.Spec.VPC.ID == "" { | ||
| c.Spec.VPC.ID = *subnet.VpcId |
There was a problem hiding this comment.
Also need to get the CIDR for the VPC (as DNS IP depends on this), and we already have them for each of the subnets too (so might as well set them here).
richardcase
approved these changes
Nov 7, 2018
|
Yeah, but I based on #306 there is still plenty of opportunities in this
particular area.
Per my comment above, there is a bug that needs to be addressed, and looks
like I will have to address #293 along the way also. This is fairly
trivial, but it does imply we might have to add mocks for a few somewhat
incidental EC2 API calls.
Having said that, maybe a good idea to go with a follow-up PR rather then
jamming everything into this one.
…On Wed, 7 Nov 2018, 9:15 am Richard Case, ***@***.***> wrote:
***@***.**** approved this pull request.
This looks good to be. I wonder if we need to think about refactoring (in
general) in the future to start reducing the size of functions to make it
cleaner and less complex.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#305 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPWS-oqJIL8OXWwJkp0LBxPnKniXs4Aks5usqTIgaJpZM4YQwCl>
.
|
errordeveloper
changed the title
I'd agree, generally a good idea. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This should sufficient to close #42 . As discussed in #303, there will be more logical features to add, but this should enable most folks.
This adds
--vpc-{private,public}-subnetsflags. It's entirely up to the user which ones they specify as private or public. This is very clearly defined in #303, but we need to come up with a concise statement to put in the docs. We also have a waning message now, to make sure we don't get too many bug reports because of bad VPC configurations, but we still need to make it crystal clear in the docs.Checklist
make build)make test)