-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Database encryption #165
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
Database encryption #165
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.
In addition to my comments, I don't think we should be co-opting an unrelated PR to s/(region|zone)/location/
. That should really be its own PR.
7f51a01
to
5423f41
Compare
description = "The secondary ip range to use for pods" | ||
} | ||
|
||
variable "database_encryption" { |
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.
Examples in this repository serve both as tangible demonstrations that users can rely on to show how functionality is utilized, and as test cases -- but primarily as demonstrations. With that in mind, it would probably be worth considering not passing this in as a variable, instead making it immediately obvious what's going on.
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 create example but no test cases for now. Would that be sufficient?
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.
Adding functionality without adding tests significantly increases the failure domain of the module, and I don't think that's a risk we should be taking.
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.
Per conversation out of band, let's just make sure #185 is addressed once we have Terraform 0.12 compatibility, and this can be merged.
region = attribute('region') | ||
|
||
control "gcloud" do | ||
title "GKE " |
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.
Extra whitespace?
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.
No tests will be added for now.
f74d2d6
to
fbdf65c
Compare
1d25fb3
to
ddf8ffd
Compare
… case. Updating documentation
209d0a4
to
b294685
Compare
database_encryption
variable that enables Application level Secrets Encryption as described here: https://cloud.google.com/kubernetes-engine/docs/how-to/encrypting-secrets