Add Analyze step for script analyzer. #148

davegreen · 2016-10-08T22:08:02Z

This pull request adds the analyze task for running script analyzer on the built project, as well as fixing or suppressing some additional script analyzer warnings/errors. Mentioned in #145.

There are some variables added to build.settings.ps1 to control the when the task will stop the build. I added a variable to also control which PowerShell hosts to automatically exempt from this task. I'm unsure if this is the right approach to take though.

I also haven't added this as a dependency to any tasks yet, I think this would probably fit as a dependency to the publish task, but i'm open to suggestions.

rkeithhill · 2016-10-08T22:09:19Z

src/TestPlasterManifest.ps1

@@ -26,6 +26,7 @@
 #>
 function Test-PlasterManifest {
    [CmdletBinding()]
+    [OutputType([System.Xml.XmlDocument])]


Nice add. Thanks.

rkeithhill · 2016-10-08T22:16:29Z

build.settings.ps1

+    # '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'


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".

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).

rkeithhill · 2016-10-08T22:29:29Z

build.psake.ps1

@@ -134,6 +134,41 @@ Task Sign -depends BuildImpl -requiredVariables SettingsPath, SignScripts {
    }
 }

+Task Analyze -depends Build -requiredVariables CodeAnalysisStop, OutDir {


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.

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.

rkeithhill · 2016-10-08T22:33:48Z

src/InvokePlaster.ps1

@@ -1,4 +1,4 @@
-<#
+<#


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.

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.

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.

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

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

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!

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.

Awesome, thanks for the help :)

rkeithhill · 2016-10-08T22:36:10Z

src/InvokePlaster.ps1

@@ -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')]


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. :-)

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?

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.

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.

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.

rkeithhill · 2016-10-08T22:36:50Z

src/InvokePlaster.ps1

@@ -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) {


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

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

rkeithhill · 2016-10-08T22:38:29Z

test/ModuleManifest.Tests.ps1

@@ -1,3 +1,4 @@
+[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SuppressImportModule')]


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?

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.

rkeithhill · 2016-10-08T22:41:18Z

src/Templates/NewModule/build.psake.ps1

@@ -375,6 +410,7 @@ function PromptUserForCredentialAndStorePassword {
 }

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


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

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'.).

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.

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

rkeithhill

Check the issue on encoding. Otherwise looks good to go.

rkeithhill · 2016-10-09T05:10:22Z

src/InvokePlaster.ps1

@@ -1,4 +1,4 @@
-<#
+<#


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

rkeithhill · 2016-10-09T05:13:40Z

src/Templates/NewModule/build.psake.ps1

@@ -375,6 +410,7 @@ function PromptUserForCredentialAndStorePassword {
 }

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


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.

rkeithhill · 2016-10-09T05:15:45Z

src/InvokePlaster.ps1

@@ -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')]


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.

rkeithhill · 2016-10-09T05:39:42Z

@daviwil Can we get the PSSA module installed on the AppVeyor build machine?

daviwil · 2016-10-10T17:30:22Z

You guys were kickin' ass this weekend, some nice work here :)

davegreen added 9 commits October 5, 2016 21:50

Merge remote-tracking branch 'refs/remotes/PowerShell/master'

0c7158c

Merge remote-tracking branch 'refs/remotes/PowerShell/master'

a0fbe4d

Merge remote-tracking branch 'refs/remotes/PowerShell/master'

0ae4cd4

Merge remote-tracking branch 'refs/remotes/PowerShell/master'

b2efc52

Resolve most scriptanalyzer warnings

2f2dab0

Add Analyze task to run PSScriptAnalyzer

b6ed918

Deal with further PSScriptAnalyzer warnings

daa1cbe

Improve analyze task control

58c4fff

Merge analyze task skip and control variables

888ce86

msftclas added the cla-already-signed label Oct 8, 2016

rkeithhill approved these changes Oct 8, 2016

View reviewed changes

davegreen added 3 commits October 9, 2016 00:18

Rename script analysis variables

37a7e14

Remove PSSA rule suppression from test files

2145282

Reorder build tasks, fix incorrect required variable

9d95056

rkeithhill approved these changes Oct 9, 2016

View reviewed changes

davegreen added 2 commits October 9, 2016 11:30

Add PSScriptAnalyzer module to appveyor config

743e289

Remove extended character set symbols

248bfe5

davegreen merged commit 23b8c1b into PowerShellOrg:master Oct 9, 2016

davegreen deleted the scriptanalyzer branch October 9, 2016 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Analyze step for script analyzer. #148

Add Analyze step for script analyzer. #148

davegreen commented Oct 8, 2016 •

edited

Loading

rkeithhill Oct 8, 2016

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016 •

edited

Loading

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016

davegreen Oct 8, 2016

rkeithhill Oct 9, 2016

davegreen Oct 9, 2016

rkeithhill Oct 9, 2016

rkeithhill Oct 9, 2016

davegreen Oct 9, 2016

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016

rkeithhill Oct 9, 2016

davegreen Oct 9, 2016

rkeithhill Oct 9, 2016

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016

rkeithhill Oct 8, 2016 •

edited

Loading

davegreen Oct 8, 2016

rkeithhill Oct 8, 2016

davegreen Oct 8, 2016

rkeithhill Oct 9, 2016 •

edited

Loading

rkeithhill Oct 9, 2016

rkeithhill left a comment

rkeithhill Oct 9, 2016

rkeithhill Oct 9, 2016 •

edited

Loading

rkeithhill Oct 9, 2016

rkeithhill commented Oct 9, 2016

daviwil commented Oct 10, 2016

		@@ -1,3 +1,4 @@
		[System.Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssigments', '', Scope='*', Target='SuppressImportModule')]

Add Analyze step for script analyzer. #148

Add Analyze step for script analyzer. #148

Conversation

davegreen commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davegreen Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Oct 9, 2016

daviwil commented Oct 10, 2016

davegreen commented Oct 8, 2016 •

edited

Loading

davegreen Oct 8, 2016 •

edited

Loading

rkeithhill Oct 8, 2016 •

edited

Loading

rkeithhill Oct 9, 2016 •

edited

Loading

rkeithhill Oct 9, 2016 •

edited

Loading