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

provider/google: add attached_disk field to google_compute_instance #13443

Merged
merged 2 commits into from
Apr 25, 2017

Conversation

danawillow
Copy link
Contributor

This adds the minimum functionality for an attached_disk field.

This PR is an alternative to #12398. Instead of allowing attaching/detaching of all disks and having to compare them field-by-field to determine whether a disk is the same, this field would allow only persistent disks that have been defined elsewhere in terraform to be attached/detached. This encourages better practices with terraform actually managing state. For the other solution, if a persistent disk that was created via the disks field is detached, then the disk disappears from terraform's knowledge. With a separate field, we can only allow detaching disks that terraform knows about.

Actually allowing attaching/detaching will come in a follow-up PR. This one was kept small for the sake of clarity and ease of reviewing.

}
if disk.DiskEncryptionKey != nil && disk.DiskEncryptionKey.Sha256 != "" {
di["disk_encryption_key_sha256"] = disk.DiskEncryptionKey.Sha256
if i < disksCount {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the disks always get returned in the order they're attached? This feels brittle. Is there no way to do this by e.g. checking if the disk's source exists in d under attached_disk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

(technically we're still relying on the fact they're returned in the same order by mixing disk.[property] and d.Get([property]) calls, but your way does certainly look less hacky)

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merge at will. 👍 :) Thanks @danawillow!

@ghost
Copy link

ghost commented Apr 13, 2020

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.

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

Successfully merging this pull request may close these issues.

2 participants