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.

Sign up for GitHub

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

[TypeSpecRequirement] Only allow one version per suppression #29001

Merged
merged 7 commits into from
May 7, 2024
Merged

[TypeSpecRequirement] Only allow one version per suppression #29001

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d5ed94
Verify path, improve npm exec command
mikeharder May 7, 2024
4c02b60
Add comment
mikeharder May 7, 2024
6635b6d
Improve comment
mikeharder May 7, 2024
82dc507
Remove unnecessary semicolon
mikeharder May 7, 2024
fe1f36a
Add comment
mikeharder May 7, 2024
e531d30
Improve comment
mikeharder May 7, 2024
65fef97
Merge branch 'main' into tsr-suppress-specific-versions
mikeharder May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions eng/scripts/TypeSpec-Requirement.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,27 @@ function Get-Suppression {

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

Choose a reason for hiding this comment

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

Unrelated to crux of PR. npm exec --no -- is more predictable in scripts than npx and should always be used instead.

Copy link
Member

Choose a reason for hiding this comment

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

The TypeSpec Requirements pipeline was not calling the npm install step so this was never working after your change on friday. And the last error check you added below started to cause the pipeline to fail which was good because we started to see that failure now.

I've added the npm install step to the pipeline in PR #29008

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 expected $suppressions = npm exec to throw by default if the command returned a non-zero exit code. I was surprised we need to manually check $LASTEXITCODE.


return $suppressions ? $suppressions[0] : $null;
if ($LASTEXITCODE -ne 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to crux of PR. But if get-suppressions fails, either because user didn't run npm ci and tool isn't compiled, or because say suppressions.yaml is malformed and get-suppressions throws, we should fail fast.

LogError "Failure running 'npm exec get-suppressions'"
LogJobFailure
exit 1
}

$suppression = $suppressions ? $suppressions[0] : $null
mikeharder marked this conversation as resolved.
Show resolved Hide resolved

if ($suppression) {
$path = $suppression["path"]
$singleVersionPattern = "/(preview|stable)/[A-Za-z0-9._-]+/"
if ($path -notmatch $singleVersionPattern) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Crux of PR. Require that paths specified in suppressions.yaml for this tool only map to a single API version. Suppressions for additional versions should require additional entries and reviews.

LogError ("Invalid path '$path'. Path must only include one version per suppression.")
LogJobFailure
exit 1
}
}

return $suppression;
mikeharder marked this conversation as resolved.
Show resolved Hide resolved
}

$repoPath = Resolve-Path "$PSScriptRoot/../.."
Expand Down
Loading