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

[TypeSpec APIView] Ensure apiview only triggers on packages directly changed by the pr #31871

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
62 changes: 55 additions & 7 deletions eng/scripts/Create-APIView.ps1
Original file line number Diff line number Diff line change
@@ -86,6 +86,32 @@ function Get-ResourceProviderFromReadMePath {
return $null
}

function Get-ImpactedTypespecProjects {
Copy link
Member

Choose a reason for hiding this comment

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

Is this logic truly APIView specific? I would feel better if this was in the common functions and shared as I want to avoid different selecting algorithms for each tool that depends on finding typespec projects.

cc @mikeharder

param (
[Parameter(Mandatory = $true)]
[string]$TypeSpecFile
)
$filePathParts = $TypeSpecFile.split([IO.Path]::DirectorySeparatorChar)
while ($filePathParts.Length -and !$configFilesInTypeSpecProjects) {
$filePathParts = $filePathParts | Select-Object -SkipLast 1
$typeSpecProjectBaseDirectory = $filePathParts -join [IO.Path]::DirectorySeparatorChar
Copy link
Member

Choose a reason for hiding this comment

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

Split-Path makes pulling off the parents pretty simple. Split-Path -Parent

$configFilesInTypeSpecProjects = Get-ChildItem -Path $typeSpecProjectBaseDirectory -File "tspconfig.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

This will still be an issue. It is possible to have tspconfig.yaml in sub folders for a shared project. Current logic will stop looking further upwards and move to next block to check if current directory has main.tsp. It won't come back and check again the parent path of current file path if there is no main.tsp.

We can simplify this logic that will work for shared projects also.

for each changed files, strip the prefix before `specification/' from path:

  1. check if current path has tspconfig.yaml and main.tsp.
  2. If yes, add this folder to project set
  3. If no, then go upwards and continue step 2 until we match current path to 'specification/[^\/]+/+`

APIView needs to be generated from paths that contain both main.tsp and tspconfig.yaml (at least to retain same behavior as current one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I don't think I understand the exact situation where this might not work.
The code right now is checking all parent directory of the changed file till we reach root.
line 96 $filePathParts = $filePathParts | Select-Object -SkipLast 1 deletes the leaf directory on each iteration

}

if ($configFilesInTypeSpecProjects) {
foreach($configFilesInTypeSpecProject in $configFilesInTypeSpecProjects) {
$entryPointFile = Get-ChildItem -Path $($configFilesInTypeSpecProject.Directory.FullName) -File "main.tsp"
Copy link
Member

Choose a reason for hiding this comment

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

Why main.tsp? What about client.tsp?

if ($entryPointFile) {
Write-Host "Found $($configFilesInTypeSpecProject.Name) and $($entryPointFile.Name) in directory $($configFilesInTypeSpecProject.Directory.FullName)"
return $configFilesInTypeSpecProject.Directory.FullName
}
else {
Write-Host "Did not find main.tsp in directory $($configFilesInTypeSpecProject.Directory.FullName)"
}
}
}
}

<#
.DESCRIPTION
Invoke the swagger parset to generate APIView tokens.
@@ -363,15 +389,33 @@ function New-TypeSpecAPIViewTokens {
[string]$APIViewArtifactsDirectoryName
)

$SourceCommitId = $(git rev-parse HEAD^)
$TargetCommitId = $(git rev-parse HEAD)
$SourceCommitId = $(git rev-parse HEAD^2)
$TargetCommitId = $(git rev-parse HEAD^1)

LogInfo " Getting changed TypeSpec files in PR, between $SourceCommitId and $TargetCommitId"
$changedFiles = Get-ChangedFiles
$changedTypeSpecFiles = Get-ChangedTypeSpecFiles -changedFiles $changedFiles

$typeSpecProjects, $null = &"$PSScriptRoot/Get-TypeSpec-Folders.ps1" `
-IgnoreCoreFiles:$true `
-BaseCommitish:$SourceCommitId `
-TargetCommitish:$TargetCommitId
if ($changedTypeSpecFiles.Count -eq 0) {
LogWarning " There are no changes to TypeSpec files in the current PR..."
Write-Host "##vso[task.complete result=SucceededWithIssues;]DONE"
exit 0
}

$typeSpecProjects = $typeSpecProjects | Where-Object {Test-Path -Path "$_/main.tsp"}
LogGroupStart " Pullrequest has changes in these TypeSpec files..."
$changedTypeSpecFiles | ForEach-Object {
LogInfo " - $_"
}
LogGroupEnd

# Get impacted TypeSpec projects
$typeSpecProjects = [System.Collections.Generic.HashSet[string]]::new()
$changedTypeSpecFiles | ForEach-Object {
$tspProj = Get-ImpactedTypespecProjects -TypeSpecFile "$_"
if ($tspProj) {
$typeSpecProjects.Add($tspProj) | Out-Null
}
}

LogGroupStart " TypeSpec APIView Tokens will be generated for the following configuration files..."
$typeSpecProjects | ForEach-Object {
@@ -391,7 +435,9 @@ function New-TypeSpecAPIViewTokens {
git checkout $SourceCommitId
Write-Host "Installing required dependencies to generate New API review"
npm ci
LogGroupStart "npm ls -a"
npm ls -a
LogGroupEnd
foreach ($typeSpecProject in $typeSpecProjects) {
$tokenDirectory = [System.IO.Path]::Combine($typeSpecAPIViewArtifactsDirectory, $typeSpecProject.split([IO.Path]::DirectorySeparatorChar)[-1])
New-Item -ItemType Directory -Path $tokenDirectory -Force | Out-Null
@@ -402,7 +448,9 @@ function New-TypeSpecAPIViewTokens {
git checkout $TargetCommitId
Write-Host "Installing required dependencies to generate Baseline API review"
npm ci
LogGroupStart "npm ls -a"
npm ls -a
LogGroupEnd
foreach ($typeSpecProject in $typeSpecProjects) {
# Skip Baseline APIView Token for new projects
if (!(Test-Path -Path $typeSpecProject)) {
Original file line number Diff line number Diff line change
@@ -17,12 +17,6 @@ model Employee is TrackedResource<EmployeeProperties> {

/** Employee properties */
model EmployeeProperties {
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
model EmployeeProperties {
model EmployeeProperties {
/** Age of employee */
age?: int32;
/** City of employee */
city?: string;

Copy link
Member Author

Choose a reason for hiding this comment

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

temp removal for testing

/** Age of employee */
age?: int32;

/** City of employee */
city?: string;

/** Profile of employee */
@encode("base64url")
profile?: bytes;