Skip to content
This repository was archived by the owner on Mar 29, 2023. It is now read-only.

Conversation

@robmorgan
Copy link
Contributor

@robmorgan robmorgan commented Jun 25, 2019

This PR upgrades terraform-google-sql to use Terraform 0.12. This is a backwards incompatible change.

  • Fix tests
  • Fix root example

Copy link
Contributor

@yorinasub17 yorinasub17 left a 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)
Copy link
Contributor

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:

Suggested change
type = list(any)
type = list(object({name = string, value = string}))

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

@robmorgan
Copy link
Contributor Author

cc @vincentdeity

@robmorgan
Copy link
Contributor Author

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

@robmorgan robmorgan merged commit 3b18b91 into master Jun 26, 2019
@robmorgan robmorgan deleted the tf12 branch June 26, 2019 14:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants