-
Notifications
You must be signed in to change notification settings - Fork 51
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
VirtualHardDisk: Resource for creating and attaching a virtual disk #279
base: main
Are you sure you want to change the base?
VirtualHardDisk: Resource for creating and attaching a virtual disk #279
Conversation
…low volumes to be formatted as Dev Drives
Codecov Report
@@ Coverage Diff @@
## main #279 +/- ##
====================================
- Coverage 95% 95% -1%
====================================
Files 7 9 +2
Lines 1025 1209 +184
====================================
+ Hits 977 1152 +175
- Misses 48 57 +9
|
FYI @PlagueHO, thanks! I'll work on an integration test for this in the mean time |
Thanks @bbonaby - I've blocked out sometime to get the review done either tomorrow arvo or on the weekend. |
…nd use safefilehandles instead of closehandle function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have run out of time tonight @bbonaby to finish review, but will try and get some more time on this tomorrow night.
Reviewed 1 of 13 files at r1, 4 of 11 files at r3, all commit messages.
Reviewable status: 5 of 13 files reviewed, 14 unresolved discussions (waiting on @bbonaby)
CHANGELOG.md
line 8 at r3 (raw file):
## [Unreleased] - Added DSC_VirtualHardDisk resource for creating virtual disks and tests. - Fixes [Issue #277](https://github.com/dsccommunity/StorageDsc/issues/277)
Can we add this to an ### Added
section above and also remove the fullstop at the end of the description. E.g.
### Added
- Added DSC_VirtualHardDisk resource for creating virtual disks and tests - Fixes [Issue #277](https://github.com/dsccommunity/StorageDsc/issues/277)
source/DSCResources/DSC_Disk/DSC_Disk.psm1
line 283 at r3 (raw file):
} if ($disk.PartitionStyle -eq 'RAW' -bor (-not $disk.PartitionStyle))
I noticed this was removed in the previous PR (DevDrive) - is this intentional?
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 5 at r3 (raw file):
class DSC_VirtualHardDisk : OMI_BaseResource { [Key, Description("Specifies the full path to the virtual hard disk file that will be created or attached. This must include the extension, and the extension must match the disk format.")] String FilePath;
"created or attached" - should this be "created and attached" ? E.g., is there a way this resource can create the vhd/x without attaching it?
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 6 at r3 (raw file):
{ [Key, Description("Specifies the full path to the virtual hard disk file that will be created or attached. This must include the extension, and the extension must match the disk format.")] String FilePath; [Write, Description("Specifies the size the of virtual hard disk.")] Uint64 DiskSize;
nit: Specifies the size the of virtual hard disk -> Specifies the size of virtual hard disk to create if it doesn't exist and Ensure is present.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 7 at r3 (raw file):
[Key, Description("Specifies the full path to the virtual hard disk file that will be created or attached. This must include the extension, and the extension must match the disk format.")] String FilePath; [Write, Description("Specifies the size the of virtual hard disk.")] Uint64 DiskSize; [Write, Description("Specifies the disk type the virtual hard disk should use."), ValueMap{"Fixed","Dynamic"}, Values{"Fixed","Dynamic"}] String DiskType;
nit: Specifies the disk type the virtual hard disk should use. -> Specifies the disk type of virtual hard disk to create if it doesn't exist and Ensure is present.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 8 at r3 (raw file):
[Write, Description("Specifies the size the of virtual hard disk.")] Uint64 DiskSize; [Write, Description("Specifies the disk type the virtual hard disk should use."), ValueMap{"Fixed","Dynamic"}, Values{"Fixed","Dynamic"}] String DiskType; [Write, Description("Specifies the disk format the virtual hard disk should use. Defaults to Vhdx."), ValueMap{"Vhd","Vhdx"}, Values{"Vhd","Vhdx"}] String DiskFormat;
nit: Specifies the disk format the virtual hard disk should use. Defaults to Vhdx. -> Specifies the disk format the virtual hard disk should use or create if it does not exist and Ensure is present. Defaults to Vhdx.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 9 at r3 (raw file):
[Write, Description("Specifies the disk type the virtual hard disk should use."), ValueMap{"Fixed","Dynamic"}, Values{"Fixed","Dynamic"}] String DiskType; [Write, Description("Specifies the disk format the virtual hard disk should use. Defaults to Vhdx."), ValueMap{"Vhd","Vhdx"}, Values{"Vhd","Vhdx"}] String DiskFormat; [Write, Description("Determines whether the virtual hard disk should be created and attached or should not be attached."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
nit: should not be attached -> should be detatched if it exists.
source/DSCResources/DSC_VirtualHardDisk/README.md
line 7 at r3 (raw file):
There are 2 high level scenarios, one where the user uses the 'Present' flag and the second when the user uses the 'Absent' flag in the resource. 1. When the 'Present' flag is used the resource checks if the virtual hard disk is on the machine using the file path that is entered.
For md, use 1. for each item (regardless of the actual number or letter). This is because md renderers will auto number items. I think our automatic md testing will highlight this as well (but isn't because of the blank lines between entries 😀)
Can also remove blank lines between items.
source/DSCResources/DSC_VirtualHardDisk/README.md
line 9 at r3 (raw file):
1. When the 'Present' flag is used the resource checks if the virtual hard disk is on the machine using the file path that is entered. a. If the file path is correct and it is confirmed to be the location of a real virtual hard disk file. The resource will do nothing.
Will the resource attach the virtual hard disk if it is not attached?
source/DSCResources/DSC_VirtualHardDisk/README.md
line 11 at r3 (raw file):
a. If the file path is correct and it is confirmed to be the location of a real virtual hard disk file. The resource will do nothing. b. If the location is a real virtual hard disk file but is not attached, the resource will attach the virtual hard disk to the system.
If the real vhd/vhdx exists but is not attached, but the size and/or disktype is different, what happens?
source/DSCResources/DSC_VirtualHardDisk/README.md
line 28 at r3 (raw file):
## How does this differ from the [DSC_VHD](https://github.com/dsccommunity/HyperVDsc/tree/main/source/DSCResources/DSC_VHD) in the Hyper-V Dsc? This DSC_VirtualHardDisk resource does not rely on the Hyper-V Windows feature being enabled to allow users to use the resource. Unlike the DSC_VHD, users can use this resource right out of the box assuming they are running at least `Windows 8.1 / Windows Server 2008 R2 or later`. The resource uses the publicly available Win32 virtual disk apis, to create and attach a virtual hard disk file (`.vhd` and `.vhdx`) to the system. See more information about the apis [here](https://learn.microsoft.com/en-us/windows/win32/api/virtdisk/).
Can we add a warning here that using both DSC_VirtualHardDisk and DSC_VHD in the same config/machine could result in an invalid config (not that anyone would... but we never know 😀 )
source/DSCResources/DSC_VirtualHardDisk/README.md
line 32 at r3 (raw file):
## Limitations 1. The resource only supports .vhd and .vhdx files. No other virtual hard disk file extension is supported at this time.
nit: wrap .vhd and .vhdx in backtick to highlight as code (even though not really code :D)
source/DSCResources/DSC_VirtualHardDisk/README.md
line 33 at r3 (raw file):
1. The resource only supports .vhd and .vhdx files. No other virtual hard disk file extension is supported at this time. 2. The ability to `expand` the max size of the virtual hard disk after its creation is not currently included in this resource.
See comments about markdown autonumbering above. Can also use the VS Markdown linting extension to automatically get this suggestion.
source/Modules/VirtualHardDisk.Win32Helpers/VirtualHardDisk.Win32Helpers.psm1
line 0 at r3 (raw file):
To help associate any imported modules with the DSC resource, can we name this as StorageDsc.VirtualHardDisk.Win32Helpers
- or something to that effect?
Hi @bbonaby - now that the other PR is merged, can you resolve the conflicts and I'll finish reviewing this one. |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
oh actually this can be removed. I had initially thought that $disk.PartitionStyle could be null when a disk is initially created/inserted into the system that does not have a partition style. But even in those cases 'RAW' is still returned. So I removed it from there, I believe it ended up here from when I was testing the new DevDrive flag in the Disk resource with this resource. I'll remove this addition thanks. |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
thanks updated |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
thanks updated |
Updating this today to resolve the merge conflicts and address the initial comments. Looks like I prematurely replied to your previous comment so sorry about that. I'll keep them as drafts before I push the update commit. Thanks |
…function calls in win32helpers
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated and clarified the behavior a bit more thanks. |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated and clarified how disksize and disktype are used. Thanks |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated and added a note about how they shouldn't be used together. I started the sentence off with 'Note:' . Let me know if you rather me put 'Warning:' thanks. |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated, thanks for the tip 😊 hope it looks better now. Let me know if you see any more issues. Thanks |
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated and changed the name to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 15 files reviewed, 14 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md
line 8 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can we add this to an
### Added
section above and also remove the fullstop at the end of the description. E.g.### Added - Added DSC_VirtualHardDisk resource for creating virtual disks and tests - Fixes [Issue #277](https://github.com/dsccommunity/StorageDsc/issues/277)
Thanks, updated.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 6 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Specifies the size the of virtual hard disk -> Specifies the size of virtual hard disk to create if it doesn't exist and Ensure is present.
thanks updated
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 7 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Specifies the disk type the virtual hard disk should use. -> Specifies the disk type of virtual hard disk to create if it doesn't exist and Ensure is present.
thanks updated
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof
line 8 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Specifies the disk format the virtual hard disk should use. Defaults to Vhdx. -> Specifies the disk format the virtual hard disk should use or create if it does not exist and Ensure is present. Defaults to Vhdx.
thanks updated
source/DSCResources/DSC_VirtualHardDisk/README.md
line 7 at r3 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
For md, use 1. for each item (regardless of the actual number or letter). This is because md renderers will auto number items. I think our automatic md testing will highlight this as well (but isn't because of the blank lines between entries 😀)
Can also remove blank lines between items.
updated, thanks
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
updated thanks |
Thanks @PlagueHO conflicts should be all resolved now |
Hey @PlagueHO any update on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies @bbonaby - been so snowed under the last couple of weeks this slipped off my radar. Will do this tonight.
Reviewable status: 0 of 15 files reviewed, 14 unresolved discussions
hey @PlagueHO any update on this PR? |
Hi @bbonaby - apologies, DSC has just managed to slip off my radar due to day job commitments. But I'll bump this one up and aim for it on the weekend. Again, apologies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 11 files at r3, 3 of 10 files at r5, 1 of 1 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: 8 of 15 files reviewed, 2 unresolved discussions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bbonaby - sorry about the delay. I've finished reviewing all the files now - nothing major just some style bits and pieces.
One question: how easy/would it be possible to add integration tests for this resource?
Reviewed 1 of 11 files at r3, 7 of 10 files at r5.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @bbonaby)
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 33 at r8 (raw file):
[Parameter(Mandatory = $true)] [ValidateNotNullOrEmpty()] [String]
nit: avoid type accelerators.
Suggestion:
[System.String]
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 43 at r8 (raw file):
$diskImage = Get-DiskImage -ImagePath $FilePath -ErrorAction SilentlyContinue $Ensure = 'Present'
nit: lower case for local vars.
Suggestion:
$ensure = 'Present'
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 47 at r8 (raw file):
if (-not $diskImage) { $Ensure = 'Absent'
nit: lower case for local vars.
Suggestion:
$ensure = 'Absent'
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 56 at r8 (raw file):
Size = $diskImage.Size DiskNumber = $diskImage.DiskNumber Ensure = $Ensure
nit: lower case for local vars.
Suggestion:
Ensure = $ensure
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 86 at r8 (raw file):
[Parameter(Mandatory = $true)] [ValidateNotNullOrEmpty()] [String]
nit: avoid type accelerators.
Suggestion:
[System.String]
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 112 at r8 (raw file):
Assert-ParametersValid -FilePath $FilePath -DiskSize $DiskSize -DiskFormat $DiskFormat $resource = Get-TargetResource -FilePath $FilePath
nit: Maybe rename variable to $currentState
to indicate that this is the current state of the virtual disk? Also aligns to style in other resources.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 144 at r8 (raw file):
{ Write-Verbose -Message ($script:localizedData.RemovingCreatedVirtualHardDiskFile -f $FilePath) Remove-Item $FilePath -verbose
Should we attempt with -Force
and -Confirm:No
?
Also capital V in verbose.
Suggestion:
Remove-Item $FilePath -Verbose
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 149 at r8 (raw file):
if ($wasLocationCreated) { Remove-Item -LiteralPath $folderPath -verbose
Should we attempt with -Force
and -Confirm:No
?
Also capital V in verbose.
Suggestion:
Remove-Item -LiteralPath $folderPath -Verbose
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 170 at r8 (raw file):
else { # Detach the virtual hard disk if its not suppose to be attached.
nit: comment correction.
Suggestion:
# Detach the virtual hard disk if its not supposed to be attached.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 211 at r8 (raw file):
[Parameter(Mandatory = $true)] [ValidateNotNullOrEmpty()] [String]
nit: avoid type accelerators.
Suggestion:
[System.String]
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 237 at r8 (raw file):
Assert-ParametersValid -FilePath $FilePath -DiskSize $DiskSize -DiskFormat $DiskFormat $resource = Get-TargetResource -FilePath $FilePath
nit: Maybe rename variable to $currentState
to indicate that this is the current state of the virtual disk? Also aligns to style in other resources.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 306 at r8 (raw file):
[Parameter(Mandatory = $true)] [ValidateNotNullOrEmpty()] [String]
nit: avoid using type accelerators.
Suggestion:
[System.String]
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 311 at r8 (raw file):
[Parameter(Mandatory = $true)] [System.UInt64] $DiskSize,
Should there also be a validate $DiskSize
is greater than zero? [ValidateScript({$ -gt 0})]
- although I can see that this would be detected lower down anyway.
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 359 at r8 (raw file):
$isInValidSizeForVhdxFormat = ($DiskSize -lt 10MB -bor $DiskSize -gt 64TB) if ((-not $isVhdxFormat -and $isInValidSizeForVhdFormat) -bor ($IsVhdxFormat -and $isInValidSizeForVhdxFormat))
nit: lower case
Suggestion:
($isVhdxFormat -and $isInValidSizeForVhdxFormat))
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 363 at r8 (raw file):
if ($DiskSize -lt 1GB) { $DiskSizeString = ($DiskSize / 1MB).ToString("0.00MB")
nit: style tweaks.
Suggestion:
$diskSizeString = ($DiskSize / 1MB).ToString('0.00MB')
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 367 at r8 (raw file):
else { $DiskSizeString = ($DiskSize / 1TB).ToString("0.00TB")
nit: style tweaks.
Suggestion:
$diskSizeString = ($DiskSize / 1TB).ToString('0.00TB')
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 370 at r8 (raw file):
} $InvalidSizeMsg = $script:localizedData.VhdFormatDiskSizeInvalid
nit: lower case local vars.
Suggestion:
$invalidSizeMsg = $script:localizedData.VhdFormatDiskSizeInvalid
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 373 at r8 (raw file):
if ($isVhdxFormat) { $InvalidSizeMsg = $script:localizedData.VhdxFormatDiskSizeInvalid
nit: lower case local vars.
Suggestion:
$invalidSizeMsg = $script:localizedData.VhdxFormatDiskSizeInvalid
source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1
line 377 at r8 (raw file):
New-InvalidArgumentException ` -Message $($InvalidSizeMsg -f $DiskSizeString) `
nit: lower case local vars.
Suggestion:
-Message $($invalidSizeMsg -f $DiskSizeString) `
source/DSCResources/DSC_VirtualHardDisk/README.md
line 61 at r8 (raw file):
1. The resource only supports `.vhd` and `.vhdx` files. No other virtual hard disk file extension is supported at this time. 1. The ability to `expand` the max size of the virtual hard disk after its creation
It looks like this line isn't complete. Presume it should end with 'is not currently included in this resource.'
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 168 at r8 (raw file):
} Context 'When file path does exist and was mounted at one point' {
This test looks like it verifies that the file exists and it is currently mounted? Should this reflect that it is currently mounted?
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 199 at r8 (raw file):
Describe 'DSC_VirtualHardDisk\Set-TargetResource' {
nit: Can remove blank line here
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 201 at r8 (raw file):
Context 'When file path is not fully qualified' {
nit: Can remove blank line here
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 276 at r8 (raw file):
Context 'When size provided is less than the minimum size for the vhd format' {
Can remove blank line.
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 277 at r8 (raw file):
Context 'When size provided is less than the minimum size for the vhd format' { $minSizeInMbString = ($DiskImageSizeBelowVirtDiskMinimum / 1MB).ToString("0.00MB")
nit: single quotes can be used here.
Suggestion:
$minSizeInMbString = ($DiskImageSizeBelowVirtDiskMinimum / 1MB).ToString('0.00MB')
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 296 at r8 (raw file):
Context 'When size provided is less than the minimum size for the vhdx format' { $minSizeInMbString = ($DiskImageSizeBelowVirtDiskMinimum / 1MB).ToString("0.00MB")
nit: single quotes can be used here.
Suggestion:
$minSizeInMbString = ($DiskImageSizeBelowVirtDiskMinimum / 1MB).ToString('0.00MB')
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 315 at r8 (raw file):
Context 'When size provided is greater than the maximum size for the vhd format' { $maxSizeInTbString = ($DiskImageSizeAboveVhdMaximum / 1TB).ToString("0.00TB")
nit: single quotes can be used here.
Suggestion:
$maxSizeInTbString = ($DiskImageSizeAboveVhdMaximum / 1TB).ToString('0.00TB')
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 334 at r8 (raw file):
Context 'When size provided is greater than the maximum size for the vhdx format' { $maxSizeInTbString = ($DiskImageSizeAboveVhdxMaximum / 1TB).ToString("0.00TB")
nit: single quotes can be used here.
Suggestion:
$maxSizeInTbString = ($DiskImageSizeAboveVhdxMaximum / 1TB).ToString('0.00TB')
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 462 at r8 (raw file):
} Context 'Virtual disk does not exist and ensure set to present, so a new one should be created.' {
No need for full stop and could specify that it will be mounted.
Suggestion:
Context 'Virtual disk does not exist and ensure set to present, so a new one should be created and mounted' {
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 492 at r8 (raw file):
Context 'When folder does not exist in user provided path but an exception occurs after creating the virtual disk' {
Can remove blank line.
tests/Unit/DSC_VirtualHardDisk.Tests.ps1
line 585 at r8 (raw file):
-DiskFormat $extension ` -Ensure 'Present' ` -Verbose | Should -Be $false
Can use -BeFalse
and -BeTrue
throughout.
Suggestion:
-Verbose | Should -BeFalse
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 309 at r8 (raw file):
<# .SYNOPSIS Calls Win32 OpenVirtualDisk api. This is used so we can mock this call
Maybe move this to the .DESCRIPTION and use the .SYNOPSIS to define the actual function? E.g., something like "Open an existing virtual disk"?
What happens if the VHD/VHDX doesn't exist?
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 395 at r8 (raw file):
[Parameter(Mandatory = $true)] [System.String] $VirtualDiskPath,
Could add a [Validate()] here to verify the file does not exist (as I presume it should not)?
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 417 at r8 (raw file):
# Get parameters for CreateVirtualDisk function [ref]$virtualStorageType = Get-VirtualStorageType -DiskFormat $DiskFormat [ref]$createVirtualDiskParameters = New-Object VirtDisk.Helper+CREATE_VIRTUAL_DISK_PARAMETERS
nit: Use parameter names
Suggestion:
[ref]$createVirtualDiskParameters = New-Object -TypeName VirtDisk.Helper+CREATE_VIRTUAL_DISK_PARAMETERS
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 440 at r8 (raw file):
$result = New-VirtualDiskUsingWin32 ` -VirtualStorageType $virtualStorageType ` -VirtualDiskPath $VirtualDiskPath `
What happens if the disk (or a file in this location) already exists? Do we just catch and throw?
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 493 at r8 (raw file):
( [Parameter(Mandatory = $true)] [System.String]
Could add a [Validate()] here to verify the file exists (as I presume it should)?
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 518 at r8 (raw file):
# Build parameters for AttachVirtualDisk function. [ref]$attachVirtualDiskParameters = New-Object VirtDisk.Helper+ATTACH_VIRTUAL_DISK_PARAMETERS
nit: Use parameter names
Suggestion:
[ref]$attachVirtualDiskParameters = New-Object -TypeName VirtDisk.Helper+ATTACH_VIRTUAL_DISK_PARAMETERS
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 529 at r8 (raw file):
virtual disk to be attached by the system at boot time. #> for ($attempts = 0; $attempts -lt 2; $attempts++)
Another way of doing this could be to use something like (which might be slightly cleaner... but not much :grinnng:):
$attemptFlagValues = @(
[VirtDisk.Helper]::ATTACH_VIRTUAL_DISK_FLAG_PERMANENT_LIFETIME -bor [VirtDisk.Helper]::ATTACH_VIRTUAL_DISK_FLAG_AT_BOOT,
[VirtDisk.Helper]::ATTACH_VIRTUAL_DISK_FLAG_PERMANENT_LIFETIME
)
foreach ($flags in $attemptFlagValues)
{
$result = Add-VirtualDiskUsingWin32 `
-Handle $Handle `
-SecurityDescriptor $securityDescriptor `
-Flags $flags `
-ProviderSpecificFlags $providerSpecificFlags `
-AttachVirtualDiskParameters $attachVirtualDiskParameters `
-Overlapped ([System.IntPtr]::Zero)
if ($result -eq 0)
{
break
}
}
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 593 at r8 (raw file):
[Parameter(Mandatory = $true)] [System.String] $VirtualDiskPath,
Could add a [Validate()] here to verify the file exists (as I presume it should)?
source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1
line 606 at r8 (raw file):
# Get parameters for OpenVirtualDisk function. [ref]$virtualStorageType = Get-VirtualStorageType -DiskFormat $DiskFormat [ref]$openVirtualDiskParameters = New-Object VirtDisk.Helper+OPEN_VIRTUAL_DISK_PARAMETERS
nit: Add parameter name
Suggestion:
[ref]$openVirtualDiskParameters = New-Object -TypeName VirtDisk.Helper+OPEN_VIRTUAL_DISK_PARAMETERS
tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1
line 29 at r8 (raw file):
# Begin Testing InModuleScope $script:subModuleName { Function New-VirtualDiskUsingWin32
nit: lower case f for function
Suggestion:
function New-VirtualDiskUsingWin32
tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1
line 73 at r8 (raw file):
} Function Add-VirtualDiskUsingWin32
nit: lower case f for function
Suggestion:
function Add-VirtualDiskUsingWin32
tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1
line 105 at r8 (raw file):
} Function Get-VirtualDiskUsingWin32
nit: lower case f for function
Suggestion:
function Get-VirtualDiskUsingWin32
Thanks @PlagueHO What I'll do it update the PR with your requested changes first, then when you're all good with them. Add the Integration test. Is that all good with you? |
Ok @bbonaby - that is all good - wasn't sure we could add them, but if it's possible we can add them later. |
Pull Request (PR) description
This PR adds a virtual hard disk resource that will allow DSC users to create and attach a virtual hard disk without needing a storage pool or enabling the Hyper-V Windows feature. Half of this PR is unit tests. I'll need to add integration tests as this feature should be usable in server 2019 and server 2022 without enabling anything special. But I'd like to start the review process since with the added unit tests the PR is getting big.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is