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

Fix xDiskAccessPath issue when reattaching volume - Fixes #66 #70

Merged

Conversation

PlagueHO
Copy link
Member

@PlagueHO PlagueHO commented Nov 20, 2016

This PR:

These are some PSSA violations still in xDisk and xDiskAccessPath:

- MSFT_xDisk.psm1 (Line 96): File 'MSFT_xDisk.psm1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
- MSFT_xDisk.psm1 (Line 533): File 'MSFT_xDisk.psm1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
- MSFT_xDiskAccessPath.psm1 (Line 98): File 'MSFT_xDiskAccessPath.psm1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
- MSFT_xDiskAccessPath.psm1 (Line 570): File 'MSFT_xDiskAccessPath.psm1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.

The WMI cmdlets should be able to be removed because they are fallback cmdlets if CIM does not work. Removing these may prevent older the resources from working in older OS's though. Unfortunately there were no code comments stating why the WMI cmdlets were included. @kwirkykat , @mbreakey3 - what is the thinking of the DSC Team on this?

This change contains the following fixes:

- Updated readme.md to remove markdown best practice rule violations.
- Updated readme.md to match DSCResources/DscResource.Template/README.md.
- xDiskAccessPath:
  - Fix bug when re-attaching disk after mount point removed or detatched.
  - Additional log entries added for improved diagnostics.
  - Additional integration tests added.
- Converted integration tests to use ```$TestDrive``` as working folder or ```temp``` folder when persistence across tests is required.
- Suppress ```PSUseShouldProcessForStateChangingFunctions``` rule violations in resources.
- Rename ```Test-AccessPath``` function to ```Assert-AccessPathValid```.
- Rename ```Test-DriveLetter``` function to ```Assert-DriveLetterValid```.
- Added ```CommonResourceHelper.psm1``` module (based on PSDscResources).
- Added ```CommonTestsHelper.psm1``` module  (based on PSDscResources).
- Converted all modules to load localization data using ```Get-LocalizedData``` from CommonResourceHelper.
- Converted all exception calls and tests to use functions in ```CommonResourceHelper.psm1``` and ```CommonTestsHelper.psm1``` respectively.
- Fixed examples:
  - Sample_InitializeDataDisk.ps1
  - Sample_InitializeDataDiskWithAccessPath.ps1
  - Sample_xMountImage_DismountISO.ps1

The changes made to xDiskAccessPath could be also ported to xDisk and it would resolve #25 . I'll raise a separate issue to address this.


This change is Reviewable

@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Nov 20, 2016
[CmdletBinding()]
param ()

return $PSVersionTable.PSEdition -ieq 'Core'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edition is not reliable way to detect Nano.

Please use this code.

function Test-IsNanoServer
{
    if(Test-Command -Name Get-ComputerInfo)
    {
        $computerInfo = Get-ComputerInfo

        if("Server" -eq $computerInfo.OsProductType -and "NanoServer" -eq $computerInfo.OsServerLevel)
        {
            return $true
        }
    }         

    return $false
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is really good to know! Thanks and fixed.

$timeout = 30000
$start = [DateTime]::Now
While ($partition.IsReadOnly `
-and ([DateTime]::Now - $start).TotalMilliseconds -lt $timeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: You are creating a time span each iteration of the loop. Calculate the timeout as an absolute time and check if now is still less than the calculated time.

Minor: you should use Get-Date instead of [DateTime]::Now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TravisEz13 - Good point! Change made. I've also changed the same loop code in xDisk.

Copy link
Contributor

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nano detection is the only blocking issue.

@TravisEz13 TravisEz13 added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Nov 29, 2016
[OutputType([Boolean])]
[CmdletBinding()]
param ()
if (Test-Command -Name Get-ComputerInfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-command isn't built in

@PlagueHO
Copy link
Member Author

PlagueHO commented Dec 2, 2016

Doh! Of course! Thanks @TravisEz13 - fixed.

@TravisEz13
Copy link
Contributor

build failed due to AppVeyor issue. Triggered rebuild.

@PlagueHO
Copy link
Member Author

PlagueHO commented Dec 2, 2016

Thanks @TravisEz13 - I didn't notice this.

@TravisEz13 TravisEz13 merged commit 19f7044 into dsccommunity:dev Dec 2, 2016
@TravisEz13 TravisEz13 removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Dec 2, 2016
@PlagueHO PlagueHO deleted the Fix-66/xDiskAccessPath-Fail-Remap branch December 2, 2016 22:19
@kwirkykat
Copy link
Contributor

@PlagueHO To answer your question about the PSSA issues, my preference would be to update to CIM cmdlets, but if the WMI cmdlets are really needed, then you can suppress the PSSA rule, and I will move this rule to the 'flagged' category and list this as an exception.

@PlagueHO
Copy link
Member Author

@kwirkykat - TBH, I think the WMI cmdlets are only there to support legacy systems. I think they were added in Windows Server 2012/Windows 8. So dropping the WMI cmdlets will prevent this resource working on Windows Server 2008R2 and Windows 7.

My feeling is that if people do really want to use this resource with these legacy OS's then they can use earlier versions of this module. Going forward I'd much rather drop the WMI cmdlets and just define a minimum requirement in the Readme.md . What do you reckon?

@kwirkykat
Copy link
Contributor

@PlagueHO I agree with this 100% with that last paragraph. Drop the WMI cmdlets and those who aren't upgrading to newer OS's can continue to use the older module versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants