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

Jump to bottom

Add pipeline to detect breaking change in swagger #1099

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Add pipeline to detect breaking change in swagger #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fd5a41c
Add pipeline to detect breaking change in swagger
pshao25 Jul 1, 2024
f017288
update
pshao25 Jul 2, 2024
6b9f144
update
pshao25 Jul 2, 2024
dc7559b
update
pshao25 Jul 2, 2024
3a354c8
update
pshao25 Jul 2, 2024
3ed778a
update
pshao25 Jul 2, 2024
6d0b364
update
pshao25 Jul 2, 2024
434c832
update
pshao25 Jul 2, 2024
6840f27
update
pshao25 Jul 2, 2024
5c8df0f
update
pshao25 Jul 2, 2024
9c52bdd
Merge branch 'main' of https://github.com/Azure/typespec-azure into p…
pshao25 Jul 2, 2024
68bd3da
update
Jul 3, 2024
8c03741
update
Jul 3, 2024
4307a0f
update
Jul 3, 2024
ba7e56c
update
Jul 3, 2024
7ae44ff
update
Jul 3, 2024
f7a9468
update
Jul 3, 2024
0f9d1e4
update
Jul 3, 2024
231ff66
update
Jul 4, 2024
7fe237f
Merge branch 'main' into pipelineForSwagger
pshao25 Jul 4, 2024
77d2d50
update
Jul 4, 2024
c0be09a
update
Jul 4, 2024
5717d70
update
Jul 4, 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
94 changes: 94 additions & 0 deletions eng/pipelines/pr-breaking-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
trigger: none

resources:
repositories:
- repository: azure-rest-api-specs
type: github
name: Azure/azure-rest-api-specs
endpoint: azure
- repository: azure-sdk-tools
type: github
name: Azure/azure-sdk-tools
endpoint: azure

MaryGao marked this conversation as resolved.
Show resolved Hide resolved
stages:
- stage: Build_and_Validate
jobs:
- job: Validate_generated_swagger
pool:
name: azsdk-pool-mms-ubuntu-2004-general
vmImage: ubuntu-20.04
steps:
# Step 1: Build TypeSpec
- checkout: self
submodules: true
- template: /eng/pipelines/templates/install.yml
parameters:
nodeVersion: "18.x"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nodeVersion: "18.x"

don't respecify here, this is otherwise an extgra place we'll need to upgrade, use the default

workingDirectory: $(Build.SourcesDirectory)/typespec-azure
- script: pnpm run build
displayName: Build
workingDirectory: $(Build.SourcesDirectory)/typespec-azure

#Step 2: Update swagger by TypeSpec in Step 1
- checkout: azure-rest-api-specs
fetchDepth: 1
- pwsh: |
Copy link
Member

Choose a reason for hiding this comment

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

We already have this logic for the cadl-ranch e2e test, could we reuse that instead of having to maintain an extra place and deal with powershell

$packageJson = Get-Content -Raw "package.json" | ConvertFrom-Json -AsHashtable

$packageJson.devDependencies["@azure-tools/typespec-autorest"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-autorest"
$packageJson.devDependencies["@azure-tools/typespec-azure-core"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-azure-core"
$packageJson.devDependencies["@azure-tools/typespec-azure-portal-core"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-azure-portal-core"
$packageJson.devDependencies["@azure-tools/typespec-azure-resource-manager"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-azure-resource-manager"
$packageJson.devDependencies["@azure-tools/typespec-client-generator-core"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-client-generator-core"
$packageJson.devDependencies["@azure-tools/typespec-azure-rulesets"] = "file:$(Build.SourcesDirectory)/typespec-azure/packages/typespec-azure-rulesets"
$packageJson.devDependencies["@typespec/compiler"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/compiler"
$packageJson.devDependencies["@typespec/http"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/http"
$packageJson.devDependencies["@typespec/openapi"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/openapi"
$packageJson.devDependencies["@typespec/openapi3"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/openapi3"
$packageJson.devDependencies["@typespec/rest"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/rest"
$packageJson.devDependencies["@typespec/versioning"] = "file:$(Build.SourcesDirectory)/typespec-azure/core/packages/versioning"

$packageJson | ConvertTo-Json -Depth 100 | Out-File -Path "package.json" -Encoding utf8 -NoNewline -Force
displayName: "Update package.json in azure-rest-api-specs"
workingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs
- script: npm install
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- script: npm install
- script: npm install --no-package-lock

displayName: Install updated TypeSpec dependencies
workingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs
- pwsh: ./eng/scripts/TypeSpec-Generate-Swagger.ps1 -CheckAll
Copy link
Member

Choose a reason for hiding this comment

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

Where is this script comming from I don't see it in azure-rest-api-specs.

TypeSpec-Validation.ps1 is the one that does the regen and checks

Copy link
Member Author

@pshao25 pshao25 Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I put details in the description that it comes from this PR.

TypeSpec-Validation.ps1 would throw error if there is any change, which is not we are expecting. We hope to get the diff.

displayName: "Generate all swagger files"
workingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs
- pwsh: |
git restore package.json
git restore package-lock.json
displayName: Discard versions change
workingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs

# Step 3: Create PR
- checkout: azure-sdk-tools
fetchDepth: 1
- template: /eng/common/pipelines/templates/steps/git-push-changes.yml@azure-sdk-tools
parameters:
BaseRepoBranch: auto-update-swagger-$(Build.BuildNumber)
BaseRepoOwner: azure-sdk
CommitMsg: Update swagger with TypeSpec built from $(Build.BuildNumber)
TargetRepoOwner: Azure
TargetRepoName: azure-rest-api-specs
WorkingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs
ScriptDirectory: $(Build.SourcesDirectory)/azure-sdk-tools/eng/common/scripts
- task: PowerShell@2
Copy link
Member

Choose a reason for hiding this comment

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

How is creating a PR going to help if everything was reverted here? What is the purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

What you mean by "everything was reverted here"? I only revert package.json and package-lock.json, which is not what we care about. We care about swagger change. An example is here.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so from experience with the spec repo most of the change we need to apply are typespec changes in the typespec-next branch so wondering if this just does a small portion of the solution

Copy link
Member

@timotheeguerin timotheeguerin Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

also because of the typespec-next branch I feel like this is just going to fail anyway if you run on main

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 didn't find the typespec-next branch you are saying. Could you point me to it?

I want to clarify the user story here:

  1. Developer does some change to typespec-azure and send out a PR with PR number like abc.
  2. Developer go to this pipeline UI and type in the target branch is ref/pulls/abc/head or ref/pulls/abc/merge. Then run. You could see the screenshot in the description.

Therefore, our target is WIP change, instead of the code already merged into main.

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. I understand it is very possible (even maybe 100%) we expect to change tsp before each release. But our intention is to uncover all the unexpected change in swagger, which might come from bug or lack of consideration. Every time you are not confident in your change, you could run the pipeline before merge. The granularity is for each RP. We might expect tsp change for each release, but we won't expect tsp change for each PR, right?

Copy link
Member

Choose a reason for hiding this comment

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

Ok but if you test against main you will always have diff from previous PRs that might have had expected change as well

Copy link
Member

Choose a reason for hiding this comment

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

I feel like we get this protection from the azure-rest-api-specs typespec-next ci, which automatically takes up the developer versions and validates them against typespec-next.

It would be nice to have a more automated way to check a particular PR against typespec-next directly

Copy link
Member Author

@pshao25 pshao25 Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you point me to typespec-next ci? Are you saying "running typespec-validation-all.yml against typespec-next branch"?

Copy link
Member

@timotheeguerin timotheeguerin Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is just the TypeSpecValidation - All pipeline from teh spec repo that is run in the typespec-next branch on every commit as well as as soon as one of the typespec and typespec azure pipeline finish https://dev.azure.com/azure-sdk/public/_build?definitionId=6143&_a=summary

displayName: Create pull request
inputs:
pwsh: true
filePath: $(Build.SourcesDirectory)/azure-sdk-tools/eng/common/scripts/Submit-PullRequest.ps1
Copy link
Member

Choose a reason for hiding this comment

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

Why are we creating these PR's against the spec repo? We don't wish to start creating a lot of PRs. Are there other ways we can get the validation we are after?

arguments: >
-RepoOwner "Azure"
-RepoName "azure-rest-api-specs"
-BaseBranch "main"
-PROwner "azure-sdk"
-PRBranch "auto-update-swagger-$(Build.BuildNumber)"
Copy link
Member

Choose a reason for hiding this comment

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

This naming pattern for a branch will create a branch/PR per build in the specs repo which is something we should definitely avoid, that will likely cause way too much noise.

Copy link
Member

Choose a reason for hiding this comment

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

In the typespec preview pipelines, we set the branch name early in the build via inline powershell. We attempt to reuse the same PR branch name for the same source PR, scheduled runs use a single branch name and all other runs will use a unique dynamic name:

azure-sdk-tools / archetype-autorest-preview.yml

    - pwsh: |
        $sourceBranch = '$(Build.SourceBranch)'
        $buildReason = '$(Build.Reason)'
        $buildNumber = '$(Build.BuildNumber)'

        if ($buildReason -eq 'Schedule') {
          $branchName = 'auto-update-autorest-scheduled'
        } elseif ($sourceBranch -match "^refs/pull/(\d+)/(head|merge)$") {
          $branchName = "auto-update-autorest-pr-$($Matches[1])"
        } else {
          $branchName = "auto-update-autorest-$buildNumber"
        }

        Write-Host "Setting variable 'branchName' to '$branchName'"
        Write-Host "##vso[task.setvariable variable=branchName;isOutput=true]$branchName"
      displayName: Set branch name
      name: set_branch_name

Copy link
Member

@weshaggard weshaggard Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That would definitely be better if we need a PR. However, I'd still like to understand if a PR into the specs repo is needed to accomplish the testing we are looking for. If it is needed who will be responsible for monitoring it and closing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to have a visualized way to see the diff. We could reduce the number by the way Patrick put above. Or maybe we could even put the PRs in azure-rest-api-specs-pr if your concern is you don't want to confuse customers.

If it is needed who will be responsible for monitoring it and closing it.

@hallipr do you know how it is monitored in azure-sdk-for-net?

Copy link
Member

Choose a reason for hiding this comment

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

You can have a visual change by pushing to a branch without needing to create a PR. Creating a PR causes a lot of other things to kick off that we should avoid if they aren't needed.

Noise inside of the private repo is still a concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about we just create a closed PR? It sounds really wired to me that pushing to a branch which is almost 100% going to create a PR.

Copy link
Member

Choose a reason for hiding this comment

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

@pshao25 we can compose the URL to view the difference between two branches like this https://github.com/[username]/[repository]/compare/[base-branch]...[compare-branch] instead of creating PRs.

Copy link
Member

Choose a reason for hiding this comment

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

If the purpose of the branch (or pr) is to use github's UI to visualize a diff, I like Ray's suggestion to just use a ref comparison instead of a PR. We could even add that compare URL to the pipeline output so it's clickable from the ADO UI

-AuthToken "$(azuresdk-github-pat)"
-PRTitle "Swagger Regen Preview from TypeSpec built from $(Build.BuildNumber)"
-PRBody "Swagger Regen Preview from TypeSpec built from $(Build.BuildNumber)"
-OpenAsDraft $true
-PRLabels "Do Not Merge"
7 changes: 6 additions & 1 deletion eng/pipelines/templates/install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ parameters:
- name: pnpmStorePath
type: string
default: $(Pipeline.Workspace)/.pnpm-store
- name: workingDirectory
type: string
default: $(Build.SourcesDirectory)
steps:
- task: UseDotNet@2
inputs:
Expand All @@ -19,7 +22,7 @@ steps:

- task: Cache@2
inputs:
key: 'pnpm | "$(Agent.OS)" | pnpm-lock.yaml'
key: 'pnpm | "$(Agent.OS)" | ${{ parameters.workingDirectory }}/pnpm-lock.yaml'
path: ${{ parameters.pnpmStorePath }}
displayName: Cache pnpm store
- script: |
Expand All @@ -40,6 +43,8 @@ steps:
- script: pnpm install
displayName: Install JavaScript Dependencies
retryCountOnTaskFailure: 3
workingDirectory: ${{ parameters.workingDirectory }}

- script: dotnet restore core/packages/typespec-vs
displayName: Restore .NET Dependencies
workingDirectory: ${{ parameters.workingDirectory }}
Loading