search

hashicorp / packer-plugin-vsphere

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

refactor: only add / remove minimul number of cdroms #355

Closed Hi-Angel closed 5 months ago

Hi-Angel commented 6 months ago

In PR #344 a feature to reuse an existing VM is being added. As part of that feature I had to factor out from AddCdrom the logic of creating a CD-rom as opposed to just mounting an image. Recently merged feature reattach_cdroms turned out to be a new user of the older logic, which made rebasing #344 non-trivial.

In this PR I refactor the reattach_cdroms logic to avoid unnecessarily calling EjectCdroms, RemoveCdroms and AddCdrom by calculating amount of CDroms that we want to have added/removed, and only calling the mentioned functions for that amount (see the last commit).

This is a useful change in itself, disregarding whether #344 exists, so I'm sending it separately from the other PR. And it should simplify rebasing #344 later.

nywilken commented 6 months ago

@Hi-Angel thanks for opening up this quick refactor PR. I was planning to release the vSphere plugin with the latest changes.

Should I wait until this is merged? Or is the refactor for supporting #344

Hi-Angel commented 6 months ago

I don't have any preference here. On one hand, I presume the code that is already in the repo has probably seen more testing than the new one, so regressions are less likely. On the other hand, this refactor reduces a bit calls which (per my understanding) communicate to ESXi via network, which is slow. So if you decide to wait for it to land, the new functional might be a tiny bit faster.

nywilken commented 6 months ago

I don't have any preference here. On one hand, I presume the code that is already in the repo has probably seen more testing than the new one, so regressions are less likely. On the other hand, this refactor reduces a bit calls which (per my understanding) communicate to ESXi via network, which is slow. So if you decide to wait for it to land, the new functional might be a tiny bit faster.

@Hi-Angel that's a good call out on the testing part. We release every week, or shoot for it when we have pending changes. I think we can focus on getting this PR reviewed and merged for the next release to give the new feature time to setting.

Hi-Angel commented 5 months ago

Any news here?

tenthirtyam commented 5 months ago

Any news here?

I've rebased from main and have squashed the commits. I'll review this w/in the next week.

Hi-Angel commented 5 months ago

Any news here?

I've rebased from main and have squashed the commits. I'll review this w/in the next week.

Why have you squashed the commits before review? 😅 I mean, I know the project squashes them on merge for some reason, but don't you want for review purposes to see each functional change separated?

tenthirtyam commented 5 months ago

It's all good - I've read through them already.

Hi-Angel commented 5 months ago

Everything seems addressed

tenthirtyam commented 5 months ago

@tenthirtyam would you like to give this another set of eyes before I merge?

Will review and test as well and update you.

nywilken commented 5 months ago

Hi Folks, apologies I've been out sick. Given that @Hi-Angel has addressed all of your request @tenthirtyam I dismissed your last review in order to unblock the merge and pending rebase. We can revisit anything if needed but this looks good to go.