xAccessPath: Access Path not found - Fixes #121 #126

positivism1 · 2017-11-13T20:35:55Z

Pull Request (PR) description

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

Task list:

Change details added to Unreleased section of CHANGELOG.md?
Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
Examples appropriately updated?
New/changed code adheres to Style Guidelines?
Unit and (optional) Integration tests created/updated where possible?

This change is

Added refresh of PS Drive using a try/catch block.

To fix issue 121 - Access Path not found.

Verbose Messaging in support of PSDrive refreshing.

msftclas · 2017-11-13T20:36:04Z

All CLA requirements met.

codecov-io · 2017-11-13T20:41:47Z

Codecov Report

Merging #126 into dev will decrease coverage by <1%.
The diff coverage is 55%.

@@        Coverage Diff         @@
##           dev   #126   +/-   ##
==================================
- Coverage   94%    94%   -1%     
==================================
  Files        5      5           
  Lines      671    680    +9     
==================================
+ Hits       636    641    +5     
- Misses      35     39    +4

PlagueHO · 2017-11-17T21:14:33Z

Thank you very much @positivism1 for submitting this! It is greatly appreciated and a really awesome job. Review comments and changes are completely normal and expected part of the process 🍰 We all get them (any of the people who have reviewed my code will agree 😉 ). Keep up the awesome work!

If you make the changes below and just push back to your branch then hit the Reviewable button on your PR you will be able to click Done on any of the changes you've made and I can continue the review.

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 11 at r1 (raw file):

- xDiskAccessPath:
  - Added support for Guid Disk Id type - See [Issue 104](https://github.com/PowerShell/xStorage/issues/104).
  - Added check for PSDrive existance  to solve bug - See [Issue 121](https://github.com/PowerShell/xStorage/issues/121).

Please remove extra space between existance and to.

Also existance should be spelled existence. 😁

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1, line 532 at r1 (raw file):

    )

  Try

I think this code actually needs to be moved into the Set-TargetResource rather than in Test-TargetResource. You'd want to add it just after the volume has been formatted. The reason is that the first time Test-TargetResource is hit and the disk isn't already assigned then Get-PSDrive will fire twice and then Set-TargetResource will create the new drive. But Get-PSDrive needs to fire after the drive has been created but before you try and use it in one of your following steps.

So, I reckon this code needs to go in somewhere around here:
https://github.com/PowerShell/xStorage/blob/dev/Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1#L466

Convert to lower case T in Try.

Also, the alignment seems a bit off on this code block and there are a few extra spaces at the end of these lines.

The easiest way to fix it is to use Visual Studio Code format document feature. I wrote a quick blog post showing how to use it:
https://dscottraynsford.wordpress.com/2017/11/18/auto-formatting-powershell-in-visual-studio-code/

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1, line 534 at r1 (raw file):

  Try 
    {
          $AccessPathDisk = $AccessPath.split(":")[0]

Please use lower case for first letter of local variables. E.g.

$AccessPathDisk = $AccessPath.split(":")[0]

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1, line 539 at r1 (raw file):

            $($localizedData.TestingPSDriveMessage -f $AccessPathDisk,$AccessPath)
        ) -join '' )
         $TestDrive = Get-PSDrive $AccessPathDisk -ErrorAction Stop

You could assign this to $null because the result will never be used. Visual Studio Code will underline this as a recommendation 😁

E.g.

$null = Get-PSDrive $AccessPathDisk -ErrorAction Stop

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1, line 541 at r1 (raw file):

         $TestDrive = Get-PSDrive $AccessPathDisk -ErrorAction Stop
    }
    Catch

Please use lower case C in catch.

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/MSFT_xDiskAccessPath.psm1, line 550 at r1 (raw file):

   
        Get-PSDrive | out-null

Assigning to $null is faster in older versions of PS (5.1 and below) rather than piping, so can you change to:

$null = Get-PSDrive

Also, how come this second Get-PSDrive is needed? The one above should be hit everytime.

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/en-us/MSFT_xDiskAccessPath.strings.psd1, line 1 at r1 (raw file):

Can you remove this extra blank line at the top here?

Modules/xStorage/DSCResources/MSFT_xDiskAccessPath/en-us/MSFT_xDiskAccessPath.strings.psd1, line 37 at r1 (raw file):

    TestingPSDriveMessage = Testing for availability of PSDrive {0} for AccessPath {1}
    UnavailablePSDriveMessage = PSDrive {0} is unavailable for AccessPath {1}, refreshing PSDrives.

Can you remove this extra blank line here?

Comments from Reviewable

positivism1 · 2017-11-21T10:44:01Z

Thank you! this is great, I will get to it as soon as I can. From what I remember from my testing, I put the change into Test-TargetResource because that is where the whole thing falls in a heap, when the test-path runs, it can't find the folder because psdrive need refreshing.

positivism1 · 2017-12-10T20:58:56Z

Getting back into this now. Apologies, have been away.

positivism1 · 2017-12-11T00:15:01Z

The psdrive refresh is in the Test-TargetResource because this is where the error occurs.

This code:
Get-PSDrive | out-null

Is the thing that does the work, only if this line errors..
$TestDrive = Get-PSDrive $AccessPathDisk -ErrorAction Stop

Without get-psdrive in catch block (error):
PowerShell DSC resource MSFT_xDiskAccessPath failed to execute Test-TargetResource functionality with error message: Access Path 'E:\DATA' is not found.
Parameter name: AccessPath
+ CategoryInfo : InvalidOperation: (:) [], CimException
+ FullyQualifiedErrorId : ProviderOperationExecutionFailure

With get-psdrive catch block (successful):
VERBOSE: [GDVRATST054]: [[xDiskAccessPath]dataAccessPath] Test-TargetResource: Testing disk with Number '2' status for access path 'E:\DATA'.
VERBOSE: [GDVRATST054]: [[xDiskAccessPath]dataAccessPath] Test-TargetResource: Testing for availability of PSDrive E for AccessPath E:\DATA
VERBOSE: [GDVRATST054]: [[xDiskAccessPath]dataAccessPath] Test-TargetResource: PSDrive E is unavailable for AccessPath E:\DATA, refreshing PSDrives.

Thanks, I will review the code, make changes and set it to Reviewable.

PlagueHO · 2017-12-11T07:06:24Z

Hi @positivism1 - it looks like you've got some merge conflicts that need to be fixed. These will have been caused when another PR got merged before yours. If you can go into Reviewable and click Done on any changes you've completed and the click Publish that will allow me to acknowledge any required changes. Thanks again for your hard word!

positivism1 · 2017-12-13T04:02:21Z

ahh yep ok thanks :-) I will try get this Publish button to work.

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 11 at r1 (raw file):