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.

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

virt-v2v: Replace the Get-Disk and Set-Disk with diskpart #991

Closed
wants to merge 1 commit into from

Conversation

mnecas
Copy link
Member

@mnecas mnecas commented Aug 21, 2024

Issue: The Get-Disk and Set-Disk do not support dynamic disks. https://learn.microsoft.com/en-us/powershell/module/storage/get-disk?view=windowsserver2022-ps

@mnecas mnecas requested a review from yaacov as a code owner August 21, 2024 14:14
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.21%. Comparing base (2748c5d) to head (370a1c0).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #991   +/-   ##
=======================================
  Coverage   16.21%   16.21%           
=======================================
  Files         106      106           
  Lines       19543    19541    -2     
=======================================
+ Hits         3168     3169    +1     
+ Misses      16094    16092    -2     
+ Partials      281      280    -1     
Flag Coverage Δ
unittests 16.21% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

I don't speak ps1, lgtm

Copy link
Contributor

@fabiand fabiand left a comment

Choose a reason for hiding this comment

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

Mostly requetsing change to avoid that we accidentally merge it.

Comment on lines +18 to +26
foreach ($disk in $disks) {
$diskNumber = $disk.Index # Get the disk index which corresponds to the disk number

# Create a diskpart script to set the disk online
$diskpartScript = @"
select disk $diskNumber
online disk
attributes disk clear readonly
"@
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels quie risky to me.

Take an index from one tool/api, and use it in a different tool/api.

This is just waiting for indexes to be out of sync for whatever reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree and I would like to not use the diskpart

@mnecas mnecas marked this pull request as draft August 27, 2024 07:12
@fabiand
Copy link
Contributor

fabiand commented Sep 25, 2024

Can we close this?

@mnecas mnecas closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants