vsphere-provider: Added folder handling for folder-qualified vm names #3939
+814
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@matt-deboer looks good. Would you be able to provide an acceptance test with a new test case? Acceptance test are here: https://github.com/hashicorp/terraform/blob/master/builtin/providers/vsphere/resource_vsphere_virtual_machine_test.go |
|
@matt-deboer are you able to add acceptance testing?? Would be much better to test this code on various vsphere systems before the terraform folks approve this pull. |
|
yeah, I've added. just preparing to ammend the PR; should be sometime today On Thu, Nov 19, 2015 at 1:12 PM, Chris Love notifications@github.com
|
|
acceptance test is uploaded--please take a look |
|
@matt-deboer should have asked sooner. Should we update the documentation? |
| "base_name": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| ForceNew: false, |
There was a problem hiding this comment.
No need to explicitly call out ForceNew: false here as it's the default.
|
Hi @matt-deboer - thanks so much for this! 👍 I dropped a few comments inline for you. I also have a higher level question: I am wondering if So sketching out what that might look like in Terraform: # Ensures the folder exists
resource "vsphere_folder" "test" {
datacenter = "mydc"
path = "terraform-test-folder"
}
# Places the VM into the folder as it creates it
resource "vsphere_virtual_machine" "test" {
datacenter = "mydc"
name = "myvm"
folder = "${vsphere_folder.test.path}"
}Not sure if this makes sense from a user perspective. I saw the Let me know what you think! |
|
@phinze: Thanks for the notes. Yes, I think like the new top-level resource option; it's cleaner. |
|
@tkak any comments on these changes? |
|
@chrislovecnm @phinze : I'm hitting some interesting issues with the acceptance tests; I'm getting "A specified parameter was not correct" regarding "spec.identity.hostName" before the vm is finished being created (based on the log output); what is the trigger that tells TF to consider a resource "created"? It seems whatever that is, it may be firing too soon... |
|
can I get some debug logs? |
|
sure: here you go: notice that around the line containing with "new vm: {VirtualMachine", TF determines the problem with spec.identity.hostName, but this is only a partially-created machine... |
|
user error : ) I had named some test vms with "_" in the name (invalid for hostname). |
|
@matt-deboer 😀 we have all done that before. Appreciate your work on this!!!! |
|
I think this is in a stable state now for review;
Please take a look when you can. |
|
@matt-deboer 👍 thanks |
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
Is existing_path meant to be user settable? If not you should omit Optional so it's treated as read-only.
|
@phinze thanks for the tip; removed the 'optional', and rebased with upstream |
| context.TODO(), fmt.Sprintf("%v/vm/%v", d.Get("datacenter").(string), | ||
| d.Get("path").(string))) | ||
|
|
||
| if err != nil || folder == nil { |
There was a problem hiding this comment.
Is there any way to scope this error check down to a "NotFound" error and return the error in other cases?
If we treat any error here as though the resource was deleted, running a terraform plan with e.g. internet disconnected could result in state losing track of all its folders instead of the error message you'd expect in that case.
|
Okay nearly there! One more comment for you and after we address that I think this thing is ready to go! 🚀 |
Added acceptance test for creation in folders Added 'baseName' as computed schema attribute for convenience Added 'base_name' computed attribute for convenience Added new vsphere folder resource Fixed folder behavior Assure test folders are properly removed Avoid creating recreating search index in loop Fix typeo in vsphere.createFolder Updated website documentation Renamed test folders to be unique across tests Fixes based on acc test findings; code cleanup Added combined folder and vm acc test Restored newline; fixed skipped acc tests Marked 'existing_path' as computed only Removed debug logging from tests Changed folder read to return error
|
@phinze Okay; updated now. It turns out that a normal failed lookup simply returns nil object and nil error, so I think we're safe to return any non-nil error. |
|
Nice! Great work on this, @matt-deboer 👍 |
phinze
added a commit
that referenced
this pull request
Dec 9, 2015
vsphere-provider: Added folder handling for folder-qualified vm names
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this update, the builtin vsphere provider will now support a new top-level resource type called vsphere_folder which can be leveraged in the form of a "folder" attribute on a vsphere_virtual_machine resource--causing the vm to be created within the specified folder path.
Example:
An additional attribute "existing_path" is computed on the vsphere_folder resource to track any folder segments that were pre-existing upon initial folder creation--to ensure that such pre-existing folder segments are not removed when the terraform resource is destroyed.
This is fix/enhancement supports #3921.