[draft] LintDiff Migration tests #32697
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
|
|
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
AzureRestAPISpecReview
added
data-plane
resource-manager
TypeSpec
openapi-pipeline-app
bot
added
PipelineBotTrigger
and removed
PipelineBotTrigger
labels
| @@ -0,0 +1,68 @@ | |||
| name: LintDiff (preview) | |||
There was a problem hiding this comment.
| name: LintDiff (preview) | |
| name: LintDiff (Preview) |
We have been capitalizing (Preview) in existing checks. I don't feel strongly about capital vs lowercase but I do want to be consistent.
| eng/ | ||
| .github/ | ||
|
|
||
| - name: Checkout 'after' state |
There was a problem hiding this comment.
We might be able to optimize the checkouts to only the folders with files changed in the PR diff.
| ref: ${{ github.event.pull_request.base.sha }} | ||
| path: before | ||
|
|
||
| - name: Setup Node and install deps |
There was a problem hiding this comment.
Add eng/tools/lint-diff to eng/tools/package.json, then it will be included transitively in the root package.json, so users (and CI) only needs to run npm ci at the repo root to install all tools.
|
|
||
| # TODO: This can probably be updated to run directly from JS | ||
| # TODO: default workspace is the after/ folder | ||
| - name: Get changed files |
There was a problem hiding this comment.
I will be creating JS-based helpers for this: #32637
| run: | | ||
| realpath ../../../before | ||
| realpath ../../../changed-files.txt | ||
| npm exec -- lint-diff --path ../../../before --changed-files-path ../../../changed-files.txt --out-file placeholder.txt |
There was a problem hiding this comment.
| npm exec -- lint-diff --path ../../../before --changed-files-path ../../../changed-files.txt --out-file placeholder.txt | |
| npm exec --no -- lint-diff --path ../../../before --changed-files-path ../../../changed-files.txt --out-file placeholder.txt |
Add param --no to ensure only pre-installed packages are used.
|
|
||
| - name: Run LintDiff After | ||
| run: | | ||
| npm exec -- lint-diff --path ../../../../after --changed-files-path ../../../../changed-files.txt --out-file placeholder.txt |
There was a problem hiding this comment.
| npm exec -- lint-diff --path ../../../../after --changed-files-path ../../../../changed-files.txt --out-file placeholder.txt | |
| npm exec --no -- lint-diff --path ../../../../after --changed-files-path ../../../../changed-files.txt --out-file placeholder.txt |
There was a problem hiding this comment.
I'm not sure if we will need this tool long-term. To me this is more "glue code" that is used by CI, rather than a tool for dev inner-loop. But it's fine for now since this is likely the simplest way to lift-and-shift the existing check.
| type: "string", | ||
| short: "o", | ||
| }, | ||
| "lint-version": { |
There was a problem hiding this comment.
This parameter should be removed, and instead all necessary dependencies should be listed in a package.json. If the dependencies are to be called directly during dev inner-loop, they should be listed in the root package.json. Else, use the package,json of the tool that calls the dependencies.
There was a problem hiding this comment.
Also, autorest is special because, by default, it uses custom code to manage it's own dependendencies. However, it can be convinced to use only pre-installed deps. We will want to use this here, and I'll extract it into a common JS helper.
Here's me making this fix to tsp-client:
| } | ||
| } | ||
|
|
||
| async function executeCommand( |
There was a problem hiding this comment.
I will extract this into a shared helper.
| * TODO: TESTS | ||
| * @param changedFiles List of changed files. | ||
| */ | ||
| async function getAffectedReadmeFiles(changedFiles: string[], repoRoot: string): Promise<string[]> { |
There was a problem hiding this comment.
Might be worth making the rest of the file shared code, if we think other tools may want similar file lists.
There was a problem hiding this comment.
This should probably also be shared code
| * @param path Path to check for existence | ||
| * @returns true if the path exists, false otherwise | ||
| */ | ||
| export async function pathExists(path: string): Promise<boolean> { |
Test a few scenarios
--openapi-typewhen editing indata-plane/--openapi-typewhen editing inresource-manager/autorestover all tags detected in a readme file but should instead find no tags affected because the new file is not referenced in any readme.md