Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix for datastores with spaces in their name. #108

Closed
wants to merge 4 commits into from
Closed

Fix for datastores with spaces in their name. #108

wants to merge 4 commits into from

Conversation

angelichorsey
Copy link

@angelichorsey angelichorsey commented Jul 25, 2017

Three split functions in the vsphere virtual machine resource split on space which breaks functionality when the datastore has a space in its name. Considering this split only needs the second element, which is the vmdk path or the folder it resides in, a split on "] " will always correctly create the second element needed in the slice/array (still haven't figured out the exact difference between these in Go).

I am new to contributing to github and I am new to Golang. I attempted to modify the acceptance tests, but it appears the datastores are stubbed with local environment variables. I can't think of any modifications to acceptance testing in this way. Please direct me otherwise needed.

@grubernaut grubernaut added the bug Type: Bug label Jul 25, 2017
@jwieringa
Copy link

jwieringa commented Aug 2, 2017

I also work with a datastore configured with a space in the name. I've applied this patch and seeing successful launches using Terraform v0.10.0-rc1.

@vancluever
Copy link
Contributor

Hey @angelichorsey - thanks for the PR!

What I would actually prefer to see here is that we don't use this method at all, and change it to read the path (and other relevant information) using API.

govmomi offers a utility function to achieve this - DatastorePath.FromString seems to be able to do this in a clean fashion, without the need for string parsing on our side.

Would you be able to adjust your PR to work with this?

Thanks!

@vancluever vancluever added the waiting-response Status: Waiting on a Response label Aug 23, 2017
@angelichorsey
Copy link
Author

I don't know if I would be able to do it anytime soon, but I can certainly take a look. It seems like if you want that then a lot of the provider code should be reworked.

Which is still a good thing. Thanks for showing me that utility. Is that function in the currently used version of govmomi?

@vancluever
Copy link
Contributor

vancluever commented Aug 23, 2017

@angelichorsey I actually agree that the VM resource could do with some refactoring, and it's something I would love to do or seen done. In the meantime though, I would like to see any updates to the resource (and the provider in general) lean on govmomi functionality whenever possible.

In regards to the govmomi version we are using - we recently updated to v0.15.0 (which is now available in the v0.2.0 release). If you don't see it, you might need to rebase on master to get the most recent commits. I did check against the v0.15.0 tag and it seems to be there.

Thanks!

@angelichorsey
Copy link
Author

I still don't have much know how or time to work on this. The split option was easy to fix when I saw it. Can you guys add this as a bug to be fixed in the latest version? The vsphere provider fails when datastores have spaces.

@vancluever
Copy link
Contributor

Hey @angelichorsey, the new version of the resource is in with #244 and should address this, as it moves all FileName parsing to the previously mentioned DatastorePath object.

Going to close this one now as a result. Thanks for your work on this!

@vancluever vancluever closed this Nov 18, 2017
@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
bug Type: Bug waiting-response Status: Waiting on a Response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants