-
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
Make VPC database resources to be conditionally added only if there is a database needed in the network #590
Conversation
…tabase in the network
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 looks good!
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.
left some nits. also, i didn't see any evidence of testing in the PR, which i'd ideally like to see before approving
infra/modules/network/main.tf
Outdated
database_subnet_tags = var.has_database ? { subnet_type = "database" } : {} | ||
database_subnet_group_name = var.has_database ? var.database_subnet_group_name : "" |
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.
nit: I believe we don't actually need the conditionals here:
- the tags only apply to created db subnets so if we don't create db subnets then the tags don't apply
- similarly the subnet group name doesn't do anything if create database subnet group is false
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.
While they aren't strictly needed, I do prefer that the usage is explicit, rather than implied. Whenever I am tracing broken code things can become confusing if people are setting what appear to be "dead" variables. Alternatively a comment may be helpful?
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'd be fine with a comment. In my opinion dead code causes more issues than dead variables.
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.
Note: This is my personal github. I'll get signed into my usual one and make sure I'm on the right account in the future.
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.
@lorenyu Good catch. I've pushed a change that removes the unnecessary conditionals and adds some more commenting. The comments were a little unwieldy, so I re-organized a little to group related items together.
Adding now and will re-ping you when I'm ready for re-review. Thanks! |
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.
LGTM. Sorry for the delayed review.
Rollout note:
One thing to remember to do after merging, is that on platform-test-nextjs you'll want to run terraform apply in the network layer, since the network layer doesn't get auto-applied as part of CD. I imagine apply would end up removing the subnets and subnet group.
Thank you for the tip! I would have missed that. |
Following up to say that this is complete! |
Ticket
N/A
Changes
Context for reviewers
Right now, the first time you configure the network and run
make infra-update-network NETWORK_NAME=<NETWORK_NAME>
, the database subnets and subnet group are created even if there are no databases specified in the network.This change makes creating those resources conditional based on whether or not there databases are needed.
I think it's better to make this conditional to reduce the number of unnecessary resources created. In general, unnecessary resources add additional cost and management overhead (i.e. it was confusing to me to see database subnets being created for an application that had no database).
Testing
Testing performed by deploying this branch to my local version of https://github.com/navapbc/platform-test and then deploying necessary resources (account, network) to my AWS account. This screenshot shows:
has_database
is set tofalse
in/infra/app/app-config/main.tf
dev
network which does not have any database VPC endpoints