Skip to content

Add Analyze step for script analyzer. #148

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Oct 9, 2016
Merged

Add Analyze step for script analyzer. #148

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0c7158c
Merge remote-tracking branch 'refs/remotes/PowerShell/master'
davegreen Oct 5, 2016
a0fbe4d
Merge remote-tracking branch 'refs/remotes/PowerShell/master'
davegreen Oct 5, 2016
0ae4cd4
Merge remote-tracking branch 'refs/remotes/PowerShell/master'
davegreen Oct 6, 2016
b2efc52
Merge remote-tracking branch 'refs/remotes/PowerShell/master'
davegreen Oct 8, 2016
2f2dab0
Resolve most scriptanalyzer warnings
davegreen Oct 8, 2016
b6ed918
Add Analyze task to run PSScriptAnalyzer
davegreen Oct 8, 2016
daa1cbe
Deal with further PSScriptAnalyzer warnings
davegreen Oct 8, 2016
58c4fff
Improve analyze task control
davegreen Oct 8, 2016
888ce86
Merge analyze task skip and control variables
davegreen Oct 8, 2016
37a7e14
Rename script analysis variables
davegreen Oct 8, 2016
2145282
Remove PSSA rule suppression from test files
davegreen Oct 8, 2016
9d95056
Reorder build tasks, fix incorrect required variable
davegreen Oct 8, 2016
743e289
Add PSScriptAnalyzer module to appveyor config
davegreen Oct 9, 2016
248bfe5
Remove extended character set symbols
davegreen Oct 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions build.psake.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,41 @@ Task Sign -depends BuildImpl -requiredVariables SettingsPath, SignScripts {
}
}

Task Analyze -depends Build -requiredVariables CodeAnalysisStop, OutDir {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should change the Test task to depend on the Analyze task instead of the Build task? IOW script analysis needs to pass first (assuming action set to error) before we are at a point where the Test task should run? And since, Publish depends on a successful Test that would mean that Publish would also require a successful Analyze task.

Another thought is that Analyze should be part of the Build task flow - perhaps between BuildImpl and Sign? You might want that if you set the Action to Error ie it is more a build failure so no point in signing files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, that's similar to what I've done in my own psake scripts before, so that's good to know. I'll do that.

if ((Get-Host).Name -in $SkipCodeAnalysisHost) {
$CodeAnalysisStop = 'Skip'
}

if ($CodeAnalysisStop -eq 'Skip') {
"Script analysis is not enabled. Skipping Analyze task."
return
}

$analysisResult = Invoke-ScriptAnalyzer -Path $OutDir -Recurse -Verbose:$VerbosePreference
$analysisResult | Format-Table
switch ($CodeAnalysisStop) {
'Error' {
Assert -conditionToCheck (
($analysisResult | Where-Object Severity -eq 'Error').Count -eq 0
) -failureMessage 'One or more Script Analyzer errors were found. Build cannot continue!'
}
'Warning' {
Assert -conditionToCheck (
($analysisResult | Where-Object {
$_.Severity -eq 'Warning' -or $_.Severity -eq 'Error'
}).Count -eq 0) -failureMessage 'One or more Script Analyzer warnings were found. Build cannot continue!'
}
'None' {
return
}
default {
Assert -conditionToCheck (
$analysisResult.Count -eq 0
) -failureMessage 'One or more Script Analyzer issues were found. Build cannot continue!'
}
}
}

Task GenerateMarkdown -depends Build, PreBuildHelp -requiredVariables DocsRootDir, ModuleName, OutDir {
if ($null -eq $DefaultLocale) {
$DefaultLocale = 'en-US'
Expand Down Expand Up @@ -375,6 +410,7 @@ function PromptUserForCredentialAndStorePassword {
}

function AddSetting {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function')]
param(
[Parameter(Mandatory)]
[string]$Key,
Expand Down
19 changes: 18 additions & 1 deletion build.settings.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,24 @@ Properties {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='CertSubjectName')]
$CertSubjectName = $null


# -------------------- Script Analysis properties ------------------------------

# The script analysis task step will run, unless your host is in the array defined below.
# This allows you to control whether code analysis is executed, for hosts where script
# analysis is included in the product.
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SkipCodeAnalysisHost')]
$SkipCodeAnalysisHost = @(
'Visual Studio Code Host',
'My Custom Host with scriptanalyzer support'
)

# To control the failure of the build with specific script analyzer rule severities,
# the CodeAnalysisStop variable can be used. The supported values for this variable are
# 'Warning', 'Error', 'All', 'None' or 'Skip'. Invalid input will stop on all rules.
# 'Skip' will skip over the code analysis step all together.
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='CodeAnalysisStop')]
$CodeAnalysisStop = 'Error'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename this to "ScriptAnalysisAction"? That would we similar to the VS Code Analysis ruleset where you can select the Action per rule. Ditto for SkipCodeAnalysisHost -> SkipScriptAnalysisHost. Just to keep it more in the "PowerShell vernacular".

Copy link
Collaborator Author

@davegreen davegreen Oct 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Of course! It's late for me so all my critical thinking goes out the window regarding variable names (that's a poor excuse, it's always like that).


# -------------------- Publishing properties ------------------------------

# Your NuGet API key for the PSGallery. Leave it as $null and the first time you publish,
Expand Down
10 changes: 7 additions & 3 deletions src/InvokePlaster.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<#
<#
Copy link
Collaborator

Choose a reason for hiding this comment

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

What editor are you using? If it is VSCode I suggest you add the user setting: "files.trimTrailingWhitespace": true. It is a very nice setting for PowerShell work and would eliminate this "apparent" trailing whitespace change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, thanks, good catch. Yeah, it is VSCode. I must have done this before on another machine because I could have sworn i'd done this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I did change the encoding of that file, as it was UTF-8, which generates the PSSA warning PSUseBOMForUnicodeEncodedFile. I can revert that if it causes an issue, but I couldn't find one in my testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What did you change the encoding to? UTF-8 with BOM? Weird that PSSA would think the file was Unicode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did, yes. I did a little investigation, it's the HELPERS art bit, the characters are non-ASCII code points.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah. Can you replace those characters with this:

###############################################################################
# Helper functions
###############################################################################

And adjust the encoding. Then I think we'll be ready to accept this PR. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, and if you want to do the merge, go for it. Just do it as a Squash and merge to clean up the commit history.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome, thanks for the help :)

NOTE TO DEVELOPERS:
All text displayed to the user except for Write-Debug (or $PSCmdlet.WriteDebug()) text must be added to the
string tables in:
Expand All @@ -24,10 +24,13 @@ Please follow the scripting style of this file when adding new script.
General notes
#>
function Invoke-Plaster {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingWriteHost', '', Scope='Function')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidShouldContinueWithoutForce', '', Scope='Function', Target='CopyFileWithConflictDetection')]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm glad this particular PSSA bug was recently fixed. Well, I haven't had a chance to test the fix (not sure it is even shipping yet) but I saw the PR go by that purports to fix it. :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which bug/fix? I did notice that the suppression rule for PSAvoidUsingWriteHost didn't actually manage to suppress all the usages, which was a bit odd. Is that what you meant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I wasn't very clear. This is the PR on PSSA I was referring to: PowerShell/PSScriptAnalyzer#625 I was running into this issue in Invoke-Plaster because I had several helper functions within begin that use $PSCmdlet.ShouldProcess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I am testing with script analyzer 1.8.0 and it appears to resolve that issue without requiring suppression anymore, so those PSShouldProcess suppression statements can go at some point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as we are using 1.8.0 in the AppVeyor build, we should be able to remove these. Let me take a crack at it.

[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function', Target='CopyFileWithConflictDetection')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function', Target='ProcessFile')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function', Target='ProcessModifyFile')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function', Target='ProcessNewModuleManifest')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingConvertToSecureStringWithPlainText', '', Scope='Function', Target='ProcessParameter')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function', Target='ProcessRequireModule')]
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidShouldContinueWithoutForce', '', Scope='Function', Target='ProcessFile')]
[CmdletBinding(SupportsShouldProcess=$true)]
Expand Down Expand Up @@ -709,7 +712,8 @@ function Invoke-Plaster {
# Copy over empty directories - if any.
$gciParams.Remove('File')
$gciParams['Directory'] = $true
$dirs = @(Microsoft.PowerShell.Management\Get-ChildItem @gciParams | Where {$_.GetFileSystemInfos().Length -eq 0})
$dirs = @(Microsoft.PowerShell.Management\Get-ChildItem @gciParams |
Where-Object {$_.GetFileSystemInfos().Length -eq 0})
foreach ($dir in $dirs) {
$dirSrcPath = $dir.FullName
$relPath = $dirSrcPath.Substring($srcRelRootPathLength)
Expand Down Expand Up @@ -1212,7 +1216,7 @@ function WriteContentWithEncoding([string]$path, [string[]]$content, [string]$en
'utf8' { $noBomEncoding = New-Object System.Text.UTF8Encoding($false) }
}

if ($content -eq $null) {
if ($null -eq $content) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. I try to be consistent with this but forget from time to time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries, it's generally a lot more consistent that what I normally manage :)

$content = [string]::Empty
}

Expand Down
36 changes: 36 additions & 0 deletions src/Templates/NewModule/build.psake.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,41 @@ Task Sign -depends BuildImpl -requiredVariables SettingsPath, SignScripts {
}
}

Task Analyze -depends Build -requiredVariables CodeAnalysisStop, OutDir {
if ((Get-Host).Name -in $SkipCodeAnalysisHost) {
$CodeAnalysisStop = 'Skip'
}

if ($CodeAnalysisStop -eq 'Skip') {
"Script analysis is not enabled. Skipping Analyze task."
return
}

$analysisResult = Invoke-ScriptAnalyzer -Path $OutDir -Recurse -Verbose:$VerbosePreference
$analysisResult | Format-Table
switch ($CodeAnalysisStop) {
'Error' {
Assert -conditionToCheck (
($analysisResult | Where-Object Severity -eq 'Error').Count -eq 0
) -failureMessage 'One or more Script Analyzer errors were found. Build cannot continue!'
}
'Warning' {
Assert -conditionToCheck (
($analysisResult | Where-Object {
$_.Severity -eq 'Warning' -or $_.Severity -eq 'Error'
}).Count -eq 0) -failureMessage 'One or more Script Analyzer warnings were found. Build cannot continue!'
}
'None' {
return
}
default {
Assert -conditionToCheck (
$analysisResult.Count -eq 0
) -failureMessage 'One or more Script Analyzer issues were found. Build cannot continue!'
}
}
}

Task GenerateMarkdown -depends Build, PreBuildHelp -requiredVariables DocsRootDir, ModuleName, OutDir {
if ($null -eq $DefaultLocale) {
$DefaultLocale = 'en-US'
Expand Down Expand Up @@ -375,6 +410,7 @@ function PromptUserForCredentialAndStorePassword {
}

function AddSetting {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function')]
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a matter of preference, I would put this outside the function. But that might just be the C# dev in me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did try that, putting it outside the function and at the top of the file, but it doesn't seem to like it at all. (Error is: Unexpected attribute 'System.Diagnostics.CodeAnalysis.SuppressMessage'.).

Copy link
Collaborator

@rkeithhill rkeithhill Oct 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

BTW is this cropping up because we are analyzing the build script? If so, perhaps we should only analyze scripts under the src dir? Sorry if this is a duplicate comment. I responded today by email (on my phone) while I was out and about with my wife. Those email responses don't seem to get added to the review. Oh well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind, this file is under src. Duh. Guess I need to just call it a night and get some sleep. :-)

param(
[Parameter(Mandatory)]
[string]$Key,
Expand Down
19 changes: 18 additions & 1 deletion src/Templates/NewModule/build.settings.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,24 @@ Properties {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='CertSubjectName')]
$CertSubjectName = $null


# -------------------- Script Analysis properties ------------------------------

# The script analysis task step will run, unless your host is in the array defined below.
# This allows you to control whether code analysis is executed, for hosts where script
# analysis is included in the product.
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SkipCodeAnalysisHost')]
$SkipCodeAnalysisHost = @(
'Visual Studio Code Host',
'My Custom Host with scriptanalyzer support'
)

# To control the failure of the build with specific script analyzer rule severities,
# the CodeAnalysisStop variable can be used. The supported values for this variable are
# 'Warning', 'Error', 'All', 'None' or 'Skip'. Invalid input will stop on all rules.
# 'Skip' will skip over the code analysis step all together.
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='CodeAnalysisStop')]
$CodeAnalysisStop = 'Error'

# -------------------- Publishing properties ------------------------------

# Your NuGet API key for the PSGallery. Leave it as $null and the first time you publish,
Expand Down
1 change: 1 addition & 0 deletions src/Templates/NewModule/test/Module.T.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SuppressImportModule')]
$SuppressImportModule = $true
. $PSScriptRoot\Shared.ps1

Expand Down
1 change: 1 addition & 0 deletions src/TestPlasterManifest.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#>
function Test-PlasterManifest {
[CmdletBinding()]
[OutputType([System.Xml.XmlDocument])]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice add. Thanks.

param(
# Specifies a path to a plasterManifest.xml file.
[Parameter(Position=0,
Expand Down
1 change: 1 addition & 0 deletions test/ModuleManifest.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SuppressImportModule')]
Copy link
Collaborator

@rkeithhill rkeithhill Oct 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

In general we exclude our C# unit tests from code analysis. Should we do the same here - exclude Pester tests from script analysis? I would be in favor of that. Getting your module's script copasetic with PSSA can be enough of a challenge. :-) Or was this to get rid of a VSCode editor warning message about the rule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems reasonable, It wasn't a VSCode warning, I just got a bit carried away suppressing warnings that came up.

How would we do that? Just ignore analysis results from *.Tests.ps1 files, or is there another approach.

$SuppressImportModule = $true
. $PSScriptRoot\Shared.ps1

Expand Down
5 changes: 5 additions & 0 deletions test/Shared.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
$ModuleManifestName = 'Plaster.psd1'
$ModuleManifestPath = "$PSScriptRoot\..\src\$ModuleManifestName"
$TemplateDir = "$PSScriptRoot\TemplateRootTemp"

[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='OutDir')]
$OutDir = "$PSScriptRoot\Out"

[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='PlasterManifestPath')]
$PlasterManifestPath = "$TemplateDir\plasterManifest.xml"

if (!$SuppressImportModule) {
Expand All @@ -11,6 +15,7 @@ if (!$SuppressImportModule) {
}

function CleanDir {
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSShouldProcess', '', Scope='Function')]
param(
[Parameter(Mandatory)]
[ValidateNotNullOrEmpty()]
Expand Down