-
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
Make google_container_node_pool resources importable. #284
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.
Hi,
Thanks for your contribution.
Can you update the documentation for the google_container_node_pool here:
https://github.com/terraform-providers/terraform-provider-google/blob/master/website/docs/r/container_node_pool.html.markdown
To add a section about Import. Here is an example:
https://github.com/terraform-providers/terraform-provider-google/blob/master/website/docs/r/compute_disk.html.markdown
d.Set("zone", parts[0]) | ||
d.Set("cluster", parts[1]) | ||
d.Set("name", parts[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.
Hum...
The Create
method sets the id of the resource to the name of the container node pool.
The import will leave the id as zone/cluster/name.
This is not a big deal since we don't use the resource id after creation right now but if we decide to use it in the future, it is nice to have them consistent.
However, only setting the id to name like it's done in Create
is wrong since it is valid for two node pool across two clusters to have the same name. Currently, we prevent this use case.
I created a separate issue (#298) to fix the Create
method and set the id to zone/cluster/name. This will require a use of schema migration and it is better to do this in a separate PR. Migration will make all the ids consistent.
Nothing to do for you here :) I will take care of the other issue.
I added documentation for the import :) |
Thank you for your contribution |
…rp#278)" (hashicorp#284) This reverts commit 09df2a6, which contains 0.12-alpha4, which is broken in surprising ways.
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! |
Fixes part of #165 by making google_container_node_pool resources importable.