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

Add datastore cluster support to datastore resources #439

Merged
merged 5 commits into from
Mar 23, 2018

Conversation

vancluever
Copy link
Contributor

This adds support for datastore clusters to both vsphere_nas_datastore and vsphere_vmfs_datastore. They share the same attribute and common behaviour, datastore_cluster_id, which stands in for folder 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 to datastore_structure.go so that it can hold shared functionality for determining what attribute between folder and datastore_cluster_id is in use at any particular time.

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.
@vancluever vancluever requested a review from a team March 21, 2018 03:46
@vancluever vancluever added the enhancement Type: Enhancement label Mar 21, 2018
Copy link
Contributor

@bflad bflad left a 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{
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

path = fvalue.(string)
case cok:
var err error
path, err = resourceVSphereDatastoreStorageClusterPathNormalized(meta, cvalue.(string))
Copy link
Contributor

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{
Copy link
Contributor

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{
Copy link
Contributor

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 🥇 👍

@vancluever
Copy link
Contributor Author

Thanks @bflad! I've made the change mentioned in datastore_cluster.go. I'm aware of the extraneous type declarations and will probably fix them in a future code cleanup, just going to keep them that way for now to be consistent, even if it's extraneously consistent. 😛

Merging now!

@vancluever vancluever merged commit 1c7ac11 into master Mar 23, 2018
@vancluever vancluever deleted the f-datastore-cluster-support-for-datastores branch March 31, 2018 17:41
@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants