Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

fix: use cdroms len instead of ISOPaths len #394

Merged
merged 4 commits into from
Apr 2, 2024
Merged

fix: use cdroms len instead of ISOPaths len #394

merged 4 commits into from
Apr 2, 2024

Conversation

Hi-Angel
Copy link
Contributor

@Hi-Angel Hi-Angel commented Mar 21, 2024 •
edited
Loading

  1. st commit improves code coverage by factoring out a function to list cdrom devices. Although, there's very little change in the test from that, bug I guess it's good to have a better testing.
  2. nd commit is the fix. As described in the commit, previously we used ISOPaths to figure out amount of cdroms, but that's not really correct and this assumption breaks when remove_cdrom is set. Fix the problem by making use of the function listing the devices that was added in prev. commit.
  3. rd commit is just a fix to the tests that nils out the list of ISOPaths added in EjectCdrom, which seems to be correct because after it's called we should no longer have paths mounted, only the actual cdrom hw devices. With that said, it seems this does not have any influence on the tests right now.

Fixes: #393

@Hi-Angel Hi-Angel requested a review from a team as a code owner March 21, 2024 14:50
@Hi-Angel Hi-Angel marked this pull request as draft March 21, 2024 14:50
@tenthirtyam tenthirtyam added this to the v1.2.6 milestone Mar 21, 2024
@Hi-Angel
common: factor out a function to list cdrom devices
4093c35 
It's a pattern that re-appears in the code, let's deduplicate that.
Hi-Angel added 2 commits March 22, 2024 12:43
@Hi-Angel
common: use cdroms len instead of ISOPaths len
1cd12f9 
There's special case when option `remove_cdrom` is set that there
would be zero cdrom devices by the time `reattach_cdroms` processing
is executed, so the comparison to ISOPaths len is wrong. Instead use
the amount of cdroms we have at that point.

Fixes: hashicorp#393
@Hi-Angel
driver/vm_mock: nil-out added ISO paths in EjectCdroms
1166938
@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Mar 22, 2024 •
edited
Loading

Okay, ready for review.

  1. st commit improves code coverage by factoring out a function to list cdrom devices. Although, there's very little change in the test from that, bug I guess it's good to have a better testing.
  2. nd commit is the fix. As described in the commit, previously we used ISOPaths to figure out amount of cdroms, but that's not really correct and this assumption breaks when remove_cdrom is set. Fix the problem by making use of the function listing the devices that was added in prev. commit.
  3. rd commit is just a fix to the tests that nils out the list of ISOPaths added in EjectCdrom, which seems to be correct because after it's called we should no longer have paths mounted, only the actual cdrom hw devices. With that said, it seems this does not have any influence on the tests right now.

Please review 😊

@Hi-Angel Hi-Angel changed the title wip: common: use cdroms len instead of ISOPaths len common: use cdroms len instead of ISOPaths len Mar 22, 2024
@Hi-Angel Hi-Angel marked this pull request as ready for review March 22, 2024 09:46
@Hi-Angel Hi-Angel mentioned this pull request Mar 22, 2024
Closed
@tenthirtyam tenthirtyam requested a review from nywilken March 22, 2024 14:09
@tenthirtyam tenthirtyam changed the title common: use cdroms len instead of ISOPaths len fix: use cdroms len instead of ISOPaths len Mar 22, 2024
tenthirtyam
tenthirtyam reviewed Mar 28, 2024
View reviewed changes
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'll give these changes and some common and corner cases a spin in my environments tomorrow and report back the results so we can move forward.

@tenthirtyam
docs: add documentation output
44c6db8 
Adds the documentation for `reattach_cdroms` to the generated documentation.

Signed-off-by: Ryan Johnson <[email protected]>
@tenthirtyam tenthirtyam self-requested a review April 2, 2024 15:24
tenthirtyam
tenthirtyam approved these changes Apr 2, 2024
View reviewed changes
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Tested with multiple cases and variations and the results were as expected.

I've also added a commit to attach the option into the generated documentation.

@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Apr 2, 2024

Cool, thank you!

nywilken
nywilken approved these changes Apr 2, 2024
View reviewed changes
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.

Nicely done - the changes look good to me. Thanks for validating @tenthirtyam

@nywilken nywilken merged commit bc09f66 into hashicorp:main Apr 2, 2024
12 checks passed
@hashicorp hashicorp locked and limited conversation to collaborators Jun 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tenthirtyam tenthirtyam tenthirtyam approved these changes

@nywilken nywilken nywilken approved these changes

@lbajolet-hashicorp lbajolet-hashicorp Awaiting requested review from lbajolet-hashicorp

Assignees

@Hi-Angel Hi-Angel

Labels
bug regression
Projects
None yet
Milestone
v1.2.7
Development

Successfully merging this pull request may close these issues.

error removing cdrom prior to reattaching: invalid number: n must be between 0 and 0
3 participants
@Hi-Angel @nywilken @tenthirtyam