Add pipeline to detect breaking change in swagger #1099
Conversation
|
No changes needing a change description found. |
|
You can try these changes at https://cadlplayground.z22.web.core.windows.net/typespec-azure/prs/1099/ Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/typespec-azure/prs/1099/ |
| #Step 2: Update swagger by TypeSpec in Step 1 | ||
| - checkout: azure-rest-api-specs | ||
| fetchDepth: 1 | ||
| - pwsh: | |
There was a problem hiding this comment.
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 | 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 |
There was a problem hiding this comment.
| - script: npm install | |
| - script: npm install --no-package-lock |
| submodules: true | ||
| - template: /eng/pipelines/templates/install.yml | ||
| parameters: | ||
| nodeVersion: "18.x" |
There was a problem hiding this comment.
| nodeVersion: "18.x" |
don't respecify here, this is otherwise an extgra place we'll need to upgrade, use the default
| TargetRepoName: azure-rest-api-specs | ||
| WorkingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs | ||
| ScriptDirectory: $(Build.SourcesDirectory)/azure-sdk-tools/eng/common/scripts | ||
| - task: PowerShell@2 |
There was a problem hiding this comment.
How is creating a PR going to help if everything was reverted here? What is the purpose?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
also because of the typespec-next branch I feel like this is just going to fail anyway if you run on main
There was a problem hiding this comment.
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:
- Developer does some change to typespec-azure and send out a PR with PR number like
abc. - Developer go to this pipeline UI and type in the target branch is
ref/pulls/abc/headorref/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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ok but if you test against main you will always have diff from previous PRs that might have had expected change as well
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Could you point me to typespec-next ci? Are you saying "running typespec-validation-all.yml against typespec-next branch"?
There was a problem hiding this comment.
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
| - script: npm install | ||
| displayName: Install updated TypeSpec dependencies | ||
| workingDirectory: $(Build.SourcesDirectory)/azure-rest-api-specs | ||
| - pwsh: ./eng/scripts/TypeSpec-Generate-Swagger.ps1 -CheckAll |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: Create pull request | ||
| inputs: | ||
| pwsh: true | ||
| filePath: $(Build.SourcesDirectory)/azure-sdk-tools/eng/common/scripts/Submit-PullRequest.ps1 |
There was a problem hiding this comment.
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?
| -RepoName "azure-rest-api-specs" | ||
| -BaseBranch "main" | ||
| -PROwner "azure-sdk" | ||
| -PRBranch "auto-update-swagger-$(Build.BuildNumber)" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_nameThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
|
@hallipr how does this related to the other validation pipelines you are doing for typespec and autorest? |
I don't need too many machines to regen for this case. I only need one. Therefore, I didn't publish any packages (which is a must in the other pipelines because they need parallel regen). I just simply clone typespec repo and azure-rest-api-specs repo. Let the versions in azure-rest-api-specs point to the local files of typepsec directly. I think this simplified process is difficult to fit into the pattern of other pipelines. |
It starts from 0.55 upgrade. We found that this upgrade removed the “readOnly: true” from “nextLink”, which causes breaking change in the generated SDK. Therefore, we are exploring some way to uncover such change as early as possible. This pipeline is what we prefer. We described to @allenjzhang in the TypeSpec sync meeting and he is supportive of it. |
This was a planned and approved breaking change, it was not a mistake, and it was detected by the existing typespec validation. I think the issue may be just communication - breaking changes are approved by the TypeSpec breaking change board and the release notes for each release contain details on changes. If there is some other mechanims that would be useful to you to be notified of these, we should explore. |
We want to ensure TypeSpec won't break generated swagger prior to its release. One solution is what this PR is doing: every time when the TypeSpec related change is in progress, we could run this pipeline to see if it impacts the existing swagger.
This script is a prerequisite because I call it to regen swaggers. I'm not calling existing TypeSpec-Validation.ps1 because I don't want to fail this script if there is any change, we want to see the diff.
After run this pipeline here, there will be a draft PR like this. Ignore the script added, we will not have this change after this merged.
How to run the pipeline: type in your PR head ref
