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

Add SerialNumber as a disk identifier to Disk, DiskAccessPath and Wai… #271

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

dshelb
Copy link
Contributor

@dshelb dshelb commented Jan 26, 2023

Pull Request (PR) description:

This PR introduces support for using disk serial number as a disk identifier in disk-related resources Disk, DiskAccessPath and WaitForDisk.

This Pull Request (PR) fixes the following issues:
Fixes #259

Task list:

  • 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).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #271 (64344d9) into main (b4f8a2a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #271   +/-   ##
===================================
  Coverage    95%    95%           
===================================
  Files         7      7           
  Lines       882    882           
===================================
  Hits        842    842           
  Misses       40     40           
Impacted Files Coverage Δ
source/DSCResources/DSC_Disk/DSC_Disk.psm1 98% <ø> (ø)
...sources/DSC_DiskAccessPath/DSC_DiskAccessPath.psm1 89% <ø> (ø)
.../DSCResources/DSC_WaitForDisk/DSC_WaitForDisk.psm1 100% <ø> (ø)
...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1 100% <ø> (ø)

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 @dshelb - the reason the integration tests are failing is because the virtual disk that is being created does not have a serial number:

image.png

This is because DISKPART CREATE VDISK command creates it without one. I'm not sure if this is a limitation of the VHD format or a limitation of DISKPART.

I thought it might be possible to set the serialNumber by adding the following to the DISKPART commands in New-VDisk:

UNIQUEID DISK ID=`"$([Guid]::NewGuid($Guid))`"

But that doesn't appear to work (in my limited testing).

So, other options:

  1. Try and find another way to set the serial number of the VHD (if possible).
  2. Use a different method of creating a disk to test with.
  3. Flag this and skip integration tests for SerialNumber as the Disk ID.

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: 13 of 14 files reviewed, all discussions resolved

@dshelb
Copy link
Contributor Author

dshelb commented Feb 1, 2023

Let's flag this for now and skip the integration tests for SerialNumber as the Disk Identifier.

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.

Happy to skip the tests for now, but we should note this in the README that these aren't performed for serial number.

Can you also add the integration tests back in but surround them in an if ($false) block with a comment as these can still be run locally with modifications.

Reviewable status: 13 of 14 files reviewed, all discussions resolved

@johlju johlju added the needs review The pull request needs a code review. label Feb 6, 2023
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 @dshelb - Nearly there - just some minor tweaks and we'll merge.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dshelb)


README.md line 36 at r3 (raw file):

    mounted as read-only (ISO, VHD, VHDx) or read/write (VHD, VHDx).
- **Disk**: used to initialize, format and mount the partition as a drive letter.
    NOTE: Integration tests are not ran for Get-DiskByIdentifier when SerialNumber

Nit: grammar, wording

Can you also move this into the README.md for the Disk resource itself under a new section "Testing": https://github.com/dsccommunity/StorageDsc/blob/main/source/DSCResources/DSC_Disk/README.md

Suggestion:

    NOTE: Integration tests are not run for the Disk resource when SerialNumber
    is used since the virtual disk that is created does not have a serial number.

tests/Integration/DSC_Disk.Integration.Tests.ps1 line 862 at r3 (raw file):

        }

        if ($false) # These serial number integration tests can be ran locally with modifications

Can you move comment above if block and include info on why they can't be run and how they could be?

Use a comment block format if more than one line...

E.g.

<#
    Integration tests are disabled for the Disk resource when being tested with 'DiskIdType' set to 'SerialNumber'
    because the Virtual Disk VHD that is created always has a blank SerialNumber.
    To test manually, use a physical/logical disk that has a SerialNumber.
#>

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 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dshelb)

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:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dshelb)

@PlagueHO PlagueHO merged commit 4a9e4fa into dsccommunity:main Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request needs a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get-DiskByIdentifier: Allow selection based on Serial Number
3 participants