vSphere custom vm params #3867
vSphere custom vm params #3867
Conversation
…vecnm/terraform into vsphere-custom-vm-params
|
@phinze might you take a look? Would appreciate getting this into master and figuring out when it will be released. Like to coordinate release schedules. |
| @@ -51,6 +51,7 @@ IMPROVEMENTS: | |||
| * provider/digitalocean: Make user_data force a new droplet [GH-3740] | |||
| * provider/vsphere: Do not add network interfaces by default [GH-3652] | |||
| * provider/openstack: Configure Fixed IPs through ports [GH-3772] | |||
| * provider/openstack: Specify a port ID on a Router Interface [GH-3903] | |||
There was a problem hiding this comment.
I don't spot any changes to the openstack provider in this pull request - is this a remnant of another branch? In general we'd prefer it if pull requests didn't update the CHANGELOG, as it tends to lead to frequent merge conflicts. Could you remove this from the PR?
There was a problem hiding this comment.
Can do ... it is from updating from the master
|
Hi @chrislovecnm! I've looked over the code and left a few comments inline. I have no vSphere environment here, however, so I'm relying on the acceptance tests being correct. |
|
@jen20 will work on the two comments. Lots of testing done, and found a couple more bugs when you hit an ipv6 address. |
|
@jen20 so now the build is failing on files that I have not changed ... Joy 😄 I addressed your comments. Lots of testing done |
|
@jen20 merged in upstream master, and now build is happy. We good with a merge now? |
| resource.TestCheckResourceAttr( | ||
| "vsphere_virtual_machine.car", "custom_configuration_parameters.car", "ferrai"), | ||
| resource.TestCheckResourceAttr( | ||
| "vsphere_virtual_machine.car", "custom_configuration_parameters.num", "42"), |
There was a problem hiding this comment.
These helpers are only checking the state file, so right now no verification is being done on the custom parameters.
In other words, if you remove all your code but the addition of the field to the schema, this test will still pass!
I'd recommend writing a CheckFunc that accepts &vm and inspects it to prove the custom attributes get set.
Something along these lines:
func testAccCheckVSphereVirtualMachineCustomConfigs(vm *virtualMachine) resource.TestCheckFunc {
return func(s *terraform.State) error {
// use the vmware client to ensure that the passed in vm has the expected custom config
// return an error if not
}
}There was a problem hiding this comment.
This is exactly what I was asking about in another ticket 😄 Will do!!
|
Hi @chrislovecnm - looks like the acceptance test could use a bit more work here. Details inline. Also could you rebase and squash this branch against the latest master? If not we can take care of that fore you at merge time. 👍 |
|
@phinze I will try a rebase again.... It turned into one of those rebases 😆 |
|
@phinze acceptance test improved. All tests are passing. Comments? |
|
Hey @chrislovecnm - sorry for the delay! Taking a look now |
|
LGTM! |
|
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
This provides support for #3834
Adding new map parameter that contains key value pairs that are stored to ExtraConfig. This provides the capability to store metadata within a virtual machine.
Acceptance test have been provided.