-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
provider/google: Add import support to google_sql_user #14457
provider/google: Add import support to google_sql_user #14457
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.
Question about a possibly errant SetId
log.Printf("[WARN] Removing SQL User %q because it's gone", d.Get("name").(string)) | ||
d.SetId("") | ||
|
||
return nil | ||
} | ||
|
||
d.SetId(name) | ||
d.SetId(fmt.Sprintf("%s.%s", 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.
It's generally not a good idea to call SetId
outside of a CREATE
method. I realize this isn't your addition, but do you know off hand why this is here? Can it be removed and have the resource work as expected?
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.
You're right - this was able to be safely removed. 🎉
@@ -77,6 +83,8 @@ func resourceSqlUserCreate(d *schema.ResourceData, meta interface{}) error { | |||
"user %s into instance %s: %s", name, instance, err) | |||
} | |||
|
|||
d.SetId(fmt.Sprintf("%s.%s", 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.
So I take back part of what I told you in person. We shouldn't change the format of the ID, since that will break refreshing (since the ID in state will still be the old version). At this point we have three possible solutions:
- Write a migration function to change all the IDs in state. I think it would work, but migration functions are kind of a pain.
- Do the ID parsing in the StateFn, so instead of doing a
schema.ImportStatePassthrough
, create a custom function that parses the ID and then sets the two properties in the state before it gets passed to the read fn. - Create a custom function that reads all available instances, and then tries each one to see if the user belongs to that instance (see resource_pagerduty_service_integration for an example). This is actually less bad than I initially made it out to be. Since Terraform is currently storing the user name as the ID, it's already not allowing multiple users with the same name in different instances. And so a naive import should fail in this case anyway. However the point could be made that Terraform should allow users with the same name in different instances.
I'm not sure yet which the best solution is. I'll consult with Hashicorp people and get back to you.
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.
All right, the experts have spoken and recommended solution 1. I can explain migration fns to you once I'm in the office, or feel free to give it a try on your own (I actually don't think it'll be much of a pain, this will be a pretty straightforward one)
instance := d.Get("instance").(string) | ||
instanceAndName := strings.SplitN(d.Id(), ".", 2) | ||
if len(instanceAndName) != 2 { | ||
return errors.New( |
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.
fmt.Errorf :D
This is great to see! I started adding importability for Also: I think you'll want to add |
func resourceSqlUserMigrateState( | ||
v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { | ||
if is.Empty() { | ||
log.Println("[DEBUG] Empty SqlUserState; nothing to migrate.") |
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.
Actually InstanceState is a terraform type so you do want InstanceState here instead of SqlUserState
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.
Done.
"name": "tf-user", | ||
"instance": "tf-instance", | ||
}, | ||
Expected: map[string]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.
Why is Expected
empty?
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.
Done.
@@ -95,32 +104,41 @@ func resourceSqlUserRead(d *schema.ResourceData, meta interface{}) error { | |||
return err | |||
} | |||
|
|||
name := d.Get("name").(string) | |||
instance := d.Get("instance").(string) | |||
instanceAndName := strings.SplitN(d.Id(), ".", 2) |
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'm slightly nervous about using dots as a separator. Even though this set of properties is safe, I can imagine a resource that needs to be imported in a similar way but dots are acceptable characters in names for both types of resources, and then we would have an inconsistency between resources. I think a "/" would be safer, since I'd be really surprised if it ever came up as a separator (because of weirdness in self_links). What do you think?
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.
This makes sense - "/" is probably a safer character to use than ".".
Done.
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Add support for "terraform import" with google_sql_user resources.
This uses a slightly different format than normal to support specifying the database instance name. The format is documented on the resource's page, along with a note that passwords are not retrieved