-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add import for google_sql_database_instance
#11
Add import for google_sql_database_instance
#11
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.
Nice work! This is a big/old file, thanks for tackling it. I left a few comments for you to look over.
@@ -630,6 +634,11 @@ func resourceSqlDatabaseInstanceRead(d *schema.ResourceData, meta interface{}) e | |||
return err | |||
} | |||
|
|||
// In the import case name won't be set yet |
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.
Instead of setting name
here, let's use d.Id() in the Get
below, and d.Set
name
using the Name
property of instance
later. That way, both refresh and import run through the same code
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've had to add a d.setId
to the Create as the resource relied on it being set at the end of the first Read.
} else { | ||
_settingsList = make([]interface{}, 1) | ||
_settings = make(map[string]interface{}) | ||
d.Set("region", instance.Region) |
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.
We should perform this d.Set
in every Read instead of only during imports; this file was written as if Terraform was a non-authoritative tool, but our current standard is to make it one.
if len(_settingsList) > 0 { | ||
_settings = _settingsList[0].(map[string]interface{}) | ||
} else { | ||
_settingsList = make([]interface{}, 1) |
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 we read _settingsList anywhere after this? I don't think we need this line.
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.
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.
It looks like when we use the _settingsList
slice down there, we are just overwriting it's contents with the _settings
object. We can introduce a new slice to wrap _settings
in there, and better semantically show that the two slices are not linked. That way, we can eliminate this line and break an implicit dependency between these parts of the function.
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 amendments, please let me know if I understood correctly.
Database instances can be imported using the `name`, e.g. | ||
|
||
``` | ||
$ terraform import google_sql_database_instance.instance instance_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.
Replace with this to match the import statement with the resource in the example:
$ terraform import google_sql_database_instance.master master-instance
Thanks for the review @rileykarson, I have made your suggested amendments.
|
|
LGTM - thanks for the great work on this @gh-mlfowler!
|
Add `google_sql_database` and `google_sql_database_instance` to Importability page. @danawillow hashicorp/terraform-provider-google#11 hashicorp/terraform-provider-google#12
Add `google_sql_database` and `google_sql_database_instance` to Importability page. @danawillow hashicorp/terraform-provider-google#11 hashicorp/terraform-provider-google#12
* Add import for `google_sql_database_instance` * Alterations suggested by review * Move declaration of array for clarification
* Add import for `google_sql_database_instance` * Alterations suggested by review * Move declaration of array for clarification
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Previously raised as hashicorp/terraform#15235.