DSC_Disk: Add dev drive creation support to disk resource #278
Conversation
…n unallocated space
Codecov Report
@@ Coverage Diff @@
## main #278 +/- ##
====================================
- Coverage 95% 95% -1%
====================================
Files 7 7
Lines 882 1025 +143
====================================
+ Hits 842 977 +135
- Misses 40 48 +8
|
There was a problem hiding this comment.
Thanks @bbonaby - I'll get onto review this week. Looks like the HQRM tests are failing:
They are just style corrections in StorageDsc.Common.psm1 - nothing major and should be a quick fix.
Reviewable status: 0 of 9 files reviewed, all discussions resolved
There was a problem hiding this comment.
Sorry about the delay.
Can we add some notes (and links) about how Dev Drive should be used in the README.md for Disk? E.g. in here: StorageDsc/source/DSCResources/DSC_Disk/README.md at main · dsccommunity/StorageDsc (github.com)
Could we include:
- is it possible to have more than one Dev Drive in a machine and if not, what will happen if more than one volume is defined as a Dev Drive? Looking at example, it does seem like more than one DevDrive can be mounted.
- what happens if a volume that was assigned as a Dev Drive is changed to no longer being a Dev Drive - e.g. can the resource turn a Dev Drive back into a normal drive/volume (required to be idempotent). If not, then we'd need to document this as there will be situations where this resource won't be able to bring the disk into desired state. Ideally, we always want a resource to be able to go from any state into the desired state - but sometimes this isn't possible - so we need to indicate this. Looking at the code, it does look like once a volume is defined as a DevDrive, it isn't possible to bring it into a new state - is my understanding correct?
- What would happen if DevDrive feature isn't enabled on a machine and DevDrive is requested?
Reviewed 4 of 9 files at r1, 3 of 5 files at r2, all commit messages.
Reviewable status: 7 of 9 files reviewed, 18 unresolved discussions (waiting on @bbonaby)
CHANGELOG.md line 8 at r2 (raw file):
## [Unreleased] - Updated DSC_Disk to allow volumes to be formatted as Dev Drives: Fixes #276
For consistency, can you use this format?
Suggestion:
- Updated DSC_Disk to allow volumes to be formatted as Dev Drives - Fixes [Issue #276](https://github.com/dsccommunity/StorageDsc/issues/276)source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 181 at r2 (raw file):
.PARAMETER UseUnallocatedSpace Specifies that a new partition and volume should be formatted onto unallocated space on the disk.
Does this parameter differ from the Size parameter being left empty above? This automatically forces the disk to use all remaining unallocated space for the volume.
I think I might be getting confused though (after looking at the example) - aren't volumes & partitions going to be always created using unallocated space?
I guess what I'm asking is: is this parameter needed?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 415 at r2 (raw file):
There are two instances when we attempt to create a new partition: 1. When there are no partitions matching the drive letter the user entered. 2. When the user has advised us that they want to create a new partition on the disk's
If $Size is not specified and there is no matching partition found and empty space then $partition should be null, so I think (I could be wrong) that $UseUnallocatedSpace isn't needed?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 584 at r2 (raw file):
if ($DevDrive) { $formatVolumeParameters['DevDrive'] = $DevDrive
We can't assume that Test has fired, so can we also assert the DevDrive requirements here?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 626 at r2 (raw file):
if ($PSBoundParameters.ContainsKey('DevDrive')) { $formatParam.Add('DevDrive', $DevDrive)
We can't assume that Test has fired, so can we also assert the DevDrive requirements here?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 796 at r2 (raw file):
} # if # Check Dev Drive feature is enabled and that the user inputted ReFS as the file system.
This block should be down further as it will fail if the disk isn't yet initialized.
It also looks like it sort of duplicates some of the code/checks from the code following (e.g., from about here onwards: StorageDsc/source/DSCResources/DSC_Disk/DSC_Disk.psm1 at main · dsccommunity/StorageDsc (github.com))
E.g., right now there are two free space size checks performed if a DevDrive is used - one check here and another on line StorageDsc/source/DSCResources/DSC_Disk/DSC_Disk.psm1 at main · dsccommunity/StorageDsc (github.com)
Can this code be merged together to reduce duplicate functionality?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 809 at r2 (raw file):
if ($disk.PartitionStyle -ne 'RAW' ) { $currentDiskFreeSpace = $disk.LargestFreeExtent
This is interesting: we are using the Get-PartitionSupportedSize to get the largest free extent value, but it's pretty flaky. So perhaps it would be better to convert to using disk.LargestFreeExtent?
source/DSCResources/DSC_Disk/DSC_Disk.schema.mof line 16 at r2 (raw file):
[Write, Description("Specifies if the disks partition schema should be removed entirely, even if data and OEM partitions are present. Only possible with AllowDestructive enabled.")] Boolean ClearDisk; [Write, Description("Specifies if the volume should be formatted as a Dev Drive.")] Boolean DevDrive; [Write, Description("Specifies that a new partition and volume should be formatted onto unallocated space in the disk.")] Boolean UseUnallocatedSpace;
See previous comments - how does this differ from when size parameter is not specified?
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 22 at r2 (raw file):
<# .DESCRIPTION This configuration will wait for disk 2 to become available, and then make the disk available as
Can you also specify that the Disk will be formatted as ReFS if not already formatted. If it is already formatted (but not ReFS) then will throw an exception.
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 50 at r2 (raw file):
DevDrive = $true Size = 50Gb UseUnallocatedSpace = $true
I'm still not 100% sure I understand this parameter purpose. If this was $false and E drive did not exist, what would happen?
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 271 at r2 (raw file):
<# .SYNOPSIS Invokes win32 IsApiSetImplemented function
Nit: end with full stop (for autodocs)
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 274 at r2 (raw file):
.PARAMETER AccessPath Specifies the contract string for the dll that houses the win32 function
Nit: end with full stop (for autodocs)
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 276 at r2 (raw file):
Specifies the contract string for the dll that houses the win32 function #> function Get-IsApiSetImplemented
Should this be Invoke-IsApiSetImplemented?
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 294 at r2 (raw file):
<# .SYNOPSIS Invokes win32 GetDeveloperDriveEnablementState function
Nit: end with full stop (for autodocs)
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 296 at r2 (raw file):
Invokes win32 GetDeveloperDriveEnablementState function #> function Get-DeveloperDriveEnablementState
For consistency, should this be Get-DevDriveEnablementState?
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 321 at r2 (raw file):
Write-Verbose -Message ($script:localizedData.CheckingDevDriveEnablementMessage) $IsApiSetImplemented = Get-IsApiSetImplemented("api-ms-win-core-sysinfo-l1-2-6")
nit: can use single quotes here.
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 323 at r2 (raw file):
$IsApiSetImplemented = Get-IsApiSetImplemented("api-ms-win-core-sysinfo-l1-2-6") $DevDriveEnablementType = [DevDrive.DevDriveHelper+DEVELOPER_DRIVE_ENABLEMENT_STATE] if ($IsApiSetImplemented)
nit: add blank line above.
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 429 at r2 (raw file):
#> $notEnoughSpace = $false if (-not $UserDesiredSize)
nit: add blank line above.
There was a problem hiding this comment.
Also - thank you for submitting this and sorry about taking so long :D
Reviewable status: 7 of 9 files reviewed, 18 unresolved discussions (waiting on @bbonaby)
|
Thanks @PlagueHO I'll update the PR based on your suggestions |
There was a problem hiding this comment.
Reviewable status: 7 of 9 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md line 8 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
For consistency, can you use this format?
thanks will update
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 181 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Does this parameter differ from the Size parameter being left empty above? This automatically forces the disk to use all remaining unallocated space for the volume.
I think I might be getting confused though (after looking at the example) - aren't volumes & partitions going to be always created using unallocated space?
I guess what I'm asking is: is this parameter needed?
So, the issue here is when the user wants to use a specific size, and not use all the remaining unallocated space when there is an existing volume on the disk, that is the same size as what the user enters in. Correct me if I'm wrong because I may have misunderstood, but I believe in this case, the Disk resource would actually use that same volume it found that matches the users size. To prevent this from happening that's what the use allocated space flag would be for.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 415 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
If
$Sizeis not specified and there is no matching partition found and empty space then$partitionshould be null, so I think (I could be wrong) that$UseUnallocatedSpaceisn't needed?
So the case for the UseUnallocatedSpace flag would be for when the size is specified. The problem is actually stemming from source/DSCResources/DSC_Disk/DSC_Disk.psm1:348, the Disk resource changes the $partition value to match a partition that is the same size the user enters in. The user may not want to format the partition it finds, but instead only use unallocated space if there is any. Let me know if you think there is a better way to address this though. If I remember correctly that was the reason why I ended up adding this flag.
On a side note, ultimately what I would want is for the ability for the Disk resource to resize an existing volume to create enough unallocated space for a Dev Drive or any other kind of volume all in one go if needed, to create a new volume. Right now, in the Disk resource you'd have to specify the size you want a volume to be first, then actually create another volume using that new unallocated space. This means you have to know the initial size of the first volume. E.g if My C drive was 200 Gb on disk 1 that has a max of 200 Gb and I want to create another volume that's 50 Gb on the same disk. In a config file I'd first have to tell the Disk resource that I want my C drive to be 150 Gb. Then separately in the config file use the Disk resource again to create a new volume that's 50 Gb using this newly unallocated space. Instead, I'd just want to only use the Disk resource once in a config file to create a new volume on a specific disk. The Disk resource would then resize an existing volume internally to create this new volume if it isn't already created. This way the user wouldn't have to know the size of the volume that will be resized, they can just expect that a volume of their specified size and parameters will be created if it isn't created already. I was thinking this would be a separate PR to add this functionality in.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 584 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
We can't assume that Test has fired, so can we also assert the DevDrive requirements here?
alright will update this thanks
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 626 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
We can't assume that Test has fired, so can we also assert the DevDrive requirements here?
alright will update this thanks
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 796 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This block should be down further as it will fail if the disk isn't yet initialized.
It also looks like it sort of duplicates some of the code/checks from the code following (e.g., from about here onwards: StorageDsc/source/DSCResources/DSC_Disk/DSC_Disk.psm1 at main · dsccommunity/StorageDsc (github.com))
E.g., right now there are two free space size checks performed if a DevDrive is used - one check here and another on line StorageDsc/source/DSCResources/DSC_Disk/DSC_Disk.psm1 at main · dsccommunity/StorageDsc (github.com)
Can this code be merged together to reduce duplicate functionality?
Hmm the only difference here I guess is that Dev Drive have to be 50 Gb or more. The Assert-DiskHasEnoughSpaceToCreateDevDrive check below is really to confirm that the user has enough space to create one and if not we throw. But actually for uninitialized disks, do you mean ones with out a partition style? I updated the check to include the check for the partition style last week and the check now works for uninitialized disks. But I don't mind moving it down if you still want me to. I think the place where you outlined only hits when we find a partition with the user inputted drive letter, while this one checks both I believe. I'll try taking a crack at moving lines
- source/DSCResources/DSC_Disk/DSC_Disk.psm1:802
- through
- source/DSCResources/DSC_Disk/DSC_Disk.psm1:827
down and see if I can merge the two.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 809 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This is interesting: we are using the
Get-PartitionSupportedSizeto get the largest free extent value, but it's pretty flaky. So perhaps it would be better to convert to using disk.LargestFreeExtent?
only issue is for situations where we have something like this on a 231 Gb disk for example:
Start disk -> vol1 (50GB) -> 60 gb unallocated space -> vol2 (51) Gb -> 70 Gb unallocated space -> End disk
doing Get-PartitionSupportedSize on vol1 would say the max size is 110 Gb. (Adding the current 50 Gb and the next 60 Gb of unallocated space next to it) but doing $disk.LargestFreeExtent would return 70 Gb, which is what we wouldn't want I believe. But in cases where we only want the highest extent (regardless of its location/position on the disk) then $disk.LargestFreeExtent would be fine.
source/DSCResources/DSC_Disk/DSC_Disk.schema.mof line 16 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
See previous comments - how does this differ from when size parameter is not specified?
answered above with my thoughts, let me know if I'm miss understanding something, Thanks
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 22 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you also specify that the Disk will be formatted as ReFS if not already formatted. If it is already formatted (but not ReFS) then will throw an exception.
Alright will do. I assume if I added the "AllowDestructive" tag here that would eliminate the Disk resource throwing right?
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 50 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I'm still not 100% sure I understand this parameter purpose. If this was $false and E drive did not exist, what would happen?
So, it would still create the volume as normal today on the unallocated space. I suppose I could have been clearer as well. But the UseUnallocatedSpace is really only necessary when the user enters a Size. This prevents the Disk resource from attempting to use a random volume on the disk that happens to have the same size as what the user enters in and will only use unallocated space if it doesn't exist. If you think of a better name let me know, thanks
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 271 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nit: end with full stop (for autodocs)
sure will do
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 274 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nit: end with full stop (for autodocs)
Sure will do
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 276 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should this be
Invoke-IsApiSetImplemented?
sure will update
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 294 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nit: end with full stop (for autodocs)
sure will do
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 296 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
For consistency, should this be
Get-DevDriveEnablementState?
Yea I can update this, thanks
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 321 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: can use single quotes here.
sure will do
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 323 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: add blank line above.
sure will do
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 429 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: add blank line above.
sure will do
|
Sorry about the delay @bbonaby - I'll get back to your comments in the weekend. |
|
Hey @PlagueHO I plan on updating this PR today. If possible would you be able to hold off until I update it? I've gotten rid of the 'UseUnallocatedSpace' and would like you to take a look at a different approach I did without needing that flag. Thanks |
…w we perform a resize automatically if there is not enough space.
There was a problem hiding this comment.
I updated the PR, we should be good to go for another round of review. This time I added alot more details and comments explaining the usage. I also updated the disk resource description with how the resource works with the Dev Drive flag. I reworked it so we automatically will resize a partition on the disk should we need more space to create the Dev Drive volume. I also added in the Description for the disk dsc that we don't have integration tests for the Dev Drive flag as its not available in server 2019 and server 2022. So I had to test the scenarios manually, which is where I found an existing bug in in dsc resource that actually formatted one of my partitions unnecessary :( , but I fixed it and added a comment in the latest commit.
Reviewed 1 of 11 files at r3.
Reviewable status: 2 of 12 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md line 8 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
thanks will update
Fixed in latest commit. Thanks.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 181 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
So, the issue here is when the user wants to use a specific size, and not use all the remaining unallocated space when there is an existing volume on the disk, that is the same size as what the user enters in. Correct me if I'm wrong because I may have misunderstood, but I believe in this case, the Disk resource would actually use that same volume it found that matches the users size. To prevent this from happening that's what the use allocated space flag would be for.
I removed the parameter in the latest commit as its no longer needed. Thanks
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 415 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
So the case for the
UseUnallocatedSpaceflag would be for when the size is specified. The problem is actually stemming from source/DSCResources/DSC_Disk/DSC_Disk.psm1:348, the Disk resource changes the$partitionvalue to match a partition that is the same size the user enters in. The user may not want to format the partition it finds, but instead only use unallocated space if there is any. Let me know if you think there is a better way to address this though. If I remember correctly that was the reason why I ended up adding this flag.On a side note, ultimately what I would want is for the ability for the Disk resource to resize an existing volume to create enough unallocated space for a Dev Drive or any other kind of volume all in one go if needed, to create a new volume. Right now, in the Disk resource you'd have to specify the size you want a volume to be first, then actually create another volume using that new unallocated space. This means you have to know the initial size of the first volume. E.g if My C drive was 200 Gb on disk 1 that has a max of 200 Gb and I want to create another volume that's 50 Gb on the same disk. In a config file I'd first have to tell the Disk resource that I want my C drive to be 150 Gb. Then separately in the config file use the Disk resource again to create a new volume that's 50 Gb using this newly unallocated space. Instead, I'd just want to only use the Disk resource once in a config file to create a new volume on a specific disk. The Disk resource would then resize an existing volume internally to create this new volume if it isn't already created. This way the user wouldn't have to know the size of the volume that will be resized, they can just expect that a volume of their specified size and parameters will be created if it isn't created already. I was thinking this would be a separate PR to add this functionality in.
I reworked how the functionality works when the Dev Drive flag is set to true. Now what I spoke about in paragraph 2 is reality with this latest commit. Thanks.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 584 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
alright will update this thanks
updated, thanks.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 626 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
alright will update this thanks
updated, thanks.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 796 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
Hmm the only difference here I guess is that Dev Drive have to be 50 Gb or more. The
Assert-DiskHasEnoughSpaceToCreateDevDrivecheck below is really to confirm that the user has enough space to create one and if not we throw. But actually for uninitialized disks, do you mean ones with out a partition style? I updated the check to include the check for the partition style last week and the check now works for uninitialized disks. But I don't mind moving it down if you still want me to. I think the place where you outlined only hits when we find a partition with the user inputted drive letter, while this one checks both I believe. I'll try taking a crack at moving lines
- source/DSCResources/DSC_Disk/DSC_Disk.psm1:802
- through
- source/DSCResources/DSC_Disk/DSC_Disk.psm1:827
down and see if I can merge the two.
updated thanks. I removed that entire block and function. Instead I did what you suggested and did the checks closer to where the we create a new partition. That made it easier to just use Assert-SizeMeetsMinimumDevDriveRequirement and either the partition size , volume size, user inputted size and the $disk.LargestFreeExtent sizes where appropriate.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 725 at r3 (raw file):
{ $partition = $assignedPartition }
Fyi this is the bug I was talking about in another comment that ended up formatting one my unrelated partitions. Its I fixed with this but let me know if you have any issues. thanks
source/DSCResources/DSC_Disk/DSC_Disk.schema.mof line 16 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
answered above with my thoughts, let me know if I'm miss understanding something, Thanks
removed the parameter, thanks.
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 274 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
Sure will do
updated thanks, missed changing this name so I updated this too.
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 276 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
sure will update
updated , thanks
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 321 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
sure will do
updated, thanks
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 323 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
sure will do
updated, thanks
source/Modules/StorageDsc.Common/StorageDsc.Common.psm1 line 429 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
sure will do
simplified function, thanks
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 22 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
Alright will do. I assume if I added the "AllowDestructive" tag here that would eliminate the Disk resource throwing right?
I updated the description to provide users with a scenario and what this configuration will do. Thanks
source/Examples/Resources/Disk/3-Disk_InitializeDiskWithADevDrive.ps1 line 50 at r2 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
So, it would still create the volume as normal today on the unallocated space. I suppose I could have been clearer as well. But the UseUnallocatedSpace is really only necessary when the user enters a Size. This prevents the Disk resource from attempting to use a random volume on the disk that happens to have the same size as what the user enters in and will only use unallocated space if it doesn't exist. If you think of a better name let me know, thanks
removed in latest commit, thanks.
… and 2022. Had to mock the Assert-DevDriveFeatureAvailable function
…dded after this is done
|
@PlagueHO any update on this? Thanks |
|
Hi @bbonaby - apologies, day job has been taking up all my bandwidth. Planning to have time for this tomorrow night. |
There was a problem hiding this comment.
Looking really awesome @bbonaby - just some super minor things and we can merge. Due to the small logic changes to Set/Test, I think it is worth marking this as a BREAKING CHANGE, just to bump the major version and avoid any configs unexpectedly breaking.
Reviewed 4 of 11 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @bbonaby)
CHANGELOG.md line 8 at r4 (raw file):
## [Unreleased] - Updated DSC_Disk to allow volumes to be formatted as Dev Drives - Fixes [Issue #276](https://github.com/dsccommunity/StorageDsc/issues/276)
I think we should make this a BREAKING CHANGE: just because it does change some existing behaviors that "could" break existing configs. This is fine though and will just bump major version to 6.
Also, can you add ### Added before change line above (with a blank line before it). E.g.
### Added
- Disk:
- BREAKING CHANGE: Added support for volumes to be formatted as Dev Drives - Fixes [Issue #276](https://github.com/dsccommunity/StorageDsc/issues/276)
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 725 at r3 (raw file):
Previously, bbonaby (bbonaby-MSFT) wrote…
Fyi this is the bug I was talking about in another comment that ended up formatting one my unrelated partitions. Its I fixed with this but let me know if you have any issues. thanks
Good catch. We didn't used to consider running Set without Test, but this is required now.
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 402 at r4 (raw file):
$tempPartition.Type -eq 'Reserved' -or ` $tempPartition.Type -eq 'Recovery' ` )
Can this be?
Suggestion:
$shouldNotBeResized = ($tempPartition.Type -in 'System','Reserved','Recovery')source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 416 at r4 (raw file):
Write-Verbose -Message ( @( "$($MyInvocation.MyCommand): " $($script:localizedData.CheckingIfPartitionCanBeResizedForDevDrive -F $tempPartition.DriveLetter)
nit: change to lower case -f
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 440 at r4 (raw file):
Write-Verbose -Message ( @( "$($MyInvocation.MyCommand): " $($script:localizedData.PartitionFoundThatCanBeResizedForDevDrive -F $tempPartition.DriveLetter)
nit: change to lower case -f
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 451 at r4 (raw file):
Write-Verbose -Message ( @( "$($MyInvocation.MyCommand): " $($script:localizedData.PartitionCantBeResizedForDevDrive -F $tempPartition.DriveLetter)
nit: change to lower case -f
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 460 at r4 (raw file):
{ $SizeInGb = [Math]::Round($Size / 1GB, 2) throw ($script:localizedData.FoundNoPartitionsThatCanResizedForDevDrive -F $SizeInGb)
nit: change to lower case -f
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 465 at r4 (raw file):
else { # Find the first basic partition matching the size
nit: to make tracing this code easier can you add to this comment that this is "when not a DevDrive"?
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 731 at r4 (raw file):
<# If the Set-TargetResource function is run as a standalone function, and $assignedPartition is not null and there are multiple partitions in $partition,
nit: Can you reduce the line length of these comments (sorry, just an old formatting thing from the style guidelines).
source/DSCResources/DSC_Disk/DSC_Disk.psm1 line 1099 at r4 (raw file):
if ($DevDrive) { <#
Should we separate this out into a common function? E.g., a "Compare-PartitionSizeUsingGB" - or something? This is also because the same functionality is used in Set as well.
tests/Unit/DSC_Disk.Tests.ps1 line 36 at r4 (raw file):
{ InModuleScope $script:dscResourceName { $script:testDriveLetter = 'G'
nit: just for consistency and to make it easier to detect mismatches on the test variables do you want to change $script:testDriveLetter to $script:testDriveLetterG?
tests/Unit/DSC_Disk.Tests.ps1 line 1403 at r4 (raw file):
It "Should return DevDrive as $($true)" { $resource.DevDrive | Should -Be $true
Can change this to be Should -BeTrue throughout (and also for false can be Should -BeFalse)
tests/Unit/DSC_Disk.Tests.ps1 line 3036 at r4 (raw file):
} Context 'When the Dev Drive flag is true, the AllowDestructive flag is false and there is not enough space on the disk to create the partition' {
nit: Dev Drive -> DevDrive (helps indicate a param rather than something else .... although a real nit :D )
tests/Unit/DSC_Disk.Tests.ps1 line 3088 at r4 (raw file):
} Context 'When the Dev Drive flag is true, AllowDestructive is false and there is enough space on the disk to create the partition' {
nit: Dev Drive -> DevDrive (helps indicate a param rather than something else .... although a real nit :D )
tests/Unit/DSC_Disk.Tests.ps1 line 3191 at r4 (raw file):
Mock -CommandName Initialize-Disk It 'Should throw an exception' {
nit: Can say Should throw an exception stating that AllowDestructive flag needs to be set to resize existing partition for DevDrive 😀
tests/Unit/DSC_Disk.Tests.ps1 line 3222 at r4 (raw file):
-ParameterFilter $script:parameterFilter_MockedDisk0Number ` -MockWith { $script:amountOfTimesGetDiskByIdentifierIsCalled++
Unlikely to be an issue, but could this be zeroed at the beginning of this Context block in case the code/test is reused?
tests/Unit/DSC_Disk.Tests.ps1 line 3250 at r4 (raw file):
Mock ` -CommandName Get-PartitionSupportedSize ` -MockWith { & Get-PartitionSupportedSizeForDevDriveScenarios $DriveLetter } `
Can include parameter name on the $DriveLetter parameter?
tests/Unit/DSC_Disk.Tests.ps1 line 3312 at r4 (raw file):
} Context 'When the Dev Drive flag is true, AllowDestructive is true, and a Partition that matches the users drive letter exists.' {
nit: no need for full stop at end of Context 😀
tests/Unit/DSC_Disk.Tests.ps1 line 3347 at r4 (raw file):
Mock ` -CommandName Assert-DevDriveFeatureAvailable ` -MockWith { $null } `
I think we can drop the MockWith parameter entirely here?
tests/Unit/DSC_Disk.Tests.ps1 line 3383 at r4 (raw file):
} Context 'When the Dev Drive flag is true, AllowDestructive is false, and a Partition that matches the users drive letter exists.' {
nit: Dev Drive to DevDrive 😀 and no need for full stop (only mentioned due to Pester output formatting in Azure DevOps .
tests/Unit/DSC_Disk.Tests.ps1 line 3431 at r4 (raw file):
-Verbose } | Should -Throw -ExpectedMessage ($script:localizedData.FailedToConfigureDevDriveVolume ` -F $script:mockedVolumeThatExistPriorToConfiguration.UniqueId, $script:testDriveLetterT)
nit: -f 😀
tests/Unit/DSC_Disk.Tests.ps1 line 4536 at r4 (raw file):
-AllowDestructive $true ` -Verbose } | Should -Throw -ExpectedMessage ($script:localizedCommonStrings.MinimumSizeNeededToCreateDevDriveVolumeError -F $userDesiredSizeInGb)
nit: -F -> -f ... and throughout (will ignore any more).
tests/Unit/DSC_Disk.Tests.ps1 line 4547 at r4 (raw file):
} Context 'When the Dev Drive flag is true, but the partition is relatively the same size as user inputted size and volume is NTFS' {
Maybe change relatively to effectively?
tests/Unit/DSC_Disk.Tests.ps1 line 4655 at r4 (raw file):
} Context 'When the Dev Drive flag is true, but the partition is relatively the same size as user inputted size, volume is ReFS formatted and is Dev Drive volume' {
Could change relatively to effectively?
tests/Unit/DSC_Disk.Tests.ps1 line 4714 at r4 (raw file):
} Context 'When the Dev Drive flag is true, but the partition is relatively the same size as user inputted size, volume is ReFS formatted and is not Dev Drive volume' {
Could change relatively to effectively?
There was a problem hiding this comment.
Reviewable status: 7 of 12 files reviewed, 28 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md line 8 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think we should make this a
BREAKING CHANGE:just because it does change some existing behaviors that "could" break existing configs. This is fine though and will just bump major version to 6.Also, can you add
### Addedbefore change line above (with a blank line before it). E.g.### Added - Disk: - BREAKING CHANGE: Added support for volumes to be formatted as Dev Drives - Fixes [Issue #276](https://github.com/dsccommunity/StorageDsc/issues/276)
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 36 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: just for consistency and to make it easier to detect mismatches on the test variables do you want to change
$script:testDriveLetterto$script:testDriveLetterG?
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 1403 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can change this to be
Should -BeTruethroughout (and also for false can beShould -BeFalse)
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3036 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Dev Drive -> DevDrive (helps indicate a param rather than something else .... although a real nit :D )
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3088 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Dev Drive -> DevDrive (helps indicate a param rather than something else .... although a real nit :D )
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3191 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Can say
Should throw an exception stating that AllowDestructive flag needs to be set to resize existing partition for DevDrive😀
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3222 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Unlikely to be an issue, but could this be zeroed at the beginning of this Context block in case the code/test is reused?
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3250 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can include parameter name on the $DriveLetter parameter?
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3312 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: no need for full stop at end of Context 😀
Thanks, updated this
tests/Unit/DSC_Disk.Tests.ps1 line 3347 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I think we can drop the
MockWithparameter entirely here?
Thanks, updated this and every where else I used this.
tests/Unit/DSC_Disk.Tests.ps1 line 3383 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: Dev Drive to DevDrive 😀 and no need for full stop (only mentioned due to Pester output formatting in Azure DevOps .
Thanks, updated this and everywhere else I did this as well.
tests/Unit/DSC_Disk.Tests.ps1 line 3431 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit:
-f😀
Thanks, updated this and everywhere else I did this as well.
tests/Unit/DSC_Disk.Tests.ps1 line 4536 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: -F -> -f ... and throughout (will ignore any more).
thanks updated
tests/Unit/DSC_Disk.Tests.ps1 line 4547 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Maybe change
relativelytoeffectively?
thanks updated
tests/Unit/DSC_Disk.Tests.ps1 line 4655 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Could change
relativelytoeffectively?
thanks updated
tests/Unit/DSC_Disk.Tests.ps1 line 4714 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Could change
relativelytoeffectively?
thanks updated
|
Thanks @PlagueHO I updated the PR and addressed your comments. |
|
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I created a generic function called Compare-SizeUsingGB so we could reuse the same functionality here. Thanks |
|
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Done. |
|
@PlagueHO Thanks for the help with this, if you're all good with these latest changes, would it be possible to get this merged in today or tomorrow? |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @bbonaby)
Pull Request (PR) description
This PR adds Dev Drive creation support and the ability to explicitly use unallocated space to create a new partition even when there is a partition with the same size as the one the user enters in.
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