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 functionality to specify and mount bootable vmdk #6146

Merged
merged 1 commit into from Apr 27, 2016

Conversation

dkalleg
Copy link
Contributor

@dkalleg dkalleg commented Apr 13, 2016

User may specify a bootable_vmdk_path in their disk definition. This assumes that a bootable vmdk is uploaded to an accessible datastore in vSphere. This does not copy the vmdk prior to starting the vm to preserve state; this, however could be a future enhancement.
The options size, template, and bootable_vmdk_path are considered to be mutually exclusive.
Todo: Enforce mutual exclusivity, validate the bootable_vmdk_path

@@ -268,6 +270,14 @@ func resourceVSphereVirtualMachine() *schema.Resource {
Optional: true,
ForceNew: true,
},

"bootable_vmdk_path": &schema.Schema{
// TODO: Add ValidateFunc to check if path exists
Copy link
Contributor

Choose a reason for hiding this comment

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

How critical is this??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure its a 'good to have' but not a blocker for my first stab. There aren't checks for things like network label / folder / datacenter / ect, which we could do, but just aren't at the yet. I figure its a low hanging fruit to call out.

@chrislovecnm
Copy link
Contributor

@dkalleg first off thanks for your help!!! You want to email me at clove at consulting dot net? I can provide a bit of guidance.

@chrislovecnm
Copy link
Contributor

Geeze type your email correctly ...

clove
At
cnmconsulting
Dot
net

@dkalleg
Copy link
Contributor Author

dkalleg commented Apr 20, 2016

@kristinn I'd appreciate hearing any feedback you had regarding this PR since tkak and phinze are so busy.

} else {
return fmt.Errorf("Size argument is required.")
return fmt.Errorf("Size or bootable_vmdk_path argument is required.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps create an anonymous function for this and get rid of the code duplication.

@kristinn
Copy link
Contributor

@dkalleg Looks very good, had just two comments. 👍

User may specify a vmdk in their disk definition.
The options size, template, and vmdk are considered
to be mutually exclusive. User may also set whether each disk
associated with the vm should try to boot after creation.
Todo: Enforce mutual exclusivity, validate the bootable_vmdk_path
@dkalleg
Copy link
Contributor Author

dkalleg commented Apr 26, 2016

@chrislovecnm @kristinn @phinze @tkak Made some adjustments to make this a little more versatile/generic. Instead of focusing on just bootable vmdks this is now focusing on mounting any existing vmdk with the option of specifying a disk as bootable. For any vm resource, the user will list their bootable disk (whether its a template or an existing vmdk) as the first positional disk (which was an existing assumption). Terraform won't try to boot from a non-template disk unless the user also defines the disk with bootable = true. This is preserving the no-boot use case when the user may specify a new disk or a non-bootable disk as the first positionally defined disk. Also preserving the attempt-to-boot of a template.

@@ -455,6 +455,68 @@ func TestAccVSphereVirtualMachine_createWithCdrom(t *testing.T) {
})
}

func TestAccVSphereVirtualMachine_createWithExistingVmdk(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do love it when I see all of the plus signs that add a unit test. Cookie for u!!!

@chrislovecnm
Copy link
Contributor

@stack72 @jen20 @phinze LGTM - may we have a merge good sirs??

@stack72 stack72 merged commit 6f62248 into hashicorp:master Apr 27, 2016
@chrislovecnm
Copy link
Contributor

@stack72 - Paul you ROCK!!

xsellier pushed a commit to xsellier/terraform that referenced this pull request May 17, 2016
User may specify a vmdk in their disk definition.
The options size, template, and vmdk are considered
to be mutually exclusive. User may also set whether each disk
associated with the vm should try to boot after creation.
Todo: Enforce mutual exclusivity, validate the bootable_vmdk_path
@ghost
Copy link

ghost commented Apr 26, 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.

@hashicorp hashicorp locked and limited conversation to collaborators Apr 26, 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.

None yet

4 participants