From 4dc2ce46dec57bfeb4900d18f1b895e1c1d38cfd Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Tue, 9 Jun 2015 15:48:01 -0700 Subject: [PATCH 1/6] Rule to detect the absence of Verbose statements in DSC Resource --- Rules/ScriptAnalyzerBuiltinRules.csproj | 2 +- Rules/Strings.Designer.cs | 83 ++++++++--------- Rules/Strings.resx | 19 ++-- Rules/UseDeclaredVarsMoreThanAssignments.cs | 3 +- ...ge.cs => UseVerboseMessageInDSCResouce.cs} | 92 ++++++++----------- 5 files changed, 87 insertions(+), 112 deletions(-) rename Rules/{ProvideVerboseMessage.cs => UseVerboseMessageInDSCResouce.cs} (54%) diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index e9b8a56f6..040413abf 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -75,7 +75,7 @@ - + True True diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 54175ef88..8c76e720b 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ // // This code was generated by a tool. -// Runtime Version:4.0.30319.35317 +// Runtime Version:4.0.30319.42000 // // Changes to this file may cause incorrect behavior and will be lost if // the code is regenerated. @@ -1167,51 +1167,6 @@ internal static string ProvideDefaultParameterValueName { } } - /// - /// Looks up a localized string similar to Verbose. - /// - internal static string ProvideVerboseMessageCommonName { - get { - return ResourceManager.GetString("ProvideVerboseMessageCommonName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Checks that Write-Verbose is called at least once in every cmdlet or script. This is in line with the PowerShell best practices.. - /// - internal static string ProvideVerboseMessageDescription { - get { - return ResourceManager.GetString("ProvideVerboseMessageDescription", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to There is no call to Write-Verbose in the function ‘{0}’.. - /// - internal static string ProvideVerboseMessageErrorFunction { - get { - return ResourceManager.GetString("ProvideVerboseMessageErrorFunction", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to ProvideVerboseMessage. - /// - internal static string ProvideVerboseMessageName { - get { - return ResourceManager.GetString("ProvideVerboseMessageName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to There is no call to Write-Verbose in the script.. - /// - internal static string ProvideVerboseMessageScript { - get { - return ResourceManager.GetString("ProvideVerboseMessageScript", resourceCulture); - } - } - /// /// Looks up a localized string similar to Reserved Cmdlet Chars. /// @@ -1913,5 +1868,41 @@ internal static string UseTypeAtVariableAssignmentName { return ResourceManager.GetString("UseTypeAtVariableAssignmentName", resourceCulture); } } + + /// + /// Looks up a localized string similar to Use verbose message in DSC resource. + /// + internal static string UseVerboseMessageInDSCResourceCommonName { + get { + return ResourceManager.GetString("UseVerboseMessageInDSCResourceCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to It is a best practice to emit informative, verbose messages in DSC resource functions. This helps in debugging issues when a DSC configuration is executed.. + /// + internal static string UseVerboseMessageInDSCResourceDescription { + get { + return ResourceManager.GetString("UseVerboseMessageInDSCResourceDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to There is no call to Write-Verbose in DSC function ‘{0}’. If you are using Write-Verbose in a helper function, suppress this rule application.. + /// + internal static string UseVerboseMessageInDSCResourceErrorFunction { + get { + return ResourceManager.GetString("UseVerboseMessageInDSCResourceErrorFunction", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to UseVerboseMessageInDSCResource. + /// + internal static string UseVerboseMessageInDSCResourceName { + get { + return ResourceManager.GetString("UseVerboseMessageInDSCResourceName", resourceCulture); + } + } } } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index c16fa35e3..dd02abedf 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -276,17 +276,14 @@ PS - - Checks that Write-Verbose is called at least once in every cmdlet or script. This is in line with the PowerShell best practices. + + It is a best practice to emit informative, verbose messages in DSC resource functions. This helps in debugging issues when a DSC configuration is executed. - - There is no call to Write-Verbose in the function ‘{0}’. + + There is no call to Write-Verbose in DSC function ‘{0}’. If you are using Write-Verbose in a helper function, suppress this rule application. - - Verbose - - - There is no call to Write-Verbose in the script. + + Use verbose message in DSC resource Some fields of the module manifest (such as ModuleVersion) are required. @@ -459,8 +456,8 @@ MissingModuleManifestField - - ProvideVerboseMessage + + UseVerboseMessageInDSCResource Command Not Found diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 1f1ecf089..f90302500 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -35,8 +35,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true); - IEnumerable assingmentVarAsts; + IEnumerable assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true); IEnumerable varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true); IEnumerable varsInAssignment; diff --git a/Rules/ProvideVerboseMessage.cs b/Rules/UseVerboseMessageInDSCResouce.cs similarity index 54% rename from Rules/ProvideVerboseMessage.cs rename to Rules/UseVerboseMessageInDSCResouce.cs index b3d191b56..470f59b30 100644 --- a/Rules/ProvideVerboseMessage.cs +++ b/Rules/UseVerboseMessageInDSCResouce.cs @@ -22,70 +22,58 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// ProvideVerboseMessage: Analyzes the ast to check that Write-Verbose is called at least once in every cmdlet or script. + /// ProvideVerboseMessage: Analyzes the ast to check that Write-Verbose is called for DSC Resources /// - [Export(typeof(IScriptRule))] - public class ProvideVerboseMessage : SkipNamedBlock, IScriptRule + [Export(typeof(IDSCResourceRule))] + public class UseVerboseMessageInDSCResouce : SkipNamedBlock, IDSCResourceRule { /// - /// AnalyzeScript: Analyzes the ast to check that Write-Verbose is called at least once in every cmdlet or script. + /// AnalyzeDSCResource: Analyzes the ast to check that Write-Verbose is called for DSC Resources /// The script's ast /// The script's file name /// - public IEnumerable AnalyzeScript(Ast ast, string fileName) + public IEnumerable AnalyzeDSCResource(Ast ast, string fileName) { - if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - - ClearList(); - this.AddNames(new List() { "Configuration", "Workflow" }); - DiagnosticRecords.Clear(); - - this.fileName = fileName; - //We only check that advanced functions should have Write-Verbose - ast.Visit(this); - - return DiagnosticRecords; - } - - /// - /// Visit function and checks that it has write verbose - /// - /// - /// - public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst funcAst) - { - if (funcAst == null) + if (ast == null) { - return AstVisitAction.SkipChildren; + throw new ArgumentNullException(Strings.NullAstErrorMessage); } - //Write-Verbose is not required for non-advanced functions - if (funcAst.Body == null || funcAst.Body.ParamBlock == null - || funcAst.Body.ParamBlock.Attributes == null || - funcAst.Body.ParamBlock.Parameters == null || - !funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute))) - { - return AstVisitAction.Continue; - } + List expectedTargetResourceFunctionNames = new List(new string[] { "Set-TargetResource", "Test-TargetResource", "Get-TargetResource" }); - var commandAsts = funcAst.Body.FindAll(testAst => testAst is CommandAst, false); - bool hasVerbose = false; + IEnumerable functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast); - if (commandAsts != null) + foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts) { - foreach (CommandAst commandAst in commandAsts) + var commandAsts = functionDefinitionAst.Body.FindAll(testAst => testAst is CommandAst, false); + bool hasVerbose = false; + + if (null != commandAsts) { - hasVerbose |= String.Equals(commandAst.GetCommandName(), "Write-Verbose", StringComparison.OrdinalIgnoreCase); + foreach (CommandAst commandAst in commandAsts) + { + hasVerbose |= String.Equals(commandAst.GetCommandName(), "Write-Verbose", StringComparison.OrdinalIgnoreCase); + } } - } - if (!hasVerbose) - { - DiagnosticRecords.Add(new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageErrorFunction, funcAst.Name), - funcAst.Extent, GetName(), DiagnosticSeverity.Information, fileName)); - } + if (!hasVerbose) + { + yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceErrorFunction, functionDefinitionAst.Name), + functionDefinitionAst.Extent, GetName(), DiagnosticSeverity.Information, fileName); + } - return AstVisitAction.Continue; + } + } + + /// + /// AnalyzeDSCClass: This function returns nothing in the case of dsc class. + /// + /// + /// + /// + public IEnumerable AnalyzeDSCClass(Ast ast, string fileName) + { + return Enumerable.Empty(); } /// @@ -93,7 +81,7 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun /// public string GetName() { - return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideVerboseMessageName); + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseVerboseMessageInDSCResourceName); } /// @@ -102,7 +90,7 @@ public string GetName() /// The common name of this rule public string GetCommonName() { - return string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageCommonName); + return string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceCommonName); } /// @@ -111,7 +99,7 @@ public string GetCommonName() /// The description of this rule public string GetDescription() { - return string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageDescription); + return string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceDescription); } /// @@ -136,7 +124,7 @@ public RuleSeverity GetSeverity() /// public string GetSourceName() { - return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + return string.Format(CultureInfo.CurrentCulture, Strings.DSCSourceName); } } -} +} \ No newline at end of file From c114056035b758b9753e7afc373f1f77d7b480b0 Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Tue, 9 Jun 2015 15:55:59 -0700 Subject: [PATCH 2/6] Moved Verbose Message test to Disabled Rules Folder --- Tests/{Rules => Disabled Rules}/ProvideVerboseMessage.tests.ps1 | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Tests/{Rules => Disabled Rules}/ProvideVerboseMessage.tests.ps1 (100%) diff --git a/Tests/Rules/ProvideVerboseMessage.tests.ps1 b/Tests/Disabled Rules/ProvideVerboseMessage.tests.ps1 similarity index 100% rename from Tests/Rules/ProvideVerboseMessage.tests.ps1 rename to Tests/Disabled Rules/ProvideVerboseMessage.tests.ps1 From 12e6a6ae49d4469e79081d7678538fef0a6c8adc Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Tue, 9 Jun 2015 16:02:20 -0700 Subject: [PATCH 3/6] Fixed Typo in Filename --- Rules/ScriptAnalyzerBuiltinRules.csproj | 2 +- ...MessageInDSCResouce.cs => UseVerboseMessageInDSCResource.cs} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename Rules/{UseVerboseMessageInDSCResouce.cs => UseVerboseMessageInDSCResource.cs} (98%) diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index 040413abf..32697b07f 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -75,7 +75,7 @@ - + True True diff --git a/Rules/UseVerboseMessageInDSCResouce.cs b/Rules/UseVerboseMessageInDSCResource.cs similarity index 98% rename from Rules/UseVerboseMessageInDSCResouce.cs rename to Rules/UseVerboseMessageInDSCResource.cs index 470f59b30..ae33b1fa5 100644 --- a/Rules/UseVerboseMessageInDSCResouce.cs +++ b/Rules/UseVerboseMessageInDSCResource.cs @@ -25,7 +25,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules /// ProvideVerboseMessage: Analyzes the ast to check that Write-Verbose is called for DSC Resources /// [Export(typeof(IDSCResourceRule))] - public class UseVerboseMessageInDSCResouce : SkipNamedBlock, IDSCResourceRule + public class UseVerboseMessageInDSCResource : SkipNamedBlock, IDSCResourceRule { /// /// AnalyzeDSCResource: Analyzes the ast to check that Write-Verbose is called for DSC Resources From bd0742b789ed69b9d7870cae6e4789dd6475fb87 Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Tue, 9 Jun 2015 16:26:10 -0700 Subject: [PATCH 4/6] Tests for Verbose DSC Rule --- .../MSFT_WaitForAny/MSFT_WaitForAny.psm1 | 8 ++++++ .../UseVerboseMessageInDSCResource.Tests.ps1 | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 Tests/Rules/UseVerboseMessageInDSCResource.Tests.ps1 diff --git a/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1 b/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1 index 096cf8b54..5b8ee83d9 100644 --- a/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1 +++ b/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1 @@ -29,6 +29,8 @@ function Get-TargetResource [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value ) + Write-Verbose "In Get-TargetResource" + Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1 $b = @{"hash" = "table"} @@ -75,10 +77,14 @@ function Set-TargetResource [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value ) + Write-Verbose "In Set-TargetResource" + Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1 if ($PSBoundParameters["Verbose"]) { + Write-Verbose "Calling xMachine with Verbose parameter" + PSDSCxMachine\Set-_InternalPSDscXMachineTR ` -RemoteResourceId $ResourceName ` -RemoteMachine $NodeName ` @@ -130,6 +136,8 @@ function Test-TargetResource [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value ) + Write-Verbose "In Test-TargetResource" + Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1 $a = $true diff --git a/Tests/Rules/UseVerboseMessageInDSCResource.Tests.ps1 b/Tests/Rules/UseVerboseMessageInDSCResource.Tests.ps1 new file mode 100644 index 000000000..9b2b3c5b8 --- /dev/null +++ b/Tests/Rules/UseVerboseMessageInDSCResource.Tests.ps1 @@ -0,0 +1,26 @@ +Import-Module PSScriptAnalyzer + +$violationMessage = "There is no call to Write-Verbose in DSC function ‘Set-TargetResource’. If you are using Write-Verbose in a helper function, suppress this rule application." +$violationName = "PSDSCUseVerboseMessageInDSCResource" +$directory = Split-Path -Parent $MyInvocation.MyCommand.Path +$violations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAll\MSFT_WaitForAll.psm1 | Where-Object {$_.RuleName -eq $violationName} +$noViolations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAny\MSFT_WaitForAny.psm1 | Where-Object {$_.RuleName -eq $violationName} +$noClassViolations = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\DSCResources\MyDscResource\MyDscResource.psm1 | Where-Object {$_.RuleName -eq $violationName} + +Describe "UseVerboseMessageInDSCResource" { + Context "When there are violations" { + It "has 2 Verbose Message violations" { + $violations.Count | Should Be 2 + } + + It "has the correct description message" { + $violations[0].Message | Should Match $violationMessage + } + } + + Context "When there are no violations" { + It "returns no violations" { + $noViolations.Count | Should Be 0 + } + } +} \ No newline at end of file From 23981df1612e885880b23690b0590848db3d5d1d Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Wed, 10 Jun 2015 10:00:12 -0700 Subject: [PATCH 5/6] Fix for failing tests since we moved Verbose message rule only for DSC resources --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 4 ++-- Tests/Engine/RuleSuppression.ps1 | 2 +- Tests/Engine/RuleSuppression.tests.ps1 | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index fa5346b6a..2606aab63 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -130,11 +130,11 @@ Describe "TestSeverity" { Describe "TestWildCard" { It "filters rules based on the -Name wild card input" { $rules = Get-ScriptAnalyzerRule -Name PSDSC* - $rules.Count | Should be 6 + $rules.Count | Should be 7 } It "filters rules based on wild card input and severity"{ $rules = Get-ScriptAnalyzerRule -Name PSDSC*　-Severity Information - $rules.Count | Should be 3 + $rules.Count | Should be 4 } } diff --git a/Tests/Engine/RuleSuppression.ps1 b/Tests/Engine/RuleSuppression.ps1 index d5cd4e055..b63aef84c 100644 --- a/Tests/Engine/RuleSuppression.ps1 +++ b/Tests/Engine/RuleSuppression.ps1 @@ -5,7 +5,7 @@ Param( function SuppressMe () { - [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideVerboseMessage")] + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideCommentHelp")] [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "unused1")] Param([string]$unUsed1, [int] $unUsed2) { diff --git a/Tests/Engine/RuleSuppression.tests.ps1 b/Tests/Engine/RuleSuppression.tests.ps1 index bb6ff4a77..17d8a719d 100644 --- a/Tests/Engine/RuleSuppression.tests.ps1 +++ b/Tests/Engine/RuleSuppression.tests.ps1 @@ -5,7 +5,7 @@ $violations = Invoke-ScriptAnalyzer $directory\RuleSuppression.ps1 Describe "RuleSuppressionWithoutScope" { Context "Function" { It "Does not raise violations" { - $suppression = $violations | Where-Object { $_.RuleName -eq "PSProvideVerboseMessage" } + $suppression = $violations | Where-Object { $_.RuleName -eq "PSProvideCommentHelp" } $suppression.Count | Should Be 0 } } From 909db528054c08d6d3e7bf021cfe3e3853d9908d Mon Sep 17 00:00:00 2001 From: "Raghu Shantha [MSFT]" Date: Wed, 10 Jun 2015 13:11:44 -0700 Subject: [PATCH 6/6] Removed reference to unused variable --- Rules/UseVerboseMessageInDSCResource.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Rules/UseVerboseMessageInDSCResource.cs b/Rules/UseVerboseMessageInDSCResource.cs index ae33b1fa5..df556d97c 100644 --- a/Rules/UseVerboseMessageInDSCResource.cs +++ b/Rules/UseVerboseMessageInDSCResource.cs @@ -37,9 +37,7 @@ public IEnumerable AnalyzeDSCResource(Ast ast, string fileName if (ast == null) { throw new ArgumentNullException(Strings.NullAstErrorMessage); - } - - List expectedTargetResourceFunctionNames = new List(new string[] { "Set-TargetResource", "Test-TargetResource", "Get-TargetResource" }); + } IEnumerable functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);