Closed jborean93 closed 5 years ago
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.
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.
@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).
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:
Is this still being considered?
@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
@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:
@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:
@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.
@dagwieers That rule shouldn't have false positives unless the type is unspecified:
https://github.com/PowerShell/PSScriptAnalyzer/issues/200#issuecomment-114946701
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?
@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
@jborean93 Do we also want to enforce some of the coding style-related tests, as we originally contemplated ?
Closing as this has been implemented ages ago, just run ansible-test sanity --test pslint --docker default
to run the checks locally.
Proposal: Checkstyle Standards for Powershell Modules
Author: Jordan Borean @jborean93
Date: 2017/05/21
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;
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
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
There are some very basic scripts I created for the output above. You can see them here.
Dependencies
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.