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 all 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
1 change: 1 addition & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ install:
Install-PackageProvider -Name NuGet -MinimumVersion 2.8.5.201 -Force | Out-Null
Install-Module Pester -MinimumVersion 3.4.0 -Scope CurrentUser -Force | Out-Null
Install-Module psake -Scope CurrentUser -Force | Out-Null
Install-Module PSScriptAnalyzer -Scope CurrentUser -Force | Out-Null

build_script:
- ps: |
Expand Down
40 changes: 38 additions & 2 deletions build.psake.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Task Clean -requiredVariables ReleaseDir {
}
}

Task Build -depends BuildImpl, Sign, PostBuild {
Task Build -depends BuildImpl, Analyze, Sign, PostBuild {
}

Task BuildImpl -depends Init, Clean, PreBuild -requiredVariables SrcRootDir, OutDir {
Expand Down Expand Up @@ -134,6 +134,41 @@ Task Sign -depends BuildImpl -requiredVariables SettingsPath, SignScripts {
}
}

Task Analyze -depends BuildImpl -requiredVariables ScriptAnalysisAction, OutDir {
if ((Get-Host).Name -in $SkipScriptAnalysisHost) {
$ScriptAnalysisAction = 'Skip'
}

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

$analysisResult = Invoke-ScriptAnalyzer -Path $OutDir -Recurse -Verbose:$VerbosePreference
$analysisResult | Format-Table
switch ($ScriptAnalysisAction) {
'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 @@ -194,7 +229,7 @@ Task InstallImpl -depends BuildHelp, PreInstall -requiredVariables OutDir {
Copy-Item -Path $OutDir\* -Destination $InstallPath -Verbose:$VerbosePreference -Recurse -Force
}

Task Test -depends Build -requiredVariables TestRootDir, ModuleName {
Task Test -depends Analyze -requiredVariables TestRootDir, ModuleName {
Import-Module Pester

try {
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
17 changes: 17 additions & 0 deletions build.settings.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,23 @@ 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')]
$SkipScriptAnalysisHost = @(
'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')]
$ScriptAnalysisAction = 'Error'

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

Expand Down
18 changes: 9 additions & 9 deletions src/InvokePlaster.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@ -1112,13 +1116,9 @@ function Invoke-Plaster {
}
}

<#
██ ██ ███████ ██ ██████ ███████ ██████ ███████
██ ██ ██ ██ ██ ██ ██ ██ ██ ██
███████ █████ ██ ██████ █████ ██████ ███████
██ ██ ██ ██ ██ ██ ██ ██ ██
██ ██ ███████ ███████ ██ ███████ ██ ██ ███████
#>
###############################################################################
# Helper functions
###############################################################################

function InitializePredefinedVariables([string]$TemplatePath, [string]$DestPath) {
# Always set these variables, even if the command has been run with -WhatIf
Expand Down Expand Up @@ -1212,7 +1212,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
40 changes: 38 additions & 2 deletions src/Templates/NewModule/build.psake.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Task Clean -requiredVariables ReleaseDir {
}
}

Task Build -depends BuildImpl, Sign, PostBuild {
Task Build -depends BuildImpl, Analyze, Sign, PostBuild {
}

Task BuildImpl -depends Init, Clean, PreBuild -requiredVariables SrcRootDir, OutDir {
Expand Down Expand Up @@ -134,6 +134,41 @@ Task Sign -depends BuildImpl -requiredVariables SettingsPath, SignScripts {
}
}

Task Analyze -depends BuildImpl -requiredVariables ScriptAnalysisAction, OutDir {
if ((Get-Host).Name -in $SkipScriptAnalysisHost) {
$ScriptAnalysisAction = 'Skip'
}

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

$analysisResult = Invoke-ScriptAnalyzer -Path $OutDir -Recurse -Verbose:$VerbosePreference
$analysisResult | Format-Table
switch ($ScriptAnalysisAction) {
'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 @@ -194,7 +229,7 @@ Task InstallImpl -depends BuildHelp, PreInstall -requiredVariables OutDir {
Copy-Item -Path $OutDir\* -Destination $InstallPath -Verbose:$VerbosePreference -Recurse -Force
}

Task Test -depends Build -requiredVariables TestRootDir, ModuleName {
Task Test -depends Analyze -requiredVariables TestRootDir, ModuleName {
Import-Module Pester

try {
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
17 changes: 17 additions & 0 deletions src/Templates/NewModule/build.settings.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,23 @@ 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')]
$SkipScriptAnalysisHost = @(
'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')]
$ScriptAnalysisAction = 'Error'

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

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