Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add option to reattach cdroms #327

Merged
merged 3 commits into from
Jan 9, 2024
Merged

feat: add option to reattach cdroms #327

merged 3 commits into from
Jan 9, 2024

Conversation

tenthirtyam
Copy link
Collaborator

@tenthirtyam tenthirtyam commented Nov 17, 2023

Summary

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

  • Keep a number of CD-ROM devices. Range: 1-4.
  • If set to 0, reattach_cdroma is ignored and the step is skipped.
  • When reattach_cdroms is set to a value in the range, remove_cdrom is ignored and the CD-ROM devices are reattached 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

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Code wise this looks good. But I have mixed feelings about the attribute name.

builder/vsphere/common/step_remove_cdrom.go Outdated Show resolved Hide resolved
@tenthirtyam
Copy link
Collaborator Author

tenthirtyam commented Nov 17, 2023

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
Copy link
Collaborator Author

@akutz what are your thoughts on the parameter name?

@erikgraa
Copy link

erikgraa commented Nov 18, 2023

@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 tenthirtyam marked this pull request as draft November 18, 2023 14:14
@tenthirtyam
Copy link
Collaborator Author

I'll noodle this after the US Holiday week.

@erikgraa
Copy link

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@nywilken
Copy link
Contributor

nywilken commented Dec 7, 2023

@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 tenthirtyam changed the title feat: add option to reset cdrom feat: add option to keep (reset) cdrom Dec 8, 2023
@tenthirtyam tenthirtyam force-pushed the feat/reset-cdrom branch 3 times, most recently from 76a198a to 1c8a86f Compare December 8, 2023 22:20
@tenthirtyam tenthirtyam marked this pull request as ready for review December 8, 2023 22:31
@tenthirtyam tenthirtyam force-pushed the feat/reset-cdrom branch 3 times, most recently from 1a19328 to cb0f42d Compare December 8, 2023 22:45
@tenthirtyam
Copy link
Collaborator Author

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

cc @nywilken @lbajolet-hashicorp

builder/vsphere/common/step_add_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/clone/builder.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_keep_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_keep_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_keep_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_keep_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_keep_cdrom.go Outdated Show resolved Hide resolved
@tenthirtyam tenthirtyam force-pushed the feat/reset-cdrom branch 3 times, most recently from da90efe to 397dd93 Compare December 14, 2023 22:11
@tenthirtyam tenthirtyam requested a review from nywilken December 14, 2023 22:11
@tenthirtyam tenthirtyam force-pushed the feat/reset-cdrom branch 2 times, most recently from 02c1493 to ff82b77 Compare December 15, 2023 02:58
@tenthirtyam
Copy link
Collaborator Author

@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 tenthirtyam changed the title feat: add option to keep (reset) cdrom feat: add option to reattach cdroms Dec 19, 2023
@tenthirtyam
Copy link
Collaborator Author

@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.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice rework. I left a couple of suggestions. Once applied we can merge. #352 will need to be rebased and updated to remove AddCdromCalled from the Reattach tests.

builder/vsphere/common/step_reattach_cdrom.go Outdated Show resolved Hide resolved
builder/vsphere/common/step_reattach_cdrom.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

LGTM

Adds support for to reattach CD-ROMs on the image after the build completes.

Signed-off-by: Ryan Johnson <ryan.johnson@broadcom.com>
@nywilken nywilken merged commit 830cace into main Jan 9, 2024
12 checks passed
@nywilken nywilken deleted the feat/reset-cdrom branch January 9, 2024 20:19
@hashicorp hashicorp locked and limited conversation to collaborators Jun 29, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
builder/vsphere-clone Builder: vsphere-clone builder/vsphere-iso Builder: vsphere-iso enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to reattach CD-ROM device
5 participants