Checkstyle Standards for Powershell Modules

[jborean93]

Proposal: Checkstyle Standards for Powershell Modules

Author: Jordan Borean @jborean93

Date: 2017/05/21

• Status: New
• Proposal type: core design
• Targeted Release: 2.4
• Associated PR: TBC
• Estimated time to implement: 4 weeks

Motivation

Trying to keep the Powershell modules consistent with each other and define
a standard that is enforcable without human intervention.

Problems

Some of the current problems we have today are;

• There is a mixture of ways Powershell modules are written which causes confusion and fragmentation
• The Python modules use PEP for standards but no automated checked are in place for Powershell
• There is also no official standard for how we write these modules, this proposal will both create that standard and enforce it

Solution proposal

The proposed solution is to use 2 Powershell modules called PSScriptAnalyzer
and Pester to run a check style analysis
as well as generate reports on this analysis in a CI like fashion. These tools
should run on the module affected on every pull request.

PSScriptAnalyzer is a tool that can run checkstyle analysis on powershell
scripts based on a custom ruleset. It has the ability to specify rules that you
wish to scan for as well as writting custom rules if the in built ones don't
match your requirements.

Pester is a framework for running tests that can easily integrate in a CI
system. While it has a lot of features we can use in the future like unit
testing for powershell modules and code coverage reporting this proposal is
limited to using Pester to run PSScriptAnalyzer and generate an NUnit XML
report at the end of the process. There are numerous tools out there to convert
an NUnit xml to HTML for reporting purposes.

While these modules need to run in Powershell with the ability to install
Powershell 5 on both Windows and Unix we have various options available to us
when running these tests.

The following rule settings for PSScriptAnalyzer is as follows

@{
    IncludeRules = @(
        'PSAvoidUsingCmdletAliases',
        'PSAvoidUsingEmptyCatchBlock',
        'PSAvoidUsingPositionalParameters',
        'PSAvoidUsingWMICmdlet',
        'PSAvoidUsingWriteHost',
        'PSMisleadingBacktick',
        'PSPlaceCloseBrace',
        'PSPlaceOpenBrace',
        'PSReservedCmdletChar',
        'PSReservedParams',
        'PSUseConsistentIndentation',
        'PSUseConsistentWhitespace'
    )

    Rules = @{
        PSAvoidUsingCmdletAliases = @{
            Whitelist = @('')
        }
        PSPlaceCloseBrace = @{
            Enable = $true
            NoEmptyLineBefore = $true
            IgnoreOneLineBlock = $false
            NewLineAfter = $false
        }
        PSPlaceOpenBrace = @{
            Enable = $true
            OnSameLine = $true
            IgnoreOneLineBlock = $false
            NewLineAfter = $true
        }
        PSUseConsistentIndentation = @{
            Enable = $true
            IndentationSize = 4
        }
        PSUseConsistentWhitespace = @{
            Enable = $true
            CheckOpenBrace = $true
            CheckOpenParen = $true
            CheckOperator = $true
            CheckSeparator = $true
        }
    }
}

The documentation for these rules can be found here. These rules are
open to debug they are just what I believe we should be aiming for but happy to
add/remove/modify rules based on a consensus with the community.

The second part of this proposal is to go through our existing modules and
ensure they conform to this new standard. Because of the different coding
styles in place it is hard to.

As well as implementing these checks into the existing testing process we will
also need to modify the existing modules to ensure they pass the new checks
added.

An example of the output of running this check on an existing module like
win_acl

PS C:\Users\jbore> powershell.exe -File C:\dev\ansible\test\runner\powershell-pester.ps1

Describing Testing against PSSA rules
   Context PSSA Ansible Rules

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSAvoidUsingCmdletAliases           Warning      win_acl.ps 67    'where' is an alias of 'Where-Object'. Alias can
                                                 1                introduce possible problems and make scripts hard to
                                                                  maintain. Please consider changing alias to its full
                                                                  content.


    [-] Should pass PSAvoidUsingCmdletAliases 1.03s
      Expected: {0}
      But was:  {1}
      13:                     $failures.Count | Should Be 0
      at <ScriptBlock>, C:\dev\ansible\test\runner\pester-tests.ps1: line 13
    [+] Should pass PSAvoidUsingEmptyCatchBlock 127ms
    [+] Should pass PSAvoidUsingPositionalParameters 19ms

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSAvoidUsingWMICmdlet               Warning      win_acl.ps 67    File 'win_acl.ps1' uses WMI cmdlet. For PowerShell 3.0 and
                                                 1                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.


    [-] Should pass PSAvoidUsingWMICmdlet 30ms
      Expected: {0}
      But was:  {1}
      13:                     $failures.Count | Should Be 0
      at <ScriptBlock>, C:\dev\ansible\test\runner\pester-tests.ps1: line 13
    [+] Should pass PSAvoidUsingWriteHost 25ms
    [+] Should pass PSMisleadingBacktick 20ms

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSPlaceCloseBrace                   Warning      win_acl.ps 41    Close brace does not follow a non-empty line.
                                                 1


    [-] Should pass PSPlaceCloseBrace 28ms
      Expected: {0}
      But was:  {1}
      13:                     $failures.Count | Should Be 0
      at <ScriptBlock>, C:\dev\ansible\test\runner\pester-tests.ps1: line 13

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSPlaceOpenBrace                    Warning      win_acl.ps 29    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 37    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 39    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 43    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 48    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 54    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 59    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 65    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 69    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 74    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 78    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 82    Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 100   Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.
PSPlaceOpenBrace                    Warning      win_acl.ps 238   Open brace not on same line as preceding keyword. It should
                                                 1                be on the same line.


    [-] Should pass PSPlaceOpenBrace 75ms
      Expected: {0}
      But was:  {14}
      13:                     $failures.Count | Should Be 0
      at <ScriptBlock>, C:\dev\ansible\test\runner\pester-tests.ps1: line 13
    [+] Should pass PSReservedCmdletChar 28ms
    [+] Should pass PSReservedParams 20ms
    [+] Should pass PSUseConsistentIndentation 23ms

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSUseConsistentWhitespace           Warning      win_acl.ps 29    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 37    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 39    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 43    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 48    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 54    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 59    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 65    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 69    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 74    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 78    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 82    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 87    Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 100   Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 238   Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 282   Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 297   Use space before open brace.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 282   Use space before open parenthesis.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 297   Use space before open parenthesis.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 198   Use space before and after binary and assignment operators.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 199   Use space before and after binary and assignment operators.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 200   Use space before and after binary and assignment operators.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 264   Use space before and after binary and assignment operators.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 267   Use space before and after binary and assignment operators.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 107   Use space after a comma.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 225   Use space after a comma.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 226   Use space after a comma.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 229   Use space after a comma.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 229   Use space after a comma.
                                                 1
PSUseConsistentWhitespace           Warning      win_acl.ps 295   Use space after a comma.
                                                 1


    [-] Should pass PSUseConsistentWhitespace 142ms
      Expected: {0}
      But was:  {30}
      13:                     $failures.Count | Should Be 0
      at <ScriptBlock>, C:\dev\ansible\test\runner\pester-tests.ps1: line 13
Tests completed in 1.58s
Passed: 7, Failed: 5, Skipped: 0, Pending: 0, Inconclusive: 0

PS C:\Users\jbore> $LASTEXITCODE
5

There are some very basic scripts I created for the output above. You can see
them here.

Dependencies

• Powershell 5.0
• PSScriptAnalyzer
• Pester

Testing

These tests only need to run on the module that is affected in the PR, there is
no reason that we need to run it on all modules but that is possible if we wish
to go down that path.

Documentation

New docs page created that will show the rules and examples that we are
enforcing as well as some guidelines on how to run these tests locally
before raising a PR.

Anything else?

While this is not part of the scope for this proposal I believe using a library
like Pester brings us a step forward to implementing unit tests on our
powershell modules and bringing more in line with our processes around Python
modules.

This is my first proposal so let me know if I have missed anything or whether I
have done something wrong.

[dagwieers]

Looking good. My only concern is that I do all my development on Linux, so I won't be able to test conformity locally and it will require a few more (amend) commits before getting it right. Which sometimes is a big time-consumer.

[jborean93]

You should be able to run these modules on a linux box if you install powershell. I haven't tested it myself but the docs indicate it is supported.

[dagwieers]

@nitzmahone @jborean93 @jhawkesworth Up to us to play with it already so we can have balanced discussion and understand the possible trade-offs.

The way I see it, during the meeting we were on the same page, so we only have to check off on the proposed rules, but that shouldn't be a concern for already looking to integrate this mechanism in Shippable.

@mattclay It would be nice to have this integrated as soon as possible (say, by AnsibleFest London ? :-P).

[gundalow]

Matt is on holiday and will only get back the Monday before London.
I'd like to discuss this with him before it gets implemented.

So to set expectation:

• This sounds good
• Check back in a month

[aaronk1]

Is this still being considered?

[dagwieers]

@aaronk1 This is already implemented, but we are not enforcing all rules we considered originally AFAIK. I think we had a set of coding style-related requirements that are not being enforced. Maybe we should have that discussion next Windows WG meeting ?

Update: There is a list of modules that fail specific tests which we currently ignore, it would be nice if we could go over that list and trim it down with fixes. https://github.com/ansible/ansible/blob/devel/test/sanity/pslint/ignore.txt

[dagwieers]

@jborean93 While fixing my Windows modules to validate with pslint, I came across this (ansible/ansible#37718, ansible/ansible#37720, ansible/ansible#37721):

PSPossibleIncorrectComparisonWithNull $null should be on the left side of equality comparisons.

And I don't agree based on readability. But maybe there's a good reason to do this in Powershell ?

Relevant information:

• PSPossibleIncorrectComparisonWithNull rule is questionable PowerShell/PSScriptAnalyzer#200

[mattclay]

@dagwieers I agree it's definitely not as readable. However, it's intended to avoid incorrect comparisons against $null. The example in the issue you linked demonstrates the problem:

https://github.com/nightroman/PowerShellTraps/blob/c1b35f876a0ce737ed31cf6c1e2747b073521733/Basic/Comparison-operators-with-collections/looks-like-object-is-null.ps1#L2-L8

[dagwieers]

@mattclay My problem here is that we know that $value is a string, we already enforce it when assigning parameter values. Pylint is simply not smart enough, still we are going to make things less readable where there's really no issue. So to me these are false positives.

[mattclay]

@dagwieers That rule shouldn't have false positives unless the type is unspecified:

PowerShell/PSScriptAnalyzer#200 (comment)

The rule is now raised only if the object on the LHS has an unspecified type or is an array.

Is this a bug in PSScriptAnalzyer, or is the LHS an unspecified type?

[dagwieers]

@mattclay I don't know if we can force the type from within a function and if it gets inherited on assignment, if not we may have to force the type on each assignment. Let's see what @jborean93 @nitzmahone think about this.

Update: So the above test only works well for declared-typed vars, in our modules they tend to give more "false positives" that we cannot easily prevent. For that reason we decided to disable it in ansible/ansible#37739

[dagwieers]

@jborean93 Do we also want to enforce some of the coding style-related tests, as we originally contemplated ?

[jborean93]

Closing as this has been implemented ages ago, just run ansible-test sanity --test pslint --docker default to run the checks locally.