hashicorp / packer-plugin-vsphere

Packer plugin for VMware vSphere Builder
https://www.packer.io/docs/builders/vsphere
Mozilla Public License 2.0
94 stars 91 forks source link

feat: add option to reattach cdroms #327

Closed tenthirtyam closed 8 months ago

tenthirtyam commented 9 months ago

Summary

Adds support for to reattach CD-ROMs on the image after the build completes without any attached media.

Cherry-picks commits from @powertim in #326 and updates scope, prepare, and tests.

Tests

Standards Tests

packer-plugin-vsphere on  feat/reset-cdrom [✘!?] via 🐹 v1.21.5 
➜ go fmt ./... 
builder/vsphere/clone/config.go
builder/vsphere/common/step_reattach_cdrom.go
builder/vsphere/common/step_reattach_cdrom_test.go
builder/vsphere/iso/config.go

packer-plugin-vsphere on  feat/reset-cdrom [✘!?] via 🐹 v1.21.5 
➜ make generate     
2023/12/19 12:58:13 Copying "docs" to ".docs/"
2023/12/19 12:58:13 Replacing @include '...' calls in .docs/
Compiling MDX docs in '.docs' to Markdown in '.web-docs'...

packer-plugin-vsphere on  feat/reset-cdrom [✘!?] via 🐹 v1.21.5 took 23.3s 
➜ make build        

packer-plugin-vsphere on  feat/reset-cdrom [✘!?] via 🐹 v1.21.5 
➜ make test
?       github.com/hashicorp/packer-plugin-vsphere      [no test files]
?       github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/common/testing       [no test files]
?       github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/examples/driver      [no test files]
?       github.com/hashicorp/packer-plugin-vsphere/version      [no test files]
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/clone        2.844s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/common       3.136s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/driver       7.613s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/iso  1.810s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/supervisor   4.899s
ok      github.com/hashicorp/packer-plugin-vsphere/post-processor/vsphere       1.924s
ok      github.com/hashicorp/packer-plugin-vsphere/post-processor/vsphere-template      1.541s

Builds:

✅ Build with remove_cdrom = true. Expected results: All CD-ROMs removed, reattach_cdroms ignored. ✅ Build with reattach_cdroms = 0 and remove_cdrom = false. Expected results: All CD-ROMs kept, reattach_cdroms ignored. ✅ Build with reattach_cdroms = 1 and remove_cdrom = false. Expected results: One CD-ROM remains. ✅ Build with reattach_cdroms = 1 and remove_cdrom = true. Expected results: One CD-ROM remains. reset_cdrom is superceeded by reattach_cdrom. ✅ Build with reattach_cdroms = 2 and remove_cdrom = false. Expected results: Two CD-ROMs remain. ✅ Build with reattach_cdroms = 2 and remove_cdrom = true. Expected results: Two CD-ROMs remains reset_cdrom is superceeded by reattach_cdroms. ✅ Build with reattach_cdroms = 3 and remove_cdrom = false. Expected results: Three CD-ROMs remain. ✅ Build with reattach_cdroms = 3 and remove_cdrom = true. Expected results: Three CD-ROMs remains reset_cdrom is superceeded by keep_cdrom. ✅ Build with reattach_cdroms = 4 and remove_cdrom = false. Expected results: Four CD-ROMs remain. ✅ Build with reattach_cdroms = 4 and remove_cdrom = true. Expected results: Four CD-ROMs remains reset_cdrom is superceeded by keep_cdrom. ✅ Build with reattach_cdroms = 5 and remove_cdrom = false. Expected results: Caught in Prepare( ).

Reference

Closes #24 Supersedes #326

tenthirtyam commented 9 months ago

Yes, it was an issue for me as well, but I settled on the reset and provided additional details in the comments for the docs. Using remove and keep didn't fit for me because it infers all. Using keep_one felt odd because that would limit the ability to expand the option later, if needed.

Do any other plugins have a similar feature?

tenthirtyam commented 9 months ago

@akutz what are your thoughts on the parameter name?

erikgraa commented 9 months ago

@akutz what are your thoughts on the parameter name?

What about one of (collapse|consolidate|keep|recreate|reset)_cdrom that takes an int with the minimum and maximum of cdroms to keep in case of remove_cdrom = true?

It could default to 0 (and maybe have a a maximum of the number of CD-ROMs that exist on the VM).

For a hypothetical packer VM with 3 CD-ROMs:

remove_cdrom = false & keep_cdrom = 0. Expected results: No CD-ROMs removed. remove_cdrom = true & keep_cdrom = 3. Expected results: No CD-ROMs removed. remove_cdrom = true & keep_cdrom = 1. Expected results: Single CD-ROM remains. remove_cdrom = true & keep_cdrom = 2. Expected results: Two CD-ROMs remain.

tenthirtyam commented 9 months ago

I'll noodle this after the US Holiday week.

erikgraa commented 9 months ago

I'll noodle this after the US Holiday week.

Cool!

Another possibility is to add a "post-shutdown hardware configuration" that would let you alter the overall hardware of the VM before it is made into a template or exported, but I think that's more along the lines of a future feature request :)

tenthirtyam commented 9 months ago

Another possibility is to add a "post-shutdown hardware configuration" that would let you alter the overall hardware of the VM before it is made into a template or exported, but I think that's more along the lines of a future feature request :)

Might be a good post-processor.

tenthirtyam commented 9 months ago

@nywilken @lbajolet-hashicorp - I'm going to pick this one back up this week in my spare time.

nywilken commented 9 months ago

@nywilken @lbajolet-hashicorp - I'm going to pick this one back up this week in my spare time.

Sounds good @tenthirtyam thanks for picking this back up. Feel free to ping when ready for review.

tenthirtyam commented 9 months ago

This pull request is ready for review but is failing for some reason on the Go Validate > Lint step.

cc @nywilken @lbajolet-hashicorp

tenthirtyam commented 8 months ago

@nywilken - I've made the requested changes we've discussed and pushed ff82b77 - let me know what you think.

One option might be to use the property reattach_cdrom.

Ryan

tenthirtyam commented 8 months ago

@nywilken - per our Slack discussion, I've updated this to reflect reattach_cdroms.

`reattach_cdroms` (int) - Reattach one or more configured CD-ROM devices. Range: 1-4. 
  You can reattach up to 4 CD-ROM devices to the final build artifact. 
  If set to 0, `reattach_cdroms` is ignored and the step is skipped.
  When set to a value in the range, `remove_cdrom` is ignored and
  the CD-ROM devices are reattached without any attached media.