-
Notifications
You must be signed in to change notification settings - Fork 422
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
fix: Added functionality to simply specify the database version number ins… #388
fix: Added functionality to simply specify the database version number ins… #388
Conversation
…tead of the whole version name for postgres 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.
Thanks for the PR @ravisiddhu
…tead of the whole version name for postgres module.
modules/postgresql/variables.tf
Outdated
@@ -34,6 +34,11 @@ variable "random_instance_name" { | |||
variable "database_version" { | |||
description = "The database version to use" | |||
type = string | |||
|
|||
validation { | |||
condition = (length(var.database_version) >= 9 && ((substr(var.database_version, 0, 9) == "POSTGRES_" || substr(var.database_version, 0, 9) == "postgres_") && can(regex("^\\d+(?:_?\\d)*$", substr(var.database_version, 9, -1))))) || can(regex("^\\d+(?:_?\\d)*$", var.database_version)) |
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 think we can just lower case to keep this shorter
condition = (length(var.database_version) >= 9 && ((substr(var.database_version, 0, 9) == "POSTGRES_" || substr(var.database_version, 0, 9) == "postgres_") && can(regex("^\\d+(?:_?\\d)*$", substr(var.database_version, 9, -1))))) || can(regex("^\\d+(?:_?\\d)*$", var.database_version)) | |
condition = (length(var.database_version) >= 9 && (lower(substr(var.database_version, 0, 9) && can(regex("^\\d+(?:_?\\d)*$", substr(var.database_version, 9, -1))))) || can(regex("^\\d+(?:_?\\d)*$", var.database_version)) |
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 have made the changes based on your suggestion.
I have also added "replace(var.database_version, substr(var.database_version, 0,8), "POSTGRES")" instead of just "var.database_version", to avoid terraform drift. (This drift was because "postgres_14" was being converted to "POSTGRES_14" by the API, hence there was a difference in state file and config file, hence the fix.)
…tead of the whole version name for postgres module.
…e there are some sensitive fields in google_sql_database_instance resource.
…le, since there are some sensitive fields in google_sql_database_instance resource." This reverts commit 6c3aeef.
@ravisiddhu |
This PR is a fix for #373
Buganizer : b/259913441
The reason for the issue is that when using postgres module, customer assumes that since they have explicitly chosen postgres module, they only need to specify the database version number instead of the whole version name as it says in doc, but that's not the case. (Example "14" instead of "POSTGRES_14")
When customer sets the database version field as simply "14" instead of "POSTGRES_14", the instances.insert API is actually trying to create a SQL Server instance (by default the database version used is "SQLSERVER_2017_ENTERPRISE"), that is why the API is throwing an error saying "Root password is required", because SQL Server instances requires root password.
But the customer assumptions seems valid and reasonable, hence I have added a functionality in postgres module which would prepend "POSTGRES_" to database version if they have only specified the numeric part of the database version.
Example "14" to "POSTGRES_14"
Successfully passed docker_test_lint