Disk-AccessPath: fixing get-partition to pull current state of object #198

JoshuaJSwain · 2019-03-04T23:29:25Z

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in the resource folder.
Resource parameter descriptions added/updated in 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 Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

codecov-io · 2019-03-04T23:38:13Z

Codecov Report

Merging #198 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           dev   #198    +/-   ##
===================================
+ Coverage   94%    94%   +<1%     
===================================
  Files        8      8            
  Lines      889    890     +1     
===================================
+ Hits       837    838     +1     
  Misses      52     52

johlju

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JoshuaJSwain)

a discussion (no related file):
Please look at the failing tests here:
https://ci.appveyor.com/project/PowerShell/storagedsc/builds/22819645?fullLog=true#L2514

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 522 at r1 (raw file):

$assignedPartition

This is already assigned on line 285 to the same as before this change. What is the difference with the line 285 and this line, with this change?
Now it only returns the specific partition number if the access path is the same as the user provided. Line 285 will return the first partition with the access path the same as the user provided?

This might just be me not understanding what you trying to achieve with this change, could you please explain? 😃

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 532 at r1 (raw file):

# Setting the partition property NoDefaultDriveLetter to True to prevent adding drive letter on reboot

I think this should say that we set the NoDefaultDriveLetter to the value provided by the user, or $true if the user did not provide a value?

PlagueHO

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johlju and @JoshuaJSwain)

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

- Clean up module manifest to correct Author and Company - fixes
  [Issue #191](https://github.com/PowerShell/StorageDsc/issues/191).
- Fix a bug where current state of NoDefaultDriveLetter state

This reads a bit odd - could it be:
Fix a bug where the current state of NoDefaultDriveLetter is not handled. Added a refresh of the current partition information prior to checking state - fixes [...]

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 522 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$assignedPartition
This is already assigned on line 285 to the same as before this change. What is the difference with the line 285 and this line, with this change?
Now it only returns the specific partition number if the access path is the same as the user provided. Line 285 will return the first partition with the access path the same as the user provided?

This might just be me not understanding what you trying to achieve with this change, could you please explain? 😃

I suspect you might be doing this because we may have changed (or created) the partition within one of the prior if blocks. However, this code doesn't really make sense anyway - That is going to get All Partitions on ALL disks that match the partition number.

It appears all you're trying to do is get the current state of the partition we're working on into the $assignedPartition variable. You'd just need this:

$assignedPartition = $partition | Get-Partition

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 525 at r1 (raw file):

        Where-Object -Property AccessPaths -Contains -Value $AccessPath

        if ($assignedPartition.NoDefaultDriveLetter -ne $NoDefaultDriveLetter)

Also this is not indented at the right level.

JoshuaJSwain

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @johlju, @JoshuaJSwain, and @PlagueHO)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Please look at the failing tests here:
https://ci.appveyor.com/project/PowerShell/storagedsc/builds/22819645?fullLog=true#L2514

Done.

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 522 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I suspect you might be doing this because we may have changed (or created) the partition within one of the prior if blocks. However, this code doesn't really make sense anyway - That is going to get All Partitions on ALL disks that match the partition number.

It appears all you're trying to do is get the current state of the partition we're working on into the $assignedPartition variable. You'd just need this:
$assignedPartition = $partition | Get-Partition

Correct, I just need to pull in the current state, changes updated.

DSCResources/MSFT_DiskAccessPath/MSFT_DiskAccessPath.psm1, line 525 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Also this is not indented at the right level.

Done.

JoshuaJSwain · 2019-03-05T14:22:01Z

Updated with the patch. I want to run this through some integration tests before calling it good.

JoshuaJSwain · 2019-03-05T14:35:25Z

Updated with the patch. I want to run this through some integration tests before calling it good.

I've tested this in a live environment and confirmed this fix.

johlju

@PlagueHO I have resolved my comments. I leave it to you to approve the last changes.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @johlju, @JoshuaJSwain, and @PlagueHO)

PlagueHO

I just noticed the unit tests for the resource are not actually that good.

All the Assert-MockCalled functions are missing the -Exactly parameter. This means the tests aren't as effective as they could be.

@JoshuaJSwain - I'm going to quickly try and get a PR through to correct the unit tests for this resource so we can be sure we're performing the expected calls. Once my PR is through you'll need to rebase this one - it'll likely cause the unit tests to fail: this is what alerted me to the problem - I'd expect your change to break the unit tests.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO · 2019-03-08T22:20:45Z

@JoshuaJSwain - when this one #201 goes through then you can rebase and see if your tests still pass (I'd expect some to fail).

PlagueHO · 2019-03-25T05:47:46Z

Ok @JoshuaJSwain - #201 has gone through. Can you rebase and see if your tests pass and correct if necessary?

PlagueHO · 2019-03-28T08:15:59Z

Hi @JoshuaJSwain - this is what I expected- the tests to fail as they weren't actually very good. Now they are better tests you will need to correct them to validate your code correctly.

JoshuaJSwain · 2019-03-28T09:42:15Z

I take a look at this one week of 4/8. Should be straight forward to fix them up. Thanks! Josh Swain

…

On Thu, Mar 28, 2019, 5:25 AM Daniel Scott-Raynsford < ***@***.***> wrote: Hi @JoshuaJSwain <https://github.com/JoshuaJSwain> - this is what I expected- the tests to fail as they weren't actually very good. Now they are better tests you will need to correct them to validate your code correctly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AdawlETt0V1NeKeptgjAxeYQIHwPuTKMks5vbHpBgaJpZM4bdbOw> .

PlagueHO

Cool! Awesome stuff and sorry about the delay.

I just realized- there isn't an entry in the CHANGELOG.MD for this - can you add something under the # Unreleased section? Then we're good to go. Sorry for not picking up earlier.

Reviewed 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

JoshuaJSwain

oops, updated the changelog. thanks!

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @PlagueHO)

PlagueHO

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JoshuaJSwain)

CHANGELOG.md, line 6 at r4 (raw file):

- DiskAccessPath:
  - Added a get-partion to properly handle setting the NoDefaultDriveLetter

I think 'partition' is spelled wrong. Also, can you use Get-Partition (e.g. capitization)

CHANGELOG.md, line 7 at r4 (raw file):

- DiskAccessPath:
  - Added a get-partion to properly handle setting the NoDefaultDriveLetter
    parameter - fixes [Issue #198](https://github.com/PowerShell/StorageDsc/pull/198)

Nitpick: Can you add a full stop on the end here after the ')'. Sorry for being a pain 😁

JoshuaJSwain

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md, line 6 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think 'partition' is spelled wrong. Also, can you use Get-Partition (e.g. capitization)

Done.

CHANGELOG.md, line 7 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Nitpick: Can you add a full stop on the end here after the ')'. Sorry for being a pain 😁

Done.

PlagueHO

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

JoshuaJSwain mentioned this pull request Mar 4, 2019

xDiskAccessPath: Mounted disks get drive letter on reboot #145

Closed

johlju reviewed Mar 5, 2019

View reviewed changes

PlagueHO added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Mar 5, 2019

PlagueHO requested changes Mar 5, 2019

View reviewed changes

JoshuaJSwain commented Mar 5, 2019

View reviewed changes

johlju reviewed Mar 5, 2019

View reviewed changes

PlagueHO approved these changes Mar 8, 2019

View reviewed changes

Josh Swain added 4 commits April 11, 2019 15:33

fixing get-partition to pull current state

8ea6e86

changing logic to get partition state

dba31ef

code review updates

bd6e0b1

get-partition called 2 times for nodefautldriveletter unit tests

e409232

JoshuaJSwain force-pushed the jswain_accesspath_145 branch from 98124c8 to e409232 Compare April 11, 2019 19:39

PlagueHO approved these changes Apr 13, 2019

View reviewed changes

update changelog for PR 198

9b8ef58

JoshuaJSwain commented Apr 13, 2019

View reviewed changes

PlagueHO requested changes Apr 13, 2019

View reviewed changes

JoshuaJSwain added 2 commits April 13, 2019 18:56

update changelog for typo spelling

cebcfd8

update changelog

0a3e30e

JoshuaJSwain commented Apr 13, 2019

View reviewed changes

PlagueHO approved these changes Apr 15, 2019

View reviewed changes

PlagueHO merged commit d1683a4 into dsccommunity:dev Apr 15, 2019

kwirkykat removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disk-AccessPath: fixing get-partition to pull current state of object #198

Disk-AccessPath: fixing get-partition to pull current state of object #198

JoshuaJSwain commented Mar 4, 2019 •

edited

Loading

codecov-io commented Mar 4, 2019 •

edited

Loading

johlju left a comment

PlagueHO left a comment

JoshuaJSwain left a comment

JoshuaJSwain commented Mar 5, 2019

JoshuaJSwain commented Mar 5, 2019

johlju left a comment

PlagueHO left a comment

PlagueHO commented Mar 8, 2019

PlagueHO commented Mar 25, 2019

PlagueHO commented Mar 28, 2019

JoshuaJSwain commented Mar 28, 2019 via email

PlagueHO left a comment

JoshuaJSwain left a comment

PlagueHO left a comment

JoshuaJSwain left a comment

PlagueHO left a comment

Disk-AccessPath: fixing get-partition to pull current state of object #198

Disk-AccessPath: fixing get-partition to pull current state of object #198

Conversation

JoshuaJSwain commented Mar 4, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented Mar 4, 2019 • edited Loading

Codecov Report

johlju left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

JoshuaJSwain left a comment

Choose a reason for hiding this comment

JoshuaJSwain commented Mar 5, 2019

JoshuaJSwain commented Mar 5, 2019

johlju left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO commented Mar 8, 2019

PlagueHO commented Mar 25, 2019

PlagueHO commented Mar 28, 2019

JoshuaJSwain commented Mar 28, 2019 via email

PlagueHO left a comment

Choose a reason for hiding this comment

JoshuaJSwain left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

JoshuaJSwain left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

JoshuaJSwain commented Mar 4, 2019 •

edited

Loading

codecov-io commented Mar 4, 2019 •

edited

Loading