dsccommunity / StorageDsc

DSC resource module is used to manage storage on Windows Servers.
https://dsccommunity.org
MIT License
66 stars 51 forks source link

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

Open bbonaby opened 11 months ago

bbonaby commented 11 months ago

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


This change is Reviewable

codecov[bot] commented 11 months ago

Codecov Report

Attention: Patch coverage is 93.22917% with 13 lines in your changes missing coverage. Please review.

Project coverage is 94%. Comparing base (a87725b) to head (3ebf094). Report is 4 commits behind head on main.

Files Patch % Lines
...lpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1 86% 11 Missing :warning:
...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1 50% 2 Missing :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279/graphs/tree.svg?width=650&height=150&src=pr&token=RLLNitScbf&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity)](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) ```diff @@ Coverage Diff @@ ## main #279 +/- ## ==================================== - Coverage 95% 94% -1% ==================================== Files 7 9 +2 Lines 1025 1238 +213 ==================================== + Hits 977 1176 +199 - Misses 48 62 +14 ``` | [Files](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity) | Coverage Δ | | |---|---|---| | [...urces/DSC\_VirtualHardDisk/DSC\_VirtualHardDisk.psm1](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279?src=pr&el=tree&filepath=source%2FDSCResources%2FDSC_VirtualHardDisk%2FDSC_VirtualHardDisk.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL0RTQ1Jlc291cmNlcy9EU0NfVmlydHVhbEhhcmREaXNrL0RTQ19WaXJ0dWFsSGFyZERpc2sucHNtMQ==) | `100% <100%> (ø)` | | | [...e/Modules/StorageDsc.Common/StorageDsc.Common.psm1](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279?src=pr&el=tree&filepath=source%2FModules%2FStorageDsc.Common%2FStorageDsc.Common.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL01vZHVsZXMvU3RvcmFnZURzYy5Db21tb24vU3RvcmFnZURzYy5Db21tb24ucHNtMQ==) | `89% <50%> (-3%)` | :arrow_down: | | [...lpers/StorageDsc.VirtualHardDisk.Win32Helpers.psm1](https://app.codecov.io/gh/dsccommunity/StorageDsc/pull/279?src=pr&el=tree&filepath=source%2FModules%2FStorageDsc.VirtualHardDisk.Win32Helpers%2FStorageDsc.VirtualHardDisk.Win32Helpers.psm1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dsccommunity#diff-c291cmNlL01vZHVsZXMvU3RvcmFnZURzYy5WaXJ0dWFsSGFyZERpc2suV2luMzJIZWxwZXJzL1N0b3JhZ2VEc2MuVmlydHVhbEhhcmREaXNrLldpbjMySGVscGVycy5wc20x) | `86% <86%> (ø)` | |
bbonaby commented 11 months ago

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

PlagueHO commented 11 months ago

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

PlagueHO commented 10 months ago

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

bbonaby commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
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 :grinning: )

updated and added a note about how they shouldn't be used together. I started the sentence off with 'Note:' . Let me know if you rather me put 'Warning:' thanks.

bbonaby commented 10 months ago

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
See comments about markdown autonumbering above. Can also use the VS Markdown linting extension to automatically get this suggestion.

updated, thanks for the tip 😊 hope it looks better now. Let me know if you see any more issues. Thanks

bbonaby commented 10 months ago

source/Modules/VirtualHardDisk.Win32Helpers/VirtualHardDisk.Win32Helpers.psm1 line at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
To help associate any imported modules with the DSC resource, can we name this as `StorageDsc.VirtualHardDisk.Win32Helpers` - or something to that effect?

updated and changed the name to StorageDsc.VirtualHardDisk.Win32Helpers thanks

bbonaby commented 10 months ago

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

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nit: wrap .vhd and .vhdx in backtick to highlight as code (even though not really code :D)

updated thanks

johlju commented 10 months ago

@bbonaby please rebase this on (pull in changes from main) and hopefully @PlagueHO will come around soon and finish the review. 🙂

PlagueHO commented 10 months ago

@bbonaby - can you resolve the conflicts on this one (caused #283).

bbonaby commented 10 months ago

Thanks @PlagueHO conflicts should be all resolved now

bbonaby commented 9 months ago

Hey @PlagueHO any update on this?

bbonaby commented 6 months ago

hey @PlagueHO any update on this PR?

PlagueHO commented 6 months ago

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.

bbonaby commented 5 months ago

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 commented 5 months ago

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

johlju commented 3 weeks ago

For the build to pass the line: https://github.com/dsccommunity/StorageDsc/blob/06a30de47e7adf41e86efc1b9ca9d2a7c1540dee/azure-pipelines.yml#L30

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.

bbonaby commented 1 week ago

@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 commented 1 week ago

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