Skip to content
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

Network 2.0 #103

Closed
morgante opened this issue Nov 7, 2019 · 6 comments · Fixed by #86
Closed

Network 2.0 #103

morgante opened this issue Nov 7, 2019 · 6 comments · Fixed by #86
Assignees
Labels
enhancement New feature or request

Comments

@morgante
Copy link
Contributor

morgante commented Nov 7, 2019

For the Network 2.0 version, we'd like to make a few architecture changes.

Specifically:

  1. Moving subnet creation into a dedicated subnets submodule.
  2. Moving route creation (and default route deletion) into a routes submodule.
  3. Move vpc creation into a dedicated vpc submodule.
  4. Changing the root module into a composition module which still accepts the same (optional) inputs and passes them through to the submodules.

If you want to do everything in 1 module call, you will continue to be able to do so (through the root module). If you want to split things out, you can invoke the root module to only create the VPC then directly invoke submodules yourself for the other features.

This should future-proof the architecture and make issues like #100 simpler to deal with.

@morgante morgante mentioned this issue Nov 7, 2019
8 tasks
@jhulndev
Copy link

Hey @morgante, is there any assistance needed for this? If so I would be happy to submit a PR.

While we are breaking things, I would also like to see...

  1. google_compute_route.route change to a for_each so that adding or removing routes doesn't try to destroy and recreate them. I'm not using this specifically now, I've just run into that issue with other modules using count and it looks like the same could happen here.
  2. Fix for secondary_ranges does not support subnets with the same name #107 by simply having it be passed in var.subnets. I'm pretty certain I've had similar nested maps/objects in some of my other modules without issues... This will allow it to more closely mirror the original google_compute_subnetwork resource. Unless there is a reason var.secondary_ranges needs to be its own top level variable?

Would you be open to either of those?

@morgante
Copy link
Contributor Author

We'd definitely appreciate any help/PRs!

google_compute_route.route change to a for_each so that adding or removing routes doesn't try to destroy and recreate them. I'm not using this specifically now, I've just run into that issue with other modules using count and it looks like the same could happen here.

Yes, this is a good idea. We should probably do it while also moving routes into a submodule.

Fix for #107 by simply having it be passed in var.subnets. I'm pretty certain I've had similar nested maps/objects in some of my other modules without issues... This will allow it to more closely mirror the original google_compute_subnetwork resource. Unless there is a reason var.secondary_ranges needs to be its own top level variable?

I think @tfhartmann tried to do this, but there are still some issues with maps of maps in Terraform 0.12. You're more than welcome to give it a shot though.

@jhulndev
Copy link

Hey @morgante, I see @tfhartmann was working on #69? I'm catching up on it now, and realized I also ran into hashicorp/terraform#22449 when trying to do something similar 😄. I'll follow up on that PR.

Is there a recommended approach for moving code to the submodules when there are PRs open against those same sections? Should I just base any changes off of the release/2.0 branch and deal with the potential conflicts if needed?

@morgante
Copy link
Contributor Author

@taylorludwig Could you pick this up?

Should I just base any changes off of the release/2.0 branch and deal with the potential conflicts if needed?
@jason-huling Yes, you should base on release/2.0 for any similar work.

@taylorludwig
Copy link
Contributor

Yep, can do.

@aaron-lane aaron-lane added the enhancement New feature or request label Nov 19, 2019
@aaron-lane
Copy link
Contributor

To clarify, @taylorludwig is going to start by focusing on the 4 points in the initial comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants