builder/vsphere-clone: Find the vm within the folder by rgl · Pull Request #8938 · hashicorp/packer

rgl · 2020-03-23T11:31:17Z

azr

Nice one, LGTM !

nywilken

I think this looks good. I left a question around support for Windows and a suggestion around consolidating the logic into a single method on LocationConfig if this change will be used in additional steps.

builder/vsphere/clone/step_clone.go

nywilken · 2020-03-23T15:05:54Z

builder/vsphere/iso/step_create.go

 func (s *StepCreateVM) Run(_ context.Context, state multistep.StateBag) multistep.StepAction {
 	ui := state.Get("ui").(packer.Ui)
 	d := state.Get("driver").(*driver.Driver)
+	vmPath := fmt.Sprintf("%s/%s", s.Location.Folder, s.Location.VMName)


Same question here regarding the path separator. Since this is happening more than once, what do you think about adding a method on LocationConfig that generates the VMPath, as opposed to doing this in any of the steps that would require the full VM path?

I think its not needed in anywhere else besides these two steps.

Is LocationConfig supposed to represent a Path in itself? if so, maybe it makes sense to put it there, but I'm not sure about its semantics, for example, a FullPath() method would return what? Maybe not only the folder+path, it maybe would also return full a vsphere inventor object path (what they call an ipath), which would be something like /Datacenter/Cluster/folder/vm-name, and that might not work with FindVM. So I'm not sure what to name that method nor its semantics.

Hummm actually returning the full path might be a good idea.

IOW, that can be added in another commit/pr I guess. As right now, this is an implementation detail.

Is LocationConfig supposed to represent a Path in itself?

That's a good question and one that I don't know the answer to off the top of my head. I think holding on this change is a good move for now, and we can revisit in the future if there ever becomes a need.

nywilken

Current Issue on master

⇶  PACKER_LOG=1 PACKER_LOG_PATH=master.vagrant-tempates.log packer build -var-file="variables.json" -var 'gateway_ip=...' -var 'vm_ip=...' -var 'folder=vagrant-templates' -var 'vmname=example' base_packer_ubuntu_clone.json
vsphere-clone: output will be in this color.

==> vsphere-clone: Cloning VM...
Build 'vsphere-clone' finished.

==> Builds finished. The artifacts of successful builds are:
--> vsphere-clone: example

⇶  PACKER_LOG=1 PACKER_LOG_PATH=master.vagrant-tempates-packer.log packer build -var-file="variables.json" -var 'gateway_ip=...' -var 'vm_ip=...' -var 'folder=vagrant-templates/packer' -var 'vmname=example' base_packer_ubuntu_clone.json
vsphere-clone: output will be in this color.

Build 'vsphere-clone' errored: example already exists, you can use -force flag to destroy it

==> Some builds didn't complete successfully and had errors:
--> vsphere-clone: example already exists, you can use -force flag to destroy it

==> Builds finished but no artifacts were created.

Fix using PR build

⇶  PACKER_LOG=1 PACKER_LOG_PATH=8938.vagrant-tempates-packer.log packer.test build -var-file="variables.json" -var 'gateway_ip=... -var 'vm_ip=...' -var 'folder=vagrant-templates/packer' -var 'vmname=example' base_packer_ubuntu_clone.json
vsphere-clone: output will be in this color.

==> vsphere-clone: Cloning VM...
Build 'vsphere-clone' finished.

==> Builds finished. The artifacts of successful builds are:
--> vsphere-clone: example

Expected error for existing vm using PR build

⇶  PACKER_LOG=1 PACKER_LOG_PATH=8938.vagrant-tempates-packer.log packer.test build -var-file="variables.json" -var 'gateway_ip=...' -var 'vm_ip=...' -var 'folder=vagrant-templates/packer' -var 'vmname=example' base_packer_ubuntu_clone.json
vsphere-clone: output will be in this color.

Build 'vsphere-clone' errored: vagrant-templates/packer/example already exists, you can use -force flag to destroy it

Behavior no folder specified using PR build

⇶  PACKER_LOG=1 PACKER_LOG_PATH=8938.empty-folder.log packer.test build -var-file="variables.json" -var 'gateway_ip=... -var 'vm_ip=...' -var 'vmname=example' base_packer_ubuntu_clone.json 
vsphere-clone: output will be in this color.

==> vsphere-clone: Cloning VM...
Build 'vsphere-clone' finished.

==> Builds finished. The artifacts of successful builds are:
--> vsphere-clone: example

⇶  PACKER_LOG=1 PACKER_LOG_PATH=8938.empty-folder-existing.log packer.test build -var-file="variables.json" -var 'gateway_ip=...' -var 'vm_ip=...' -var 'vmname=example' base_packer_ubuntu_clone.json
vsphere-clone: output will be in this color.

==> vsphere-clone: Cloning VM...
Build 'vsphere-clone' errored: The name 'example' already exists.

==> Some builds didn't complete successfully and had errors:
--> vsphere-clone: The name 'example' already exists.

==> Builds finished but no artifacts were created.

jhawk28 · 2020-03-24T05:02:15Z

This new code always adds a "/" on the front of the vm name when calling FindVM() regardless of whether a folder is defined. Does this matter?

nywilken · 2020-03-24T14:28:22Z

This new code always adds a "/" on the front of the vm name when calling FindVM() regardless of whether a folder is defined. Does this matter?

@jhawk28 that's a good question. I thought that would be an issue yesterday when I tested and found that the code is actually splitting the path and manipulating it to find the proper vm name. You can follow the code path on GitHub starting at

packer/builder/vsphere/driver/vm.go

Line 91 in e6368b9

vm, err := d.finder.VirtualMachine(d.ctx, name)

…atching Before change ``` 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata b81800d Merge pull request #8935 from zaventh/feature/start-on-boot 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 d068430 make sure locals are evaluated only once variables are + test this (#8918) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 9075b80 Merge pull request #8891 from rhencke/patch-1 6477d8a Merge pull request #8882 from hashicorp/fix-var-file-hcl 6008f91 Merge pull request #8847 from takaishi/support-keyboard-interactive 5604561 Merge pull request #8877 from paulcichonski/remote-esxi-bastion 698f744 Merge pull request #8887 from hashicorp/untangle_ssh_docs_from_aws aeedc9a Merge pull request #8879 from mbrancato/specify_keyvault_sku 5365fda Merge pull request #8884 from hashicorp/fix_codecov_config 4bd7b14 Merge pull request #8732 from jhawk28/reorder_cdrom_drive 072a71b Merge pull request #8863 from hashicorp/update_go-cty_regex 8a1caaa Merge pull request #8837 from hashicorp/fix_8730 7873cab Merge pull request #8858 from hashicorp/fix_8791 7e382d0 Merge pull request #8828 from mvitaly/fix_8816 8832b3e Merge pull request #8787 from jhawk28/vsphere_iso_multiple_disks 5281740 Merge pull request #8831 from rjhornsby/master e35a872 Merge pull request #8830 from hashicorp/d-var-file-hcl2-not-yet ``` After change ``` ⇶ git log v1.5.4...v1.5.5 --first-parent --oneline --grep="Merge pull request #[0-9]\+" --grep="(#[0-9]\+)$" 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata c387dc2 builder/vsphere-clone: Find the vm within the folder (#8938) b17b211 Add cleanup_remote_cache config option to vmware-iso (#8917) e6368b9 Fix azure winrm_password attribution and allow to set winrm_username (#8928) fcf10e9 Replace Amazon with Outscale for OSC BSU doc (#8944) 9240fb7 Fix typo in title (#8943) 2c6f096 Allow accepting image for the members in OpenStack builder (#8931) b81800d Merge pull request #8935 from zaventh/feature/start-on-boot daffd9c CONTRIBUTING: Update documentation for linting on Travis (#8933) 3a9d356 golangci-lint: Update --new-from-rev option to check only newly added commits (#8923) 97d797d Fix small typos in osc-bsuvolume.html.md (#8926) 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 99b0b98 Add ovf export capability to vsphere builders (#8764) d068430 make sure locals are evaluated only once variables are + test this (#8918) ad8dafa HCL: add tests and fixes around var-file and var args (#8914) 7979ab0 Add after_n_builds to codecov.yml (#8913) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 b94937c Update provisioner_test.go (#8900) 2319521 Add iso config test for checksum from file specific case (#8897) 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter 690bf71 Add Codecov badge and remove report style (#8896) fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 ```

ghost · 2020-04-24T01:50:26Z

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.

find the vm within the folder

9d32ee7

rgl requested a review from a team as a code owner March 23, 2020 11:31

rgl mentioned this pull request Mar 23, 2020

vsphere-iso: vm_name must be unique within the folder and not the entire cluster #8936

Closed

azr approved these changes Mar 23, 2020

View reviewed changes

sylviamoss added builder/vsphere bug labels Mar 23, 2020

nywilken reviewed Mar 23, 2020

View reviewed changes

nywilken approved these changes Mar 23, 2020

View reviewed changes

nywilken changed the title ~~find the vm within the folder~~ builder/vsphere-clone: Find the vm within the folder Mar 24, 2020

nywilken merged commit c387dc2 into hashicorp:master Mar 24, 2020

ghost locked and limited conversation to collaborators Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder/vsphere-clone: Find the vm within the folder#8938

builder/vsphere-clone: Find the vm within the folder#8938
nywilken merged 1 commit intohashicorp:masterfrom
rgl:fix_8936_findvm_within_folder

rgl commented Mar 23, 2020

Uh oh!

azr left a comment

Uh oh!

nywilken left a comment

Uh oh!

Uh oh!

nywilken Mar 23, 2020

Uh oh!

rgl Mar 23, 2020 •

edited

Loading

Uh oh!

nywilken Mar 23, 2020

Uh oh!

nywilken left a comment •

edited

Loading

Uh oh!

jhawk28 commented Mar 24, 2020

Uh oh!

nywilken commented Mar 24, 2020 •

edited

Loading

Uh oh!

ghost commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rgl commented Mar 23, 2020

Uh oh!

azr left a comment

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nywilken Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

rgl Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhawk28 commented Mar 24, 2020

Uh oh!

nywilken commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rgl Mar 23, 2020 •

edited

Loading

nywilken left a comment •

edited

Loading

nywilken commented Mar 24, 2020 •

edited

Loading