Allow attaching and detaching disks from instances

[danawillow]

Third time's the charm? Fixes #33.

[danawillow]

$ make testacc TEST=./google TESTARGS='-run=TestAccComputeInstance_attachedDisk -parallel 8'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeInstance_attachedDisk -parallel 8 -timeout 120m
=== RUN   TestAccComputeInstance_attachedDisk
=== RUN   TestAccComputeInstance_attachedDisk_sourceUrl
=== RUN   TestAccComputeInstance_attachedDiskUpdate
--- PASS: TestAccComputeInstance_attachedDisk_sourceUrl (71.67s)
--- PASS: TestAccComputeInstance_attachedDisk (72.10s)
--- PASS: TestAccComputeInstance_attachedDiskUpdate (152.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	152.576s

[catsby]

Hi there! Drive-by thoughts:

• is the ordering of attached_disk relevant? If so, TypeList is correct because we want the order of the config to be enforced, and reading from the API with a different order should trigger a diff. If not, we should use TypeSet for them; their key will be the hash and not their spot in line, if that makes sense. I realize the usage of TypeList is not part of this PR, but it's relevant to my next point..
• Continuing down that path, Set’s offer an advantage with the Difference() method. See Set#Difference and usage example in AWS Security Groups UPDATE method. It seems in this code we’re doing a similar diff operation, and using Sets would greatly simply it for you
• I see in the READ method (understandably not being changed here) there seems to be a notion of updating existing disks in config and also adding extra disks and storing them in attached_disk. It seems there is an issue here, in that we’re not completely overwriting attached_disk with what we find in the API results. Example, if there is a change and a disk Terraform thinks is attached (it’s in the config) is removed outside of Terraform, does Terraform notice on refresh? There doesn’t seem to be any part in the READ that’s removing disks not found. If we simply built up the list from the API and wrote those results, regardless of what we had in the config, we could detect that drift. Again, I realize this is outside of the current PR, but I noticed it.

[paddycarver]

Silly question: does this take scratch disks into account?

[danawillow]

I don't think it would have done anything unexpected since we're only using it to check whether a disk in state is still attached, but may as well do the check just in case. Done.

[paddycarver]

This isn't the previous config, it's the state. Terraform does not version configs itself, and so cannot tell you what was in the previous config.

[danawillow]

Fixed.

[paddycarver]

I see in the READ method (understandably not being changed here) there seems to be a notion of updating existing disks in config and also adding extra disks and storing them in attached_disk. It seems there is an issue here, in that we’re not completely overwriting attached_disk with what we find in the API results. Example, if there is a change and a disk Terraform thinks is attached (it’s in the config) is removed outside of Terraform, does Terraform notice on refresh? There doesn’t seem to be any part in the READ that’s removing disks not found. If we simply built up the list from the API and wrote those results, regardless of what we had in the config, we could detect that drift. Again, I realize this is outside of the current PR, but I noticed it.

From what I can see, the disks are being properly set based on the API response. That code should detect drift of:

• Disks set in the config but not set on the API
• Disks set in the API but not set in the config

I think the confusion may be around the extraAttachedDisks variable, which--as I understand it--exists to work around attached_disk being a List and not a Set, but I could be wrong on that. Essentially, the code looks to be ordering disks in the order they're in the config, with any disks not in the config appended to the end.

[danawillow]

Yeah, having attached_disks as a Set would definitely simplify things, especially with that Difference method. I'll add that in a new PR and then come back to this one.

[danawillow]

Paddy and I talked this over, and we're not going to try to make attached_disks into a set before this change, since it's a fair amount more work than expected, a breaking change, and not currently breaking anyone. See #656 for details.

[catsby]

@danawillow 👍

[catsby]

👍

[ghost]

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!