-
Notifications
You must be signed in to change notification settings - Fork 2
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
Nest network and within service & database modules #161
base: main
Are you sure you want to change the base?
Conversation
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.
nice!
infra/modules/network/data/main.tf
Outdated
@@ -3,30 +3,34 @@ module "interface" { | |||
name = var.name | |||
} | |||
|
|||
module "project_config" { |
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 we discussed in our 1:1 don't introduce dependencies on config modules in reusable modules
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 can definitely make that change! I just don't totally remember the motivation there. Like, what's the conceptual difference between nesting the network model and nesting the project config module?
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.
infra/modules/ are reusable modules. they should have no dependencies on anything project specific.
Longer term once these modules are stable we may decided to publish them to the terraform registry, so we should have no dependencies on any local project configuration.
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.
Ah! So infra/modules are truly meant to be as flexible as terraform modules you would find on the registry. Gotcha.
That changes my point of view here a bit... but I'm going to continue on with nesting the VPC module
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.
Yeah that's also why I was on the fence about nesting the network module, because by nesting the network module it means that database and service modules now have a dependency on the network module, meaning that in order for someone to use the database and service modules they would also need to use the network module, or at least adhere to the same interface for the network module. But after seeing how it cleans up the interface I think maybe the benefit is worth the tradeoff.
vpc_id = module.network.vpc_id | ||
public_subnet_ids = module.network.public_subnet_ids | ||
private_subnet_ids = module.network.private_subnet_ids | ||
aws_services_security_group_id = module.network.aws_services_security_group_id |
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.
😎
@@ -33,6 +33,10 @@ locals { | |||
) | |||
} | |||
|
|||
module "project_config" { |
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.
clean this up
Motivation and Context
The motivation of this PR is in this diff:
Nesting the network module within the service/database modules has the following impact:
This makes the modules more flexible and resilient!
Testing
Preview environment for app
Preview environment for app-rails