Skip to content
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.

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

[TypeSpecValidation] Add suppression mechanism specific to TSV-All #30324

Merged
merged 12 commits into from
Aug 26, 2024
60 changes: 60 additions & 0 deletions eng/scripts/Suppressions-Functions.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<#
.DESCRIPTION
Returns the suppressions for a tool applicable to a path. Walks up the directory tree to find all files named
"suppressions.yaml", parses and validates the contents, and returns the suppressions matching the tool and path.
Suppressions are ordered by file (closest to path is first), then within the file (closest to top is first).

.PARAMETER Tool
Name of tool. Matched against property "tool" in suppressions.yaml.

.PARAMETER Path
Path to file or directory under analysis.

.OUTPUTS
Hashtable[]
Array of suppressions matching tool and path (may be empty). See the "get-suppressions" tool for the definition
of the suppression object.
#>
function Get-Suppressions {
param (
[string]$Tool,
[string]$Path
)

# -NoEnumerate to prevent single-element arrays from being collapsed to a single object
# -AsHashtable is closer to raw JSON than PSCustomObject
$suppressions = npm exec --no -- get-suppressions $Tool $Path | ConvertFrom-Json -NoEnumerate -AsHashtable

if ($LASTEXITCODE -ne 0) {
throw "Failure running 'npm exec get-suppressions'"
}

return $suppressions;
}

<#
.DESCRIPTION
Returns the first suppression for a tool applicable to a path. Walks up the directory tree to find all files named
"suppressions.yaml", parses and validates the contents, and returns the first suppression matching the tool and path.
Suppressions are ordered by file (closest to path is first), then within the file (closest to top is first).

.PARAMETER Tool
Name of tool. Matched against property "tool" in suppressions.yaml.

.PARAMETER Path
Path to file or directory under analysis.

.OUTPUTS
Hashtable
First suppressions matching tool and path (may be null). See the "get-suppressions" tool for the definition
of the suppression object.
#>
function Get-Suppression {
param (
[string]$Tool,
[string]$Path
)

$suppressions = @(Get-Suppressions $Tool $Path)
return $suppressions ? $suppressions[0] : $null;
}
18 changes: 4 additions & 14 deletions eng/scripts/TypeSpec-Requirement.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,14 @@ Set-StrictMode -Version 3

. $PSScriptRoot/ChangedFiles-Functions.ps1
. $PSScriptRoot/Logging-Functions.ps1
. $PSScriptRoot/Suppressions-Functions.ps1

function Get-Suppression {
function Get-ValidatedSuppression {
param (
[string]$fileInSpecFolder
)

# -NoEnumerate to prevent single-element arrays from being collapsed to a single object
# -AsHashtable is closer to raw JSON than PSCustomObject
$suppressions = npm exec --no -- get-suppressions TypeSpecRequirement $fileInSpecFolder | ConvertFrom-Json -NoEnumerate -AsHashtable

if ($LASTEXITCODE -ne 0) {
LogError "Failure running 'npm exec get-suppressions'"
LogJobFailure
exit 1
}

# For now, we just use the first matching suppression returned by "get-suppressions" (#29003)
$suppression = $suppressions ? $suppressions[0] : $null
$suppression = Get-Suppression "TypeSpecRequirement" $fileInSpecFolder

if ($suppression) {
# Each path must specify a single version (without wildcards) under "preview|stable"
Expand Down Expand Up @@ -90,7 +80,7 @@ else {

$fullPath = (Join-Path $repoPath $file)

$suppression = Get-Suppression $fullPath
$suppression = Get-ValidatedSuppression $fullPath
if ($suppression) {
$reason = $suppression["reason"] ?? "<no reason specified>"

Expand Down
19 changes: 19 additions & 0 deletions eng/scripts/TypeSpec-Validation.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
param (
[switch]$CheckAll = $false,
[switch]$GitClean = $false,
[switch]$DryRun = $false,
[string]$BaseCommitish = "HEAD^",
[string]$TargetCommitish = "HEAD"
)

. $PSScriptRoot/Logging-Functions.ps1
. $PSScriptRoot/Suppressions-Functions.ps1

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

Expand All @@ -15,7 +17,24 @@ if ($typespecFolders) {
$typespecFolders = $typespecFolders.Split('',[System.StringSplitOptions]::RemoveEmptyEntries)
foreach ($typespecFolder in $typespecFolders) {
LogGroupStart "Validating $typespecFolder"

if ($CheckAll) {
$suppression = Get-Suppression "TypeSpecValidationAll" $typespecFolder
if ($suppression) {
$reason = $suppression["reason"] ?? "<no reason specified>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it is worth putting this default into the core suppression tool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean because it's a little bit of duplicate code. However, I think it's a tricky question, how much the core get-suppressions tool (both the CLI and the JS function) should respect what's actually in suppressions.yaml, versus adding defaults like a reason text if none is specified.

For now, I think leave it to clients, what to do if the reason is unspecified. If we wanted to make any change in this area, we could consider requiring reason in the suppressions.yaml, so spec authors always need to set some reason string. It's currently optional (but strongly recommended).

LogInfo "Suppressed: $reason"
LogGroupEnd
continue
}
}

LogInfo "npm exec --no -- tsv $typespecFolder"

if ($DryRun) {
LogGroupEnd
continue
}

npm exec --no -- tsv $typespecFolder 2>&1 | Write-Host
if ($LASTEXITCODE) {
$typespecFoldersWithFailures += $typespecFolder
Expand Down