fix: use cdroms len instead of ISOPaths len#394
Merged
nywilken merged 4 commits intovmware:mainfrom Apr 2, 2024
Merged
Conversation
It's a pattern that re-appears in the code, let's deduplicate that.
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: vmware#393
Contributor
Author
|
Okay, ready for review.
Please review 😊 |
tenthirtyam
reviewed
Mar 28, 2024
Adds the documentation for `reattach_cdroms` to the generated documentation. Signed-off-by: Ryan Johnson <ryan.johnson@broadcom.com>
tenthirtyam
approved these changes
Apr 2, 2024
Collaborator
tenthirtyam
left a comment
There was a problem hiding this comment.
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.
Contributor
Author
|
Cool, thank you! |
nywilken
approved these changes
Apr 2, 2024
Contributor
nywilken
left a comment
There was a problem hiding this comment.
Nicely done - the changes look good to me. Thanks for validating @tenthirtyam
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ISOPathsto figure out amount of cdroms, but that's not really correct and this assumption breaks whenremove_cdromis set. Fix the problem by making use of the function listing the devices that was added in prev. commit.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