Skip to content

Commit

Permalink
Improve Error Logging for CI checks (#26078)
Browse files Browse the repository at this point in the history
  • Loading branch information
weshaggard authored Oct 5, 2023
1 parent f3b2c51 commit be3cd97
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 82 deletions.
28 changes: 14 additions & 14 deletions documentation/ci-fix.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,20 +130,21 @@ Then refer to [this TSG](https://dev.azure.com/azure-sdk/internal/_wiki/wikis/in

## TypeSpec Validation

This validator is to ensure the TypeSpec & swagger files in PR are consistent and passing validation.
This validator will help ensure your TypeSpec project follows [standard conventions](https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/typespec-structure-guidelines.md) as well ensures that the [generated swagger](https://azure.github.io/typespec-azure/docs/emitters/typespec-autorest) files are in-sync with your project.

### How to fix

| Error Code | Severity | Solution |
|------------------------------|----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| MissingTypeSpecFile | Error | Adding the related TypeSpec project into {RP-Name} folder, like [Qumulo.Management](https://github.com/Azure/azure-rest-api-specs/tree/main/specification/liftrqumulo/Qumulo.Management) |
| MissingExamplesDirectory | Error | The example files should be kept in the 'examples' folder under the TypeSpec project,the typespec-autorest emitter will copy them into the output folder and create corresponding 'x-ms-examples' in the swagger automatically when geneates the swagger, you should also check in it in PR. |
| InConsistentSwagger | Error | The generated swagger is inconsistent with the swagger in PR, so you need to re-generate swagger from TypeSpec project and check in it. |
| SwaggerNotExistInPR | Error | It occurs when there is a TypeSpec file in the PR but the generated swagger is not present in the PR, so you need to add the swagger to the PR. |
| GeneratedSwaggerNotFound | Error | It occurs when there is a TypeSpec file in the PR but there is no swagger produced by the typespec-autorest emitter, so you need to check the tspconfig.yaml to see if it has the wrong configuration, like 'output-dir' or 'azure-resource-provider-folder'. |
| MissingTypeSpecProjectConfig | Warning | The configuration of '@azure-tools/typespec-autorest' including 'output-file','azure-resource-provider-folder' are used to customize the generated swagger file name and folder structure, it's recommended to use them in the 'tspconfig.yaml', here is a [sample tspconfig.yaml]. |

See [typespec-autorest](https://azure.github.io/typespec-azure/docs/emitters/typespec-autorest) for more information about how the swagger is generated from the TypeSpec project.
### Run tsv locally
```
cd <repo>
git checkout <your-branch>
git merge <target-branch>
npm ci
npx tsv <path-to-your-spec>
git commit; git push (if any changes)
# example
npx tsv specification/contosowidgetmanager/Contoso.WidgetManager
```
Then check any errors that might be outputted and address any issues as needed. If there are changed files after the run it generally means that the generated swagger files were not in-sync with the TypeSpec project and you should include those changes in your pull request as well.

## Suppression Process

Expand All @@ -159,4 +160,3 @@ Following tools have been removed from the validation toolchain as of August 202

[automated validation tooling]: https://eng.ms/docs/products/azure-developer-experience/design/api-specs/api-tooling
[Azure REST API specs PR]: https://eng.ms/docs/products/azure-developer-experience/design/api-specs-pr/api-specs-pr
[sample tspconfig.yaml]: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml#L11
1 change: 1 addition & 0 deletions eng/pipelines/typespec-validation-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@ jobs:
- pwsh: |
$(Build.SourcesDirectory)/eng/scripts/Validate-TypeSpec.ps1 -CheckAll -GitClean -Verbose
displayName: Validate All Specs
ignoreLASTEXITCODE: true
condition: succeededOrFailed()
1 change: 1 addition & 0 deletions eng/pipelines/typespec-validation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ jobs:
- pwsh: |
$(Build.SourcesDirectory)/eng/scripts/Validate-TypeSpec.ps1 -GitClean -Verbose
displayName: Validate Impacted Specs
ignoreLASTEXITCODE: true
condition: succeededOrFailed()
84 changes: 40 additions & 44 deletions eng/scripts/ChangedFiles-Functions.ps1
Original file line number Diff line number Diff line change
@@ -1,58 +1,54 @@
function Get-ChangedFiles($baseCommitish="HEAD^", $targetCommitish="HEAD", $diffFilter="d")
{
# diff-filter=d is to exclude any deleted files as we don't generally
# want to do work against files that no longer exist.
# https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
function Get-ChangedFiles($baseCommitish = "HEAD^", $targetCommitish = "HEAD", $diffFilter = "d") {
# diff-filter=d is to exclude any deleted files as we don't generally
# want to do work against files that no longer exist.
# https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203

# core.quotepath=off is to help avoid quoting in the returned paths
# https://git-scm.com/docs/git-config/2.22.0#Documentation/git-config.txt-corequotePath
# core.quotepath=off is to help avoid quoting in the returned paths
# https://git-scm.com/docs/git-config/2.22.0#Documentation/git-config.txt-corequotePath

# Get all the files that have changed between HEAD and the commit before head
# For PR's that last commit is always a merge commit so HEAD^ will get the parent
# commit of the base branch and as such will diff HEAD against HEAD^
$changedFiles = git -c core.quotepath=off diff --name-only --diff-filter=$diffFilter $baseCommitish $targetCommitish
# Get all the files that have changed between HEAD and the commit before head
# For PR's that last commit is always a merge commit so HEAD^ will get the parent
# commit of the base branch and as such will diff HEAD against HEAD^
$changedFiles = git -c core.quotepath=off diff --name-only --diff-filter=$diffFilter $baseCommitish $targetCommitish

Write-Verbose "Changed files:"
$changedFiles | ForEach-Object { Write-Verbose "$_" }
Write-Verbose "Changed files:"
$changedFiles | ForEach-Object { Write-Verbose "$_" }

return $changedFiles
return $changedFiles
}

function Get-ChangedSwaggerFiles()
{
$changedFiles = Get-ChangedFilesUnderSpecification
function Get-ChangedSwaggerFiles() {
$changedFiles = Get-ChangedFilesUnderSpecification

$changedSwaggerFiles = $changedFiles | Where-Object {
$_.EndsWith(".json")
}
$changedSwaggerFiles = $changedFiles | Where-Object {
$_.EndsWith(".json")
}

return $changedSwaggerFiles
return $changedSwaggerFiles
}

function Get-ChangedFilesUnderSpecification($changedFiles = (Get-ChangedFiles))
{
$changedFilesUnderSpecification = $changedFiles | Where-Object {
$_.StartsWith("specification")
}
function Get-ChangedFilesUnderSpecification($changedFiles = (Get-ChangedFiles)) {
$changedFilesUnderSpecification = $changedFiles | Where-Object {
$_.StartsWith("specification")
}

return $changedFilesUnderSpecification
return $changedFilesUnderSpecification
}

function Get-ChangedCoreFiles($changedFiles = (Get-ChangedFiles))
{
$rootFiles = @(
".gitattributes",
".prettierrc.json",
"package-lock.json",
"package.json",
"tsconfig.json"
)

$coreFiles = $changedFiles | Where-Object {
$_.StartsWith("eng/") -or
$_.StartsWith("specification/common-types/") -or
$_ -in $rootFiles
}

return $coreFiles
function Get-ChangedCoreFiles($changedFiles = (Get-ChangedFiles)) {
$rootFiles = @(
".gitattributes",
".prettierrc.json",
"package-lock.json",
"package.json",
"tsconfig.json"
)

$coreFiles = $changedFiles | Where-Object {
$_.StartsWith("eng/") -or
$_.StartsWith("specification/common-types/") -or
$_ -in $rootFiles
}

return $coreFiles
}
61 changes: 61 additions & 0 deletions eng/scripts/Logging-Functions.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
function Test-SupportsDevOpsLogging() {
return ($null -ne $env:SYSTEM_TEAMPROJECTID)
}

function LogInfo {
Write-Host "$args"
}

function LogWarning {
if (Test-SupportsDevOpsLogging) {
Write-Host ("##vso[task.LogIssue type=warning;]$args" -replace "`n", "%0D%0A")
}
else {
Write-Warning "$args"
}
}

function LogErrorForFile($file, $errorString)
{
if (Test-SupportsDevOpsLogging) {
Write-Host ("##vso[task.logissue type=error;sourcepath=$file;linenumber=1;columnnumber=1;]$errorString" -replace "`n", "%0D%0A")
}
else {
Write-Error "[Error in file $file]$errorString"
}
}
function LogError {
if (Test-SupportsDevOpsLogging) {
Write-Host ("##vso[task.LogIssue type=error;]$args" -replace "`n", "%0D%0A")
}
else {
Write-Error "$args"
}
}

function LogDebug {
if (Test-SupportsDevOpsLogging) {
Write-Host "[debug]$args"
}
else {
Write-Debug "$args"
}
}

function LogGroupStart() {
if (Test-SupportsDevOpsLogging) {
Write-Host "##[group]$args"
}
}

function LogGroupEnd() {
if (Test-SupportsDevOpsLogging) {
Write-Host "##[endgroup]"
}
}

function LogJobFailure() {
if (Test-SupportsDevOpsLogging) {
Write-Host "##vso[task.complete result=Failed;]"
}
}
35 changes: 22 additions & 13 deletions eng/scripts/Swagger-Prettier-Check.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,49 @@ param (
Set-StrictMode -Version 3

. $PSScriptRoot/ChangedFiles-Functions.ps1

$exitCode = 0
. $PSScriptRoot/Logging-Functions.ps1

$repoPath = Resolve-Path "$PSScriptRoot/../.."
$pathsWithErrors = @()

if ($CheckAll) {
Write-Host "npx --no -- prettier --check $repoPath/specification/**/*.json --log-level warn"
LogInfo "npx --no -- prettier --check $repoPath/specification/**/*.json --log-level warn"
npx --no -- prettier --check $repoPath/specification/**/*.json --log-level warn
if ($LASTEXITCODE) {
Write-Host "##vso[task.logissue type=error;]Code style issues found please run prettier.%0D%0A> npm install%0D%0A> npx prettier --write $repoPath/specification/**/*.json"
$exitCode = 1
$pathsWithErrors += "$repoPath/specification/**/*.json"
}
}
else
{
$filesToCheck = @(Get-ChangedSwaggerFiles)
if (!$filesToCheck) {
Write-Host "No specification files found to check."
LogInfo "No specification files found to check."
}
else {
foreach ($file in $filesToCheck) {
Write-Host "npx --no -- prettier --check $repoPath/$file --log-level warn"
LogInfo "npx --no -- prettier --check $repoPath/$file --log-level warn"
npx --no -- prettier --check $repoPath/$file --log-level warn
if ($LASTEXITCODE) {
Write-Host "##vso[task.logissue type=error;sourcepath=$file;linenumber=1;columnnumber=1;]Code style issues found, please run prettier.%0D%0A> npm install%0D%0A> npx prettier --write $file"
$exitCode = 1
$pathsWithErrors += $file
}
}
}
}

if ($exitCode) {
Write-Host "##vso[task.logissue type=error;]Code style issues found in the above file(s), please run prettier to update. For more detailed docs see https://aka.ms/azsdk/specs/prettier"
Write-Host "##vso[task.complete result=Failed;]"
if ($pathsWithErrors.Count -gt 0)
{
# DevOps only adds the first 4 errors to the github checks list so lets always add the generic one first and then as many of the individual ones as can be found afterwards
LogError "Code style issues found in the above file(s), please run prettier to update. For more detailed docs see https://aka.ms/azsdk/specs/prettier"
LogJobFailure

foreach ($path in $pathsWithErrors)
{
$errorString = "Code style issues found, please run prettier."
$errorString += "`n > npm install"
$errorString += "`n > npx prettier --write $path"
LogErrorForFile $path $errorString
}
exit 1
}

exit $exitCode
exit 0
34 changes: 23 additions & 11 deletions eng/scripts/Validate-TypeSpec.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,42 @@ param (
[switch]$GitClean = $false
)

$exitCode = 0
. $PSScriptRoot/Logging-Functions.ps1

$typespecFolders = &"$PSScriptRoot/Get-TypeSpec-Folders.ps1" -CheckAll:$CheckAll

Write-Host "typespecFolders:"
foreach ($typespecFolder in $typespecFolders) {
Write-Host " $typespecFolder"
}
Write-Host

$typespecFoldersWithFailures = @()
if ($typespecFolders) {
$typespecFolders = $typespecFolders.Split('',[System.StringSplitOptions]::RemoveEmptyEntries)
foreach ($typespecFolder in $typespecFolders) {
LogGroupStart "Validating $typespecFolder"
LogInfo "npx --no tsv $typespecFolder"
npx --no tsv $typespecFolder 2>&1 | Write-Host
if ($LASTEXITCODE) {
$exitCode = 1
$typespecFoldersWithFailures += $typespecFolder
$errorString = "TypeSpec Validation failed for project $typespecFolder run the following command locally to validate."
$errorString += "`n > npm ci"
$errorString += "`n > npx tsv $typespecFolder"
$errorString += "`nFor more detailed docs see https://aka.ms/azsdk/specs/typespec-validation"
LogError $errorString
}
if ($GitClean) {
Write-Host "git restore ."
git restore .
Write-Host "git clean -df"
git clean -df
}
LogGroupEnd
}
}

if ($typespecFoldersWithFailures.Count -gt 0) {
LogInfo "TypeSpec Validation failed for some folder to fix run and address any errors:"
LogInfo " > npm ci"
foreach ($typespecFolderWithFailure in $typespecFoldersWithFailures) {
LogInfo " > npx tsv $typespecFolderWithFailure"
}
LogInfo "For more detailed docs see https://aka.ms/azsdk/specs/typespec-validation"
LogJobFailure
exit 1
}

exit $exitCode
exit 0

0 comments on commit be3cd97

Please sign in to comment.