-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add datastore cluster support to datastore resources #439
Add datastore cluster support to datastore resources #439
Conversation
Adding the datastore_cluster_id attribute to the vsphere_nas_datastore resource, allowing the deployment of a datastore to a datastore cluster.
We are moving some resource helpers that are shared between NAS and VMFS datastore resources here, so a more general name is needed, as it's not just for summary anymore.
This adds the datastore_cluster_id attribute to the VMFS datastore resource, allowing one to add a VMFS datastore resource to a datastore cluster. In addition to this, a bit of refactoring was done to remove some unused error messages, and adding additional disks during creation was moved up in precedence so that the ID could be set earlier, removing the need for rollback on folder moves and ID/custom attribute modification.
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.
Code-wise looks pretty good! Few minor nitpicks that you can choose to address or not.
// resource schema) | ||
// * Type (redundant attribute as the datastore type will be represented by | ||
// the resource) | ||
"accessible": &schema.Schema{ |
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.
Minor nitpick: the &schema.Schema
here are extraneous since its defined above in map[string]*schema.Schema
d.Set("uncommitted_space", structure.ByteToMB(obj.Uncommitted)) | ||
d.Set("url", obj.Url) | ||
|
||
// Set the name attribute off of the name here - since we do not track this |
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.
👍
vsphere/datastore_structure.go
Outdated
path = fvalue.(string) | ||
case cok: | ||
var err error | ||
path, err = resourceVSphereDatastoreStorageClusterPathNormalized(meta, cvalue.(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.
Minor nitpick: Seems like we can just return resourceVSphereDatastoreStorageClusterPathNormalized(meta, cvalue.(string))
here
ConflictsWith: []string{"datastore_cluster_id"}, | ||
StateFunc: folder.NormalizePath, | ||
}, | ||
"datastore_cluster_id": &schema.Schema{ |
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.
Minor nitpick: extraneous &schema.Schema
ConflictsWith: []string{"datastore_cluster_id"}, | ||
StateFunc: folder.NormalizePath, | ||
}, | ||
"datastore_cluster_id": &schema.Schema{ |
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.
Minor nitpick: extraneous &schema.Schema
@@ -199,8 +157,35 @@ func resourceVSphereVmfsDatastoreCreate(d *schema.ResourceData, meta interface{} | |||
} | |||
} | |||
|
|||
// Set the ID here now as most other issues here can be applied on an update, |
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.
💯 🥇 👍
Thanks @bflad! I've made the change mentioned in Merging now! |
This adds support for datastore clusters to both
vsphere_nas_datastore
andvsphere_vmfs_datastore
. They share the same attribute and common behaviour,datastore_cluster_id
, which stands in forfolder
when used. As a result, the attribute conflicts with folder - as datastore clusters are containers in of themselves, placement in one or the other is mutually exclusive.Docs have been updated as well, tests have been added, and the
datastore_summary_structure.go
file has been renamed todatastore_structure.go
so that it can hold shared functionality for determining what attribute betweenfolder
anddatastore_cluster_id
is in use at any particular time.