From 9408469458b5ecb66c41ab1b49d475af1518dbda Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Wed, 17 Feb 2021 22:04:32 -0800 Subject: [PATCH 1/6] Add common tools for spell check --- .../templates/steps/check-spelling.yml | 31 +++++++++++ eng/common/scripts/Test-Spelling.ps1 | 52 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 eng/common/pipelines/templates/steps/check-spelling.yml create mode 100644 eng/common/scripts/Test-Spelling.ps1 diff --git a/eng/common/pipelines/templates/steps/check-spelling.yml b/eng/common/pipelines/templates/steps/check-spelling.yml new file mode 100644 index 0000000000000..9aecaa58e04f6 --- /dev/null +++ b/eng/common/pipelines/templates/steps/check-spelling.yml @@ -0,0 +1,31 @@ +# Checks spelling of files that changed between the current state of the repo +# and some ref (branch, tag, etc.) or commit hash. Only runs on PRs. +# ContinueOnError - true: Pipeline warns on spelling error +# false: Pipeline fails on spelling error + +parameters: + ContinueOnError: true + +steps: + - ${{ if eq(variables['Build.Reason'], 'PullRequest') }}: + # Devops uses a git config that changes ref names. + # For example: refs/heads/master -> refs/remotes/origin/master + # Set in: fetch = +refs/heads/*:refs/remotes/origin/* + # Rather than try to convert $(System.PullRequest.TargetBranch) this + # explicitly fetches the ref with a specific name that will not collide with + # the other refs fetched + - task: PowerShell@2 + displayName: Fetch ref for $(System.PullRequest.TargetBranch) + inputs: + pwsh: true + targetType: inline + script: git fetch origin $(System.PullRequest.TargetBranch):refs/spellcheck/baseref + + - task: PowerShell@2 + displayName: Check spelling (cspell) + continueOnError: ${{ parameters.ContinueOnError }} + inputs: + targetType: filePath + filePath: eng/common/scripts/Test-Spelling.ps1 + arguments: -TargetRef refs/spellcheck/baseref + pwsh: true diff --git a/eng/common/scripts/Test-Spelling.ps1 b/eng/common/scripts/Test-Spelling.ps1 new file mode 100644 index 0000000000000..c9c32aed4c373 --- /dev/null +++ b/eng/common/scripts/Test-Spelling.ps1 @@ -0,0 +1,52 @@ +[CmdletBinding()] +Param ( + [Parameter()] + [ValidateNotNullOrEmpty()] + [string] $TargetRef = 'master' +) + +. $PSScriptRoot/logging.ps1 + + +if ((Get-Command npx | Measure-Object).Count -eq 0) { + Write-Error "Could not locate npx. Install NodeJS (includes npm and npx) https://nodejs.org/en/download/" + exit 1 +} + +$initialDirectory = Get-Location +$exitCode = 0 +try { + Set-Location "$PSScriptRoot/../../.." + + # Lists names of files that were in some way changed between the + # current ref and $TargetRef. Excludes files that were deleted to + # prevent errors in Resolve-Path + Write-Host "git diff --diff-filter=d --name-only $TargetRef" + $changedFiles = git diff --diff-filter=d --name-only $TargetRef ` + | Resolve-Path + + $changedFilesCount = ($changedFiles | Measure-Object).Count + Write-Host "Git Detected $changedFilesCount changed file(s). Files checked by cspell may exclude files according to cspell.json" + + if ($changedFilesCount -eq 0) { + Write-Host "No changes detected" + # The finally block still runs after calling exit here + exit $exitCode + } + + $changedFilesString = $changedFiles | Join-String -Separator ' ' + + Write-Host "npx cspell --config ./eng/cspell.json $changedFilesString" + $spellingErrors = Invoke-Expression "npx cspell --config ./eng/cspell.json $changedFilesString" + + if ($spellingErrors) { + $exitCode = 1 + foreach ($spellingError in $spellingErrors) { + LogWarning $spellingError + } + } +} finally { + Set-Location $initialDirectory +} + +exit $exitCode \ No newline at end of file From 5a63ecaa054cd1eb5b5cc681479aa4b95c30a25a Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Wed, 17 Feb 2021 23:28:12 -0800 Subject: [PATCH 2/6] Updates from further iteration --- .../templates/steps/check-spelling.yml | 15 +++++-- eng/common/scripts/Test-Spelling.ps1 | 41 ++++++++++++++++--- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/eng/common/pipelines/templates/steps/check-spelling.yml b/eng/common/pipelines/templates/steps/check-spelling.yml index 9aecaa58e04f6..5958aa2579bc3 100644 --- a/eng/common/pipelines/templates/steps/check-spelling.yml +++ b/eng/common/pipelines/templates/steps/check-spelling.yml @@ -2,24 +2,29 @@ # and some ref (branch, tag, etc.) or commit hash. Only runs on PRs. # ContinueOnError - true: Pipeline warns on spelling error # false: Pipeline fails on spelling error +# TargetRef - Target ref (e.g. master) to compare to create file change +# list +# CspellConfigPath - Path to cspell.json config location parameters: ContinueOnError: true + TargetRef: $(System.PullRequest.TargetBranch) + CspellConfigPath: ./.vscode/cspell.json steps: - ${{ if eq(variables['Build.Reason'], 'PullRequest') }}: # Devops uses a git config that changes ref names. # For example: refs/heads/master -> refs/remotes/origin/master # Set in: fetch = +refs/heads/*:refs/remotes/origin/* - # Rather than try to convert $(System.PullRequest.TargetBranch) this + # Rather than try to convert ${{ parameters.TargetRef }} this # explicitly fetches the ref with a specific name that will not collide with # the other refs fetched - task: PowerShell@2 - displayName: Fetch ref for $(System.PullRequest.TargetBranch) + displayName: Fetch ref for ${{ parameters.TargetRef }} inputs: pwsh: true targetType: inline - script: git fetch origin $(System.PullRequest.TargetBranch):refs/spellcheck/baseref + script: git fetch origin ${{ parameters.TargetRef }}:refs/spellcheck/baseref - task: PowerShell@2 displayName: Check spelling (cspell) @@ -27,5 +32,7 @@ steps: inputs: targetType: filePath filePath: eng/common/scripts/Test-Spelling.ps1 - arguments: -TargetRef refs/spellcheck/baseref + arguments: >- + -TargetRef refs/spellcheck/baseref + -CspellConfigPath ${{ parameters.CspellConfigPath }} pwsh: true diff --git a/eng/common/scripts/Test-Spelling.ps1 b/eng/common/scripts/Test-Spelling.ps1 index c9c32aed4c373..b2f8162746166 100644 --- a/eng/common/scripts/Test-Spelling.ps1 +++ b/eng/common/scripts/Test-Spelling.ps1 @@ -2,14 +2,18 @@ Param ( [Parameter()] [ValidateNotNullOrEmpty()] - [string] $TargetRef = 'master' + [string] $TargetRef = 'master', + + [Parameter()] + [ValidateNotNullOrEmpty()] + [string] $CspellConfigPath = "./.vscode/cspell.json" ) . $PSScriptRoot/logging.ps1 if ((Get-Command npx | Measure-Object).Count -eq 0) { - Write-Error "Could not locate npx. Install NodeJS (includes npm and npx) https://nodejs.org/en/download/" + LogError "Could not locate npx. Install NodeJS (includes npm and npx) https://nodejs.org/en/download/" exit 1 } @@ -36,8 +40,8 @@ try { $changedFilesString = $changedFiles | Join-String -Separator ' ' - Write-Host "npx cspell --config ./eng/cspell.json $changedFilesString" - $spellingErrors = Invoke-Expression "npx cspell --config ./eng/cspell.json $changedFilesString" + Write-Host "npx cspell --config $CspellConfigPath $changedFilesString" + $spellingErrors = Invoke-Expression "npx cspell --config $CspellConfigPath $changedFilesString" if ($spellingErrors) { $exitCode = 1 @@ -49,4 +53,31 @@ try { Set-Location $initialDirectory } -exit $exitCode \ No newline at end of file +exit $exitCode + +<# +.SYNOPSIS +Uses cspell (from NPM) to check spelling of recently changed files + +.DESCRIPTION +This script checks files that have changed relative to a base branch (default +`master`) for spelling errors. Dictionaries and spelling configurations reside +in a configurable `cspell.json` location. + +This script uses `npx` and assumes that NodeJS (and by extension `npm` and `npx` +) are installed on the machine. If it does not detect `npx` it will warn the +user and exit with an error. + +The entire file is scanned, not just changed sections. Spelling errors in parts +of the file not touched will still be shown. + +Running this on the local machine will trigger tests + +.PARAMETER TargetRef +Optional git ref to compare changes. Default value is `master`. + +.PARAMETER CspellConfigPath +Optional location to use for cspell.json path. Default value is +`./.vscode/cspell.json` + +#> From fcf77e6a105d804a164dba2326c419c15f15718d Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Thu, 18 Feb 2021 09:04:51 -0800 Subject: [PATCH 3/6] Review feedback: Add check for git as well --- eng/common/scripts/Test-Spelling.ps1 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/eng/common/scripts/Test-Spelling.ps1 b/eng/common/scripts/Test-Spelling.ps1 index b2f8162746166..2ce21095b51d5 100644 --- a/eng/common/scripts/Test-Spelling.ps1 +++ b/eng/common/scripts/Test-Spelling.ps1 @@ -11,6 +11,10 @@ Param ( . $PSScriptRoot/logging.ps1 +if ((Get-Command git | Measure-Object).Count -eq 0) { + LogError "Could not locate git. Install git https://git-scm.com/downloads" + exit 1 +} if ((Get-Command npx | Measure-Object).Count -eq 0) { LogError "Could not locate npx. Install NodeJS (includes npm and npx) https://nodejs.org/en/download/" From 3adcb6b46abcda60d15e680ba5c7f07f96bd36f1 Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Thu, 18 Feb 2021 23:27:31 -0800 Subject: [PATCH 4/6] Review feedback * Use common approach to resolving base branch name * Eliminate default base branch "master" as this will be changed later, providing no default and using a mandatory parameter means a dev must provide the value * Check for existence of $CspellConfigPath * No need to Set-Location if run from the root of the repo (most common scenario) * -join --- .../templates/steps/check-spelling.yml | 17 +---- eng/common/scripts/Test-Spelling.ps1 | 76 ++++++++++--------- 2 files changed, 42 insertions(+), 51 deletions(-) diff --git a/eng/common/pipelines/templates/steps/check-spelling.yml b/eng/common/pipelines/templates/steps/check-spelling.yml index 5958aa2579bc3..7654d5b45ffd8 100644 --- a/eng/common/pipelines/templates/steps/check-spelling.yml +++ b/eng/common/pipelines/templates/steps/check-spelling.yml @@ -3,7 +3,7 @@ # ContinueOnError - true: Pipeline warns on spelling error # false: Pipeline fails on spelling error # TargetRef - Target ref (e.g. master) to compare to create file change -# list +# list. # CspellConfigPath - Path to cspell.json config location parameters: @@ -13,19 +13,6 @@ parameters: steps: - ${{ if eq(variables['Build.Reason'], 'PullRequest') }}: - # Devops uses a git config that changes ref names. - # For example: refs/heads/master -> refs/remotes/origin/master - # Set in: fetch = +refs/heads/*:refs/remotes/origin/* - # Rather than try to convert ${{ parameters.TargetRef }} this - # explicitly fetches the ref with a specific name that will not collide with - # the other refs fetched - - task: PowerShell@2 - displayName: Fetch ref for ${{ parameters.TargetRef }} - inputs: - pwsh: true - targetType: inline - script: git fetch origin ${{ parameters.TargetRef }}:refs/spellcheck/baseref - - task: PowerShell@2 displayName: Check spelling (cspell) continueOnError: ${{ parameters.ContinueOnError }} @@ -33,6 +20,6 @@ steps: targetType: filePath filePath: eng/common/scripts/Test-Spelling.ps1 arguments: >- - -TargetRef refs/spellcheck/baseref + -TargetRef "origin/$("${{ parameters.TargetRef }}" -replace "refs/heads/")" -CspellConfigPath ${{ parameters.CspellConfigPath }} pwsh: true diff --git a/eng/common/scripts/Test-Spelling.ps1 b/eng/common/scripts/Test-Spelling.ps1 index 2ce21095b51d5..0b3abb0808b36 100644 --- a/eng/common/scripts/Test-Spelling.ps1 +++ b/eng/common/scripts/Test-Spelling.ps1 @@ -1,8 +1,8 @@ [CmdletBinding()] Param ( - [Parameter()] + [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] - [string] $TargetRef = 'master', + [string] $TargetRef, [Parameter()] [ValidateNotNullOrEmpty()] @@ -21,40 +21,38 @@ if ((Get-Command npx | Measure-Object).Count -eq 0) { exit 1 } -$initialDirectory = Get-Location +if (!(Test-Path $CspellConfigPath)) { + LogError "Could not locate config file $CspellConfigPath" + exit 1 +} + $exitCode = 0 -try { - Set-Location "$PSScriptRoot/../../.." - - # Lists names of files that were in some way changed between the - # current ref and $TargetRef. Excludes files that were deleted to - # prevent errors in Resolve-Path - Write-Host "git diff --diff-filter=d --name-only $TargetRef" - $changedFiles = git diff --diff-filter=d --name-only $TargetRef ` - | Resolve-Path - - $changedFilesCount = ($changedFiles | Measure-Object).Count - Write-Host "Git Detected $changedFilesCount changed file(s). Files checked by cspell may exclude files according to cspell.json" - - if ($changedFilesCount -eq 0) { - Write-Host "No changes detected" - # The finally block still runs after calling exit here - exit $exitCode - } - - $changedFilesString = $changedFiles | Join-String -Separator ' ' - - Write-Host "npx cspell --config $CspellConfigPath $changedFilesString" - $spellingErrors = Invoke-Expression "npx cspell --config $CspellConfigPath $changedFilesString" - - if ($spellingErrors) { - $exitCode = 1 - foreach ($spellingError in $spellingErrors) { - LogWarning $spellingError - } - } -} finally { - Set-Location $initialDirectory + +# Lists names of files that were in some way changed between the +# current ref and $TargetRef. Excludes files that were deleted to +# prevent errors in Resolve-Path +Write-Host "git diff --diff-filter=d --name-only $TargetRef" +$changedFiles = git diff --diff-filter=d --name-only $TargetRef ` + | Resolve-Path + +$changedFilesCount = ($changedFiles | Measure-Object).Count +Write-Host "Git Detected $changedFilesCount changed file(s). Files checked by cspell may exclude files according to cspell.json" + +if ($changedFilesCount -eq 0) { + Write-Host "No changes detected" + exit $exitCode +} + +$changedFilesString = $changedFiles -join ' ' + +Write-Host "npx cspell --config $CspellConfigPath $changedFilesString" +$spellingErrors = Invoke-Expression "npx cspell --config $CspellConfigPath $changedFilesString" + +if ($spellingErrors) { + $exitCode = 1 + foreach ($spellingError in $spellingErrors) { + LogWarning $spellingError + } } exit $exitCode @@ -78,10 +76,16 @@ of the file not touched will still be shown. Running this on the local machine will trigger tests .PARAMETER TargetRef -Optional git ref to compare changes. Default value is `master`. +Git ref to compare changes. .PARAMETER CspellConfigPath Optional location to use for cspell.json path. Default value is `./.vscode/cspell.json` +.EXAMPLE +./eng/common/scripts/Test-Spelling.ps1 -TargetRef 'target_branch_name' + +This will run spell check with changes in the current branch with respect to +`target_branch_name` + #> From 9ea39cea81c35fd69880da7583971f91f50735e2 Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Fri, 19 Feb 2021 15:19:09 -0800 Subject: [PATCH 5/6] Review feedback: Rename TargetRef -> TargetBranch, add SourceBranch, Add reference to spelling docs, exit 0, Rename to check-spelling-in-changed-files.ps1, --- .../templates/steps/check-spelling.yml | 18 +++++---- ...s1 => check-spelling-in-changed-files.ps1} | 38 +++++++++++-------- 2 files changed, 33 insertions(+), 23 deletions(-) rename eng/common/scripts/{Test-Spelling.ps1 => check-spelling-in-changed-files.ps1} (67%) diff --git a/eng/common/pipelines/templates/steps/check-spelling.yml b/eng/common/pipelines/templates/steps/check-spelling.yml index 7654d5b45ffd8..3865a3f26ece8 100644 --- a/eng/common/pipelines/templates/steps/check-spelling.yml +++ b/eng/common/pipelines/templates/steps/check-spelling.yml @@ -1,14 +1,15 @@ # Checks spelling of files that changed between the current state of the repo # and some ref (branch, tag, etc.) or commit hash. Only runs on PRs. -# ContinueOnError - true: Pipeline warns on spelling error -# false: Pipeline fails on spelling error -# TargetRef - Target ref (e.g. master) to compare to create file change -# list. -# CspellConfigPath - Path to cspell.json config location +# ContinueOnError - true: Pipeline warns on spelling error +# false: Pipeline fails on spelling error +# TargetBranch - Target ref (e.g. master) to compare to create file change +# list. +# CspellConfigPath - Path to cspell.json config location parameters: ContinueOnError: true - TargetRef: $(System.PullRequest.TargetBranch) + TargetBranch: $(System.PullRequest.TargetBranch) + SourceBranch: HEAD CspellConfigPath: ./.vscode/cspell.json steps: @@ -18,8 +19,9 @@ steps: continueOnError: ${{ parameters.ContinueOnError }} inputs: targetType: filePath - filePath: eng/common/scripts/Test-Spelling.ps1 + filePath: eng/common/scripts/check-spelling-in-changed-files.ps1 arguments: >- - -TargetRef "origin/$("${{ parameters.TargetRef }}" -replace "refs/heads/")" + -TargetBranch "origin/$("${{ parameters.TargetBranch }}" -replace "refs/heads/")" + -SourceBranch ${{ parameters.SourceBranch }} -CspellConfigPath ${{ parameters.CspellConfigPath }} pwsh: true diff --git a/eng/common/scripts/Test-Spelling.ps1 b/eng/common/scripts/check-spelling-in-changed-files.ps1 similarity index 67% rename from eng/common/scripts/Test-Spelling.ps1 rename to eng/common/scripts/check-spelling-in-changed-files.ps1 index 0b3abb0808b36..cc1b21bf1db0e 100644 --- a/eng/common/scripts/Test-Spelling.ps1 +++ b/eng/common/scripts/check-spelling-in-changed-files.ps1 @@ -1,14 +1,17 @@ [CmdletBinding()] Param ( - [Parameter(Mandatory)] - [ValidateNotNullOrEmpty()] - [string] $TargetRef, + [Parameter()] + [string] $TargetBranch, + + [Parameter()] + [string] $SourceBranch, [Parameter()] [ValidateNotNullOrEmpty()] [string] $CspellConfigPath = "./.vscode/cspell.json" ) +$ErrorActionPreference = "Continue" . $PSScriptRoot/logging.ps1 if ((Get-Command git | Measure-Object).Count -eq 0) { @@ -26,13 +29,11 @@ if (!(Test-Path $CspellConfigPath)) { exit 1 } -$exitCode = 0 - # Lists names of files that were in some way changed between the -# current ref and $TargetRef. Excludes files that were deleted to +# current ref and $TargetBranch. Excludes files that were deleted to # prevent errors in Resolve-Path -Write-Host "git diff --diff-filter=d --name-only $TargetRef" -$changedFiles = git diff --diff-filter=d --name-only $TargetRef ` +Write-Host "git diff --diff-filter=d --name-only $TargetBranch $SourceBranch" +$changedFiles = git diff --diff-filter=d --name-only $TargetBranch $SourceBranch ` | Resolve-Path $changedFilesCount = ($changedFiles | Measure-Object).Count @@ -40,7 +41,7 @@ Write-Host "Git Detected $changedFilesCount changed file(s). Files checked by cs if ($changedFilesCount -eq 0) { Write-Host "No changes detected" - exit $exitCode + exit 0 } $changedFilesString = $changedFiles -join ' ' @@ -49,13 +50,13 @@ Write-Host "npx cspell --config $CspellConfigPath $changedFilesString" $spellingErrors = Invoke-Expression "npx cspell --config $CspellConfigPath $changedFilesString" if ($spellingErrors) { - $exitCode = 1 foreach ($spellingError in $spellingErrors) { LogWarning $spellingError - } + } + LogWarning "Spelling errors detected. To correct false positives or learn about spell checking see: https://aka.ms/azsdk/engsys/spellcheck" } -exit $exitCode +exit 0 <# .SYNOPSIS @@ -75,15 +76,22 @@ of the file not touched will still be shown. Running this on the local machine will trigger tests -.PARAMETER TargetRef -Git ref to compare changes. +.PARAMETER TargetBranch +Git ref to compare changes. This is usually the "base" (GitHub) or "target" +(DevOps) branch for which a pull request would be opened. + +.PARAMETER SouceBranch +Git ref to use instead of changes in current repo state. Use `HEAD` here to +check spelling of files that have been committed and exlcude any new files or +modified files that are not committed. This is most useful in CI scenarios where +builds may have modified the state of the repo. .PARAMETER CspellConfigPath Optional location to use for cspell.json path. Default value is `./.vscode/cspell.json` .EXAMPLE -./eng/common/scripts/Test-Spelling.ps1 -TargetRef 'target_branch_name' +./eng/common/scripts/Test-Spelling.ps1 -TargetBranch 'target_branch_name' This will run spell check with changes in the current branch with respect to `target_branch_name` From cfad164ee53aebe82c1b1e92405b1da5d327d0b3 Mon Sep 17 00:00:00 2001 From: Daniel Jurek Date: Mon, 22 Feb 2021 13:31:33 -0800 Subject: [PATCH 6/6] Review feedback: Remove ValidateNotNullOrEmpty (we do more definitive validation farther down that will also catch these errors), Update comments and script name --- eng/common/scripts/check-spelling-in-changed-files.ps1 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/eng/common/scripts/check-spelling-in-changed-files.ps1 b/eng/common/scripts/check-spelling-in-changed-files.ps1 index cc1b21bf1db0e..8a9654d203c76 100644 --- a/eng/common/scripts/check-spelling-in-changed-files.ps1 +++ b/eng/common/scripts/check-spelling-in-changed-files.ps1 @@ -7,7 +7,6 @@ Param ( [string] $SourceBranch, [Parameter()] - [ValidateNotNullOrEmpty()] [string] $CspellConfigPath = "./.vscode/cspell.json" ) @@ -30,7 +29,7 @@ if (!(Test-Path $CspellConfigPath)) { } # Lists names of files that were in some way changed between the -# current ref and $TargetBranch. Excludes files that were deleted to +# current $SourceBranch and $TargetBranch. Excludes files that were deleted to # prevent errors in Resolve-Path Write-Host "git diff --diff-filter=d --name-only $TargetBranch $SourceBranch" $changedFiles = git diff --diff-filter=d --name-only $TargetBranch $SourceBranch ` @@ -84,14 +83,15 @@ Git ref to compare changes. This is usually the "base" (GitHub) or "target" Git ref to use instead of changes in current repo state. Use `HEAD` here to check spelling of files that have been committed and exlcude any new files or modified files that are not committed. This is most useful in CI scenarios where -builds may have modified the state of the repo. +builds may have modified the state of the repo. Leaving this parameter blank +includes files for whom changes have not been committed. .PARAMETER CspellConfigPath Optional location to use for cspell.json path. Default value is `./.vscode/cspell.json` .EXAMPLE -./eng/common/scripts/Test-Spelling.ps1 -TargetBranch 'target_branch_name' +./eng/common/scripts/check-spelling-in-changed-files.ps1 -TargetBranch 'target_branch_name' This will run spell check with changes in the current branch with respect to `target_branch_name`