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

VirtualHardDisk: Resource for creating and attaching a virtual disk #279

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented Sep 25, 2023

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

  • [x ] Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • [x ] Resource documentation added/updated in README.md.
  • [x ] Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • [ x] Comment-based help added/updated.
  • [x ] Localization strings added/updated in all localization files as appropriate.
  • [ x] Examples appropriately added/updated.
  • [ x] Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • [ x] New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Attention: Patch coverage is 94.21053% with 11 lines in your changes missing coverage. Please review.

Project coverage is 95%. Comparing base (a87725b) to head (49fb638).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...lpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 86% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #279    +/-   ##
====================================
- Coverage    95%    95%    -1%     
====================================
  Files         7      9     +2     
  Lines      1025   1236   +211     
====================================
+ Hits        977   1176   +199     
- Misses       48     60    +12     
Files with missing lines Coverage Δ
...urces/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 100% <100%> (ø)
...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1 91% <100%> (ø)
...lpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 86% <86%> (ø)

@bbonaby
Copy link
Contributor Author

bbonaby commented Sep 26, 2023

FYI @PlagueHO, thanks! I'll work on an integration test for this in the mean time

@johlju johlju added the needs review The pull request needs a code review. label Sep 26, 2023
@PlagueHO
Copy link
Member

Thanks @bbonaby - I've blocked out sometime to get the review done either tomorrow arvo or on the weekend.

Copy link
Member

@PlagueHO PlagueHO left a 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?

@PlagueHO
Copy link
Member

Hi @bbonaby - now that the other PR is merged, can you resolve the conflicts and I'll finish reviewing this one.

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 27, 2023

source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 283 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I noticed this was removed in the previous PR (DevDrive) - is this intentional?

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.

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 27, 2023

source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof line 5 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

"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?

thanks updated

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 27, 2023

source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.schema.mof line 9 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: should not be attached -> should be detatched if it exists.

thanks updated

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 27, 2023

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

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 28, 2023

source/DSCResources/DSC_VirtualHardDisk/README.md line 9 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Will the resource attach the virtual hard disk if it is not attached?

updated and clarified the behavior a bit more thanks.

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 28, 2023

source/DSCResources/DSC_VirtualHardDisk/README.md line 11 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

If the real vhd/vhdx exists but is not attached, but the size and/or disktype is different, what happens?

updated and clarified how disksize and disktype are used. Thanks

@bbonaby
Copy link
Contributor Author

bbonaby commented Oct 31, 2023

Thanks @PlagueHO conflicts should be all resolved now

@bbonaby
Copy link
Contributor Author

bbonaby commented Nov 9, 2023

Hey @PlagueHO any update on this?

Copy link
Member

@PlagueHO PlagueHO left a 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

@bbonaby
Copy link
Contributor Author

bbonaby commented Feb 13, 2024

hey @PlagueHO any update on this PR?

@PlagueHO
Copy link
Member

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.

Copy link
Member

@PlagueHO PlagueHO left a 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

Copy link
Member

@PlagueHO PlagueHO left a 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

@bbonaby
Copy link
Contributor Author

bbonaby commented Mar 8, 2024

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?

@PlagueHO
Copy link
Member

Ok @bbonaby - that is all good - wasn't sure we could add them, but if it's possible we can add them later.

@johlju
Copy link
Member

johlju commented Aug 7, 2024

For the build to pass the line:

dotnet tool install --global GitVersion.Tool

Need to be change to:

dotnet tool install --global GitVersion.Tool --version 5.*

This is due to GitVersion 6 that was release has breaking changes that are not yet support by the Sampler pipeline.

Copy link
Contributor Author

@bbonaby bbonaby left a comment

Choose a reason for hiding this comment

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

Thanks, I updated the PR. To add this line.

Reviewable status: 6 of 19 files reviewed, 44 unresolved discussions (waiting on @PlagueHO)


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 43 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case for local vars.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 47 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case for local vars.

Thanks, updated


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 56 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case for local vars.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 86 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: avoid type accelerators.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 112 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

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.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 144 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should we attempt with -Force and -Confirm:No?

Also capital V in verbose.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 149 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should we attempt with -Force and -Confirm:No?

Also capital V in verbose.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 211 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: avoid type accelerators.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 237 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

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.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 306 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: avoid using type accelerators.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 311 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

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.

Thanks, I added a validationScript tag


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 359 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case

Thanks, updated


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 363 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: style tweaks.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 367 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: style tweaks.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 370 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case local vars.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 373 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case local vars.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 377 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case local vars.

Thanks, updated.


source/DSCResources/DSC_VirtualHardDisk/README.md line 61 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

It looks like this line isn't complete. Presume it should end with 'is not currently included in this resource.'

Thanks, updated.


source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 645 at r11 (raw file):

    return $currentPrincipal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)
} # end function Test-RunningAsAdministrator

@PlagueHO I added this new method because running the Set-DscResource isn't guaranteed to be running in a process with admin privileges. Mounting a disk requires running elevated.


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 168 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This test looks like it verifies that the file exists and it is currently mounted? Should this reflect that it is currently mounted?

I'm not sure what you mean here. Could you explain it a bit more?


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 199 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Can remove blank line here

removed, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 201 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Can remove blank line here

removed, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 276 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can remove blank line.

removed, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 277 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: single quotes can be used here.

updated, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 296 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: single quotes can be used here.

removed, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 315 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: single quotes can be used here.

updated thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 334 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: single quotes can be used here.

updated thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 462 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

No need for full stop and could specify that it will be mounted.

updated thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 492 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can remove blank line.

removed, thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 585 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can use -BeFalse and -BeTrue throughout.

updated thanks


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 309 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

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?

I added a .Description and updated the .synopsis.

When the vhd or vhdx file doesn't exist, the win32 apis return the error code for file not found. Which we will then throw a win32 exception and display the error message for file not found. But I updated the $VirtualDiskPath parameter in the win32 function wrappers to use a ValidateScript tag as an extra layer of protection with my latest commits. We use the Test-Path to confirm whether the file exists or not.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 395 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could add a [Validate()] here to verify the file does not exist (as I presume it should not)?

I added one to the New-VirtualDiskUsingWin32 method, so we should be fine here I believe. I did attempt to put one here but couldn't get the unit tests to mock the Test-Path function in the ValidateScript. The validation script runs before the Mocked call so it ends up using the real Test-Path function and ultimately the unit test for New-SimpleVirtualDisk fails because of it since the path isn't real. But if you know of another way let me know.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 417 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Use parameter names

updated, thanks


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 440 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

What happens if the disk (or a file in this location) already exists? Do we just catch and throw?

originally this would just return the file not found error and then we would throw that exception. Now that I've added the [ValidateScript({ -not (Test-Path $_ )})] tag to New-VirtualDiskUsingWin32's VirtualDiskPath parameter, we'll get an error message about the parameter before calling the win32 api


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 493 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could add a [Validate()] here to verify the file exists (as I presume it should)?

See the reply to your comment in the New-SimpleVirtualDisk function, the same thing applies here


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 518 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Use parameter names

updated, thanks


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 529 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

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
            }
}

Thanks, I tweaked it a little since I was getting a bitwise_or error when adding the second line in your suggestion to the array but the structure of your suggestion remains the same.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 593 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could add a [Validate()] here to verify the file exists (as I presume it should)?

I added one to the Get-VirtualDiskUsingWin32 method, which this calls. I had the same issue with New-SimpleVirtualDisk with unit testing when using ValidateScript on the parameter with Test-Path.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 606 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Add parameter name

updated thanks


tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1 line 29 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case f for function

updated thanks


tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1 line 73 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case f for function

updated thanks


tests/Unit/StorageDsc.VirtualHardDisk.Win32Helpers.Tests.ps1 line 105 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: lower case f for function

updated thanks

@bbonaby
Copy link
Contributor Author

bbonaby commented Aug 20, 2024

@PlagueHO Sorry it took so long getting back to you, was busy with MS Build 2024 at the time, then summer hit. I updated the PR based on your comments and also added integration tests as well, thanks for your patience.

@PlagueHO
Copy link
Member

No problem @bbonaby - will review this weekend (day job stuff got in the way this week).

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

THanks for your patience @bbonaby - Just some super minor bits. Ping my on Teams when good to go.

Reviewed 1 of 1 files at r9, 11 of 12 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @bbonaby)


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 112 at r11 (raw file):

    Assert-ParametersValid -FilePath $FilePath -DiskSize $DiskSize -DiskFormat $DiskFormat

    if (-not (Test-RunningAsAdministrator))

Is it possible to use Assert-ElevatedUser in DSCResource.Common for this? I haven't checked in detail, but it looks like it's the same implementation?


source/DSCResources/DSC_VirtualHardDisk/en-US/DSC_VirtualHardDisk.strings.psd1 line 17 at r11 (raw file):

    GettingVirtualHardDisk = Getting virtual hard disk information for virtual hard disk located at '{0}.
    RemovingCreatedVirtualHardDiskFile = The virtual hard disk file at location '{0}' is being removed due to an error while attempting to mount it to the system.
    VirtualDiskAdminError = Creating and mounting a virtual disk requires running PowerShell as an Administrator. Please launch PowerShell as Administrator and try again.

nit: Thinking about the wording of this message: If the resource is applied by the LCM them it won't be performed by launching PS. I guess though, the LCM usually runs as Administrator anyway, so this issue shouldn't occur. But maybe state: "Creating and mounting a virtual disk requires running local Administrator permissions. Please ensure this resource is being applied with an account with local Administrator permissions." - or something like that?


source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 645 at r11 (raw file):

Previously, bbonaby (Branden Bonaby) wrote…

@PlagueHO I added this new method because running the Set-DscResource isn't guaranteed to be running in a process with admin privileges. Mounting a disk requires running elevated.

Is it possible to use Assert-ElevatedUser in DSCResource.Common for this? I haven't checked in detail, but it looks like it's the same implementation?


tests/Integration/DSC_VirtualHardDisk.config.ps1 line 1 at r11 (raw file):

$TestFixedVirtualHardDiskVhd = @{

Can we move this block into a $configData block in the *.Integration.Tests.ps1 file? E.g., like this: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)


tests/Integration/DSC_VirtualHardDisk.config.ps1 line 8 at r11 (raw file):

}

$TestDynamicVirtualHardDiskVhdx = @{

Can we move this block into a $configData block in the *.Integration.Tests.ps1 file? E.g., like this: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)


tests/Integration/DSC_VirtualHardDisk.Integration.Tests.ps1 line 27 at r11 (raw file):

    Describe "$($script:dscResourceName)_CreateAndAttachFixedVhd_Integration" {
        Context 'Create and attach a fixed virtual disk' {

Could you copy the integration test pattern from ComputerManagementDsc: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)

Specifically, spit the "compile and apply" into two It blocks and use the $configData object.


tests/Integration/DSC_VirtualHardDisk.Integration.Tests.ps1 line 43 at r11 (raw file):

                    $_.ConfigurationName -eq "$($script:dscResourceName)_CreateAndAttachFixedVhd_Config"
                }
                $currentState.FilePath   | Should -Be $script:TestFixedVirtualHardDiskVhd.FilePath

Once the $configData object is implemented, these tests can compare the values against the properties in that object. E.g. ComputerManagementDsc/tests/Integration/DSC_UserAccountControl.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 168 at r8 (raw file):

Previously, bbonaby (Branden Bonaby) wrote…

I'm not sure what you mean here. Could you explain it a bit more?

I mean, should the context be "When file path does exist and is currently mounted"'


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 137 at r11 (raw file):

        }

        function Test-RunningAsAdministrator

Might not need this if use the Assert from DSCResource.Common.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 156 at r11 (raw file):

        This is used so we can mock this call easier.

    .SYNOPSIS

nit: .SYNOPSIS should be first in block


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 250 at r11 (raw file):

        to the system. This is used so we can mock this call easier.

    .SYNOPSIS

nit: .SYNOPSIS should be first in block


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 319 at r11 (raw file):

        This is used so we can mock this call easier.

    .SYNOPSIS

nit: .SYNOPSIS should be first in block

Copy link
Contributor Author

@bbonaby bbonaby left a comment

Choose a reason for hiding this comment

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

Reviewable status: 12 of 19 files reviewed, 12 unresolved discussions (waiting on @PlagueHO)


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 170 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: comment correction.

updated, thanks


source/DSCResources/DSC_VirtualHardDisk/DSC_VirtualHardDisk.psm1 line 112 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is it possible to use Assert-ElevatedUser in DSCResource.Common for this? I haven't checked in detail, but it looks like it's the same implementation?

yea that works, I updated this. Thanks


source/DSCResources/DSC_VirtualHardDisk/en-US/DSC_VirtualHardDisk.strings.psd1 line 17 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: Thinking about the wording of this message: If the resource is applied by the LCM them it won't be performed by launching PS. I guess though, the LCM usually runs as Administrator anyway, so this issue shouldn't occur. But maybe state: "Creating and mounting a virtual disk requires running local Administrator permissions. Please ensure this resource is being applied with an account with local Administrator permissions." - or something like that?

Oh that sounds good. I updated the text. Thanks


source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 645 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is it possible to use Assert-ElevatedUser in DSCResource.Common for this? I haven't checked in detail, but it looks like it's the same implementation?

yea, you're right. I updated this. Thanks. Now that I know this I can make a PR to fix this Dev Drive issue about not telling the user they need to run as administrator: Dev Drive DSC error message when not run as admin · Issue #284 · dsccommunity/StorageDsc (github.com)


tests/Integration/DSC_VirtualHardDisk.config.ps1 line 1 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we move this block into a $configData block in the *.Integration.Tests.ps1 file? E.g., like this: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)

thanks, updated. The only thing I kept was the file path, since it's needed in the "AfterAll" case when the integration test is cleaning up the virtual disk. At that point the $configData doesn't exist anymore.


tests/Integration/DSC_VirtualHardDisk.config.ps1 line 8 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we move this block into a $configData block in the *.Integration.Tests.ps1 file? E.g., like this: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)

thanks, updated. The only thing I kept was the file path, since it's needed in the "AfterAll" case when the integration test is cleaning up the virtual disk. At that point the $configData doesn't exist anymore.


tests/Integration/DSC_VirtualHardDisk.Integration.Tests.ps1 line 27 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you copy the integration test pattern from ComputerManagementDsc: ComputerManagementDsc/tests/Integration/DSC_SystemLocale.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)

Specifically, spit the "compile and apply" into two It blocks and use the $configData object.

thanks updated


tests/Integration/DSC_VirtualHardDisk.Integration.Tests.ps1 line 43 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Once the $configData object is implemented, these tests can compare the values against the properties in that object. E.g. ComputerManagementDsc/tests/Integration/DSC_UserAccountControl.Integration.Tests.ps1 at main · dsccommunity/ComputerManagementDsc (github.com)

thanks updated


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 168 at r8 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I mean, should the context be "When file path does exist and is currently mounted"'

Oh I see. Updated the text. Thanks


tests/Unit/DSC_VirtualHardDisk.Tests.ps1 line 137 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Might not need this if use the Assert from DSCResource.Common.

you're right. I removed this, thanks


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 156 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: .SYNOPSIS should be first in block

thanks, updated.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 250 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: .SYNOPSIS should be first in block

thanks, updated.


source/Modules/StorageDsc.VirtualHardDisk.Win32Helpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 line 319 at r11 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

nit: .SYNOPSIS should be first in block

thanks, updated.

@bbonaby
Copy link
Contributor Author

bbonaby commented Sep 13, 2024

Thanks for the feedback @PlagueHO. I updated the PR based on your comments. Thanks

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Thanks @bbonaby !

Reviewed 7 of 7 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bbonaby)

@PlagueHO PlagueHO merged commit 215b284 into dsccommunity:main Sep 16, 2024
11 checks passed
@johlju johlju removed the needs review The pull request needs a code review. label Sep 29, 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.

VirtualHardDisk: New resource proposal
4 participants