Skip to content

Conversation

mhendric
Copy link
Contributor

@mhendric mhendric commented Oct 29, 2018

Pull Request (PR) description

Fixes most PSScriptAnalyzer issues for the module. Note that I'm unsure why the 3 .psm1 files are showing in GitHub that the whole file was changed. I suspect that this has to do with a recent change to .gitattributes, but am not positive. Looks like they show up correctly in Reviewable though.

PSScriptAnalyzer Before:

Count Name
----- ----
   18 PSPossibleIncorrectComparisonWithNull
    8 PSDSCUseVerboseMessageInDSCResource
    6 PSUseDeclaredVarsMoreThanAssignments
    4 PSAvoidTrailingWhitespace
    3 PSUseToExportFieldsInManifest
    3 PSDSCDscExamplesPresent
    3 PSUseShouldProcessForStateChangingFunctions
    2 PSAvoidUsingWriteHost
    2 PSAvoidGlobalVars
    2 PSDSCDscTestsPresent
    2 PSAvoidUsingCmdletAliases

PSScriptAnalyzer After:

Count Name
----- ----
    3 PSUseShouldProcessForStateChangingFunctions
    3 PSDSCDscExamplesPresent
    3 PSUseToExportFieldsInManifest
    2 PSDSCDscTestsPresent

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, 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 Reviewable

@codecov-io
Copy link

codecov-io commented Oct 29, 2018

Codecov Report

Merging #27 into dev will not change coverage.
The diff coverage is 26.08%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev      #27   +/-   ##
=======================================
  Coverage   26.08%   26.08%           
=======================================
  Files           3        3           
  Lines          92       92           
=======================================
  Hits           24       24           
  Misses         68       68
Impacted Files Coverage Δ
DSCResources/MSFT_xBLTpm/MSFT_xBLTpm.psm1 3.7% <3.7%> (ø) ⬆️
...s/MSFT_xBLAutoBitlocker/MSFT_xBLAutoBitlocker.psm1 42.3% <42.3%> (ø) ⬆️
...Resources/MSFT_xBLBitlocker/MSFT_xBLBitlocker.psm1 7.69% <7.69%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5d39bc...eee82fe. Read the comment docs.

@mhendric
Copy link
Contributor Author

FYI, I'm done pushing changes on this one. This should be ready for review when someone gets to it. Thanks!

Copy link

@athaynes athaynes left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @athaynes and @mhendric)


README.md, line 115 at r1 (raw file):

*   *Identity:Not actually used, so could be anything
*   AllowClear:Indicates that the provisioning process clears the TPM, if necessary, to move the TPM closer to complying with Windows Server? 2012 standards

why the question mark?


DSCResources/MSFT_xBLAutoBitlocker/MSFT_xBLAutoBitlocker.psm1, line 368 at r1 (raw file):

    #First get all Bitlocker Volumes of type Data
    $allBlvs = Get-BitLockerVolume | Where-Object -FilterScript {$_.VolumeType -eq "Data"}

Not really an issue, but if you don't have a variable, you don't need double quotes.


Test/Test-xBitlocker.ps1, line 104 at r1 (raw file):

function RunTest
{
    param([string]$TestName, [string[]]$ModulesToImport, [Hashtable]$Parameters)

This function does not conform to style guides. This is my first time to look at this module, but do the standard test templates not work for this module?

Copy link
Contributor Author

@mhendric mhendric left a comment

Choose a reason for hiding this comment

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

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


README.md, line 115 at r1 (raw file):

Previously, athaynes (Adam Haynes) wrote…

why the question mark?

This was originally a 'Registered Trademark' symbol, but running the metafixer ConvertTo-ASCII seems to have changed it to a ?. It's probably best to have neither. I will drop it entirely.


DSCResources/MSFT_xBLAutoBitlocker/MSFT_xBLAutoBitlocker.psm1, line 368 at r1 (raw file):

Previously, athaynes (Adam Haynes) wrote…

Not really an issue, but if you don't have a variable, you don't need double quotes.

This line wasn't actually changed for this PR, but I agree, Double Quotes aren't necessary here, and go against the DSC style guidelines. At some point this will need to be fixed as part of an effort to make this a High Quality Resource Module, but I will leave that for another PR. I'll open an Issue to track this.


Test/Test-xBitlocker.ps1, line 104 at r1 (raw file):

Previously, athaynes (Adam Haynes) wrote…

This function does not conform to style guides. This is my first time to look at this module, but do the standard test templates not work for this module?

This is the original integration test file which was created well before this module went open source. The file should probably be removed entirely once similar Integration or Unit tests have been added. I'll open an Issue to track getting everything in this file moved to appropriate templates.

Copy link
Contributor Author

@mhendric mhendric left a comment

Choose a reason for hiding this comment

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

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


README.md, line 115 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

This was originally a 'Registered Trademark' symbol, but running the metafixer ConvertTo-ASCII seems to have changed it to a ?. It's probably best to have neither. I will drop it entirely.

Done.

Copy link

@athaynes athaynes left a comment

Choose a reason for hiding this comment

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

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

@mhendric mhendric merged commit 80eb3b2 into dsccommunity:dev Nov 8, 2018
@mhendric mhendric deleted the FixScriptAnalyzerIssues2 branch November 20, 2018 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants