-
Notifications
You must be signed in to change notification settings - Fork 104
Conversation
yorinasub17
left a comment
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! Left some minor suggestions for improving the types.
| variable "database_flags" { | ||
| description = "List of Cloud SQL flags that are applied to the database server" | ||
| type = "list" | ||
| type = list(any) |
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.
Do you know if both name and value are required? If so, maybe switch to:
| type = list(any) | |
| type = list(object({name = string, value = string})) |
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.
@yorinasub17 they are both optional, but it's pointless to pass anything else or only pass one of them, should I make that change anyway?
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 guess the value would need to be any as you can pass integers and strings etc: https://cloud.google.com/sql/docs/mysql/flags
|
I'm going to go ahead and release this. There's one minor open question about improving the types, but we can ship it in a patch release. cc @yorinasub17 |
This PR upgrades
terraform-google-sqlto use Terraform 0.12. This is a backwards incompatible change.