Skip to content
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

google_compute_instance.attached_disks should be a Set #656

Closed
danawillow opened this issue Oct 31, 2017 · 6 comments
Closed

google_compute_instance.attached_disks should be a Set #656

danawillow opened this issue Oct 31, 2017 · 6 comments

Comments

@danawillow
Copy link
Contributor

Since these can easily be reordered in a config, it makes sense for them to be a Set. Having attached_disks as a set would also make the update logic added in #636 simpler.

Because we support adding attached disks both by name and by self link but only store the self link in state, we can't easily add logic to make it a set. On plan, the new hash would be created based on the name, but when it's stored in state it would be stored based on a hash of the self link, leading to a perma-diff.

This change will come in two parts:

  1. Update the logic so we store the value of source in the same format (name vs self link) that it was created in, if it's a resource that was created via terraform.
  2. Change attached_disks into a set, using the source value to calculate the hash. This is a breaking change, since disks that have already been created with the name will have a config with the name but a state file with the self link, and there's no way to differentiate this case from one where the disk was created with the source url.

The first part can be done now, the second will wait until we're ready for 2.0.0.

@catsby
Copy link
Contributor

catsby commented Nov 2, 2017

Can you share some documentation links on what a "self link" is? I've seen/heard of this but am not 100% sure what they are or their properties.

@danawillow
Copy link
Contributor Author

I don't think there's any documentation on self links or their properties, but they're (as far as I know) a link such that when you do a GET on it, it returns the resource. They take a common shape that depends on whether the resource is global, regional, or zonal.

For example, a disk named "test-disk" might have a self link of https://www.googleapis.com/compute/v1/projects/MY_PROJECT/zones/us-central1-a/disks/test-disk.

When referring to resources from other resources in GCP, the API often does so in the form of a self link: https://cloud.google.com/compute/docs/reference/latest/instances/attachDisk. The "source" field asks for "a valid partial or full URL to an existing Persistent Disk resource". In Terraform, we sometimes require the users to specify the self link but also allow them to specify just the resource name (because if the resource isn't managed by Terraform the user would have to write it out in their config and it's way simpler for the user to just write the name) and then extrapolate the self link from there.

Does that make sense? Anything else you're curious about regarding self links?

@paddycarver paddycarver added this to the 2.0.0 milestone Mar 8, 2018
@lubars
Copy link

lubars commented Jun 5, 2018

For the purpose of creating logical volume groups, I'd like to use count in google_compute_disk to specify the number of disks per google_compute_instance, but am currently unable to because each disk requires its own attached_disk, so the number needs to be known ahead of time. Would treating attached disks as a set let me attach all (or a subset) of my google_compute_disk with a single attached_disk block?

@evralston
Copy link

Lubars: What you describe is more like hashicorp/terraform#7034 . This issue has to do with Terraform insisting on an order for the disk objects, which does not match how the GCE API handles them.

@paddycarver paddycarver modified the milestone: 2.0.0 Sep 30, 2018
@danawillow
Copy link
Contributor Author

This can't be done for the same reason as node pools in #780: it's a nested object, and attached_disk.device_name is both optional+computed.

@ghost
Copy link

ghost commented Jan 21, 2019

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!

@ghost ghost locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants