diff --git a/.github/shared/src/changed-files.js b/.github/shared/src/changed-files.js index cab93e7e2199..5641a95e64ed 100644 --- a/.github/shared/src/changed-files.js +++ b/.github/shared/src/changed-files.js @@ -2,6 +2,7 @@ import debug from "debug"; import { simpleGit } from "simple-git"; +import { includesFolder } from "./path.js"; // Enable simple-git debug logging to improve console output debug.enable("simple-git"); @@ -12,17 +13,23 @@ debug.enable("simple-git"); * @param {string} [options.cwd] Current working directory. Default: process.cwd(). * @param {string} [options.headCommitish] Default: "HEAD". * @param {import('./logger.js').ILogger} [options.logger] - * @returns {Promise} List of changed files, using posix paths, relative to options.cwd. Example: ["specification/foo/Microsoft.Foo/main.tsp"]. + * @param {string[]} [options.paths] Limits the diff to the named paths. If not set, includes all paths in repo. Default: [] + * @returns {Promise} List of changed files, using posix paths, relative to repo root. Example: ["specification/foo/Microsoft.Foo/main.tsp"]. */ export async function getChangedFiles(options = {}) { - const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger } = options; + const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger, paths = [] } = options; + + if (paths.length > 0) { + // Use "--" to separate paths from revisions + paths.unshift("--"); + } // TODO: If we need to filter based on status, instead of passing an argument to `--diff-filter, // consider using "--name-status" instead of "--name-only", and return an array of objects like // { name: "/foo/baz.js", status: Status.Renamed, previousName: "/foo/bar.js"}. // Then add filter functions to filter based on status. This is more flexible and lets consumers // filter based on status with a single call to `git diff`. - const result = await simpleGit(cwd).diff(["--name-only", baseCommitish, headCommitish]); + const result = await simpleGit(cwd).diff(["--name-only", baseCommitish, headCommitish, ...paths]); const files = result.trim().split("\n"); @@ -41,11 +48,23 @@ export async function getChangedFiles(options = {}) { * @param {string} [options.cwd] Current working directory. Default: process.cwd(). * @param {string} [options.headCommitish] Default: "HEAD". * @param {import('./logger.js').ILogger} [options.logger] + * @param {string[]} [options.paths] Limits the diff to the named paths. If not set, includes all paths in repo. Default: [] * @returns {Promise<{additions: string[], modifications: string[], deletions: string[], renames: {from: string, to: string}[], total: number}>} */ export async function getChangedFilesStatuses(options = {}) { - const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger } = options; - const result = await simpleGit(cwd).diff(["--name-status", baseCommitish, headCommitish]); + const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger, paths = [] } = options; + + if (paths.length > 0) { + // Use "--" to separate paths from revisions + paths.unshift("--"); + } + + const result = await simpleGit(cwd).diff([ + "--name-status", + baseCommitish, + headCommitish, + ...paths, + ]); const categorizedFiles = { additions: /** @type {string[]} */ ([]), @@ -133,6 +152,7 @@ export async function getChangedFilesStatuses(options = {}) { } // Functions suitable for passing to string[].filter(), ordered roughly in order of increasing specificity +// Functions accept both relative and absolute paths, since paths are resolve()'d before searching (when needed) /** * @param {string} [file] @@ -152,22 +172,13 @@ export function readme(file) { return typeof file === "string" && file.toLowerCase().endsWith("readme.md"); } -/** - * @param {string} [file] - * @returns {boolean} - */ -export function specification(file) { - // Folder name "specification" should match case, since it already exists in repo - return typeof file === "string" && file.startsWith("specification/"); -} - /** * @param {string} [file] * @returns {boolean} */ export function dataPlane(file) { // Folder name "data-plane" should match case for consistency across specs - return typeof file === "string" && specification(file) && file.includes("/data-plane/"); + return typeof file === "string" && includesFolder(file, "data-plane"); } /** @@ -176,7 +187,7 @@ export function dataPlane(file) { */ export function resourceManager(file) { // Folder name "resource-manager" should match case for consistency across specs - return typeof file === "string" && specification(file) && file.includes("/resource-manager/"); + return typeof file === "string" && includesFolder(file, "resource-manager"); } /** @@ -185,9 +196,7 @@ export function resourceManager(file) { */ export function example(file) { // Folder name "examples" should match case for consistency across specs - return ( - typeof file === "string" && json(file) && specification(file) && file.includes("/examples/") - ); + return typeof file === "string" && json(file) && includesFolder(file, "examples"); } /** @@ -197,8 +206,7 @@ export function example(file) { export function typespec(file) { return ( typeof file === "string" && - (file.toLowerCase().endsWith(".tsp") || file.toLowerCase().endsWith("tspconfig.yaml")) && - specification(file) + (file.toLowerCase().endsWith(".tsp") || file.toLowerCase().endsWith("tspconfig.yaml")) ); } @@ -221,7 +229,5 @@ export function swagger(file) { * @returns {boolean} */ export function scenario(file) { - return ( - typeof file === "string" && json(file) && specification(file) && file.includes("/scenarios/") - ); + return typeof file === "string" && json(file) && includesFolder(file, "scenarios"); } diff --git a/.github/shared/src/swagger.js b/.github/shared/src/swagger.js index 8532c6cd9787..0e204c4a613a 100644 --- a/.github/shared/src/swagger.js +++ b/.github/shared/src/swagger.js @@ -4,7 +4,7 @@ import $RefParser, { ResolverError } from "@apidevtools/json-schema-ref-parser"; import { readFile } from "fs/promises"; import { dirname, relative, resolve } from "path"; import { mapAsync } from "./array.js"; -import { includesFolder } from "./path.js"; +import { example } from "./changed-files.js"; import { SpecModelError } from "./spec-model-error.js"; /** @@ -222,26 +222,6 @@ export class Swagger { } } -// TODO: Remove duplication with changed-files.js (which currently requires paths relative to repo root) - -/** - * @param {string} [file] - * @returns {boolean} - */ -function example(file) { - // Folder name "examples" should match case for consistency across specs - return typeof file === "string" && json(file) && includesFolder(file, "examples"); -} - -/** - * @param {string} [file] - * @returns {boolean} - */ -function json(file) { - // Extension "json" with any case is a valid JSON file - return typeof file === "string" && file.toLowerCase().endsWith(".json"); -} - // API version lifecycle stages export const API_VERSION_LIFECYCLE_STAGES = Object.freeze({ PREVIEW: "preview", diff --git a/.github/shared/test/changed-files.test.js b/.github/shared/test/changed-files.test.js index 7ecf778a0740..92bd84b827db 100644 --- a/.github/shared/test/changed-files.test.js +++ b/.github/shared/test/changed-files.test.js @@ -8,6 +8,7 @@ vi.mock("simple-git", () => ({ }), })); +import { resolve } from "path"; import * as simpleGit from "simple-git"; import { dataPlane, @@ -18,7 +19,6 @@ import { readme, resourceManager, scenario, - specification, swagger, typespec, } from "../src/changed-files.js"; @@ -47,6 +47,13 @@ describe("changedFiles", () => { "cspell.yaml", "MixedCase.jSoN", "README.MD", + "not-spec/contosowidgetmanager/data-plane/readme.md", + "not-spec/contosowidgetmanager/resource-manager/readme.md", + "not-spec/contosowidgetmanager/Contoso.Management/main.tsp", + "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", + "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", + "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/data-plane/readme.md", "specification/contosowidgetmanager/Contoso.Management/main.tsp", "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", @@ -57,10 +64,15 @@ describe("changedFiles", () => { "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", ]; + const filesResolved = files.map((f) => resolve(f)); + it("filter:json", () => { const expected = [ "cspell.json", "MixedCase.jSoN", + "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", + "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", + "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json", @@ -68,35 +80,26 @@ describe("changedFiles", () => { ]; expect(files.filter(json)).toEqual(expected); + expect(filesResolved.filter(json)).toEqual(expected.map((f) => resolve(f))); }); it("filter:readme", () => { const expected = [ "README.MD", + "not-spec/contosowidgetmanager/data-plane/readme.md", + "not-spec/contosowidgetmanager/resource-manager/readme.md", "specification/contosowidgetmanager/data-plane/readme.md", "specification/contosowidgetmanager/resource-manager/readme.md", ]; expect(files.filter(readme)).toEqual(expected); - }); - - it("filter:specification", () => { - const expected = [ - "specification/contosowidgetmanager/data-plane/readme.md", - "specification/contosowidgetmanager/Contoso.Management/main.tsp", - "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", - "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", - "specification/contosowidgetmanager/resource-manager/readme.md", - "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", - "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json", - "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", - ]; - - expect(files.filter(specification)).toEqual(expected); + expect(filesResolved.filter(readme)).toEqual(expected.map((f) => resolve(f))); }); it("filter:typespec", () => { const expected = [ + "not-spec/contosowidgetmanager/Contoso.Management/main.tsp", + "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml", "specification/contosowidgetmanager/Contoso.Management/main.tsp", "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", ]; @@ -104,44 +107,57 @@ describe("changedFiles", () => { }); it("filter:data-plane", () => { - const expected = ["specification/contosowidgetmanager/data-plane/readme.md"]; + const expected = [ + "not-spec/contosowidgetmanager/data-plane/readme.md", + "specification/contosowidgetmanager/data-plane/readme.md", + ]; expect(files.filter(dataPlane)).toEqual(expected); + expect(filesResolved.filter(dataPlane)).toEqual(expected.map((f) => resolve(f))); }); it("filter:resource-manager", () => { const expected = [ + "not-spec/contosowidgetmanager/resource-manager/readme.md", + "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/resource-manager/readme.md", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json", ]; expect(files.filter(resourceManager)).toEqual(expected); + expect(filesResolved.filter(resourceManager)).toEqual(expected.map((f) => resolve(f))); }); it("filter:example", () => { const expected = [ + "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json", ]; expect(files.filter(example)).toEqual(expected); + expect(filesResolved.filter(example)).toEqual(expected.map((f) => resolve(f))); }); it("filter:scenarios", () => { const expected = [ + "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json", ]; expect(files.filter(scenario)).toEqual(expected); + expect(filesResolved.filter(scenario)).toEqual(expected.map((f) => resolve(f))); }); it("filter:swagger", () => { const expected = [ + "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json", ]; expect(files.filter(swagger)).toEqual(expected); + expect(filesResolved.filter(swagger)).toEqual(expected.map((f) => resolve(f))); }); describe("getChangedFilesStatuses", () => { diff --git a/.github/workflows/lintdiff-code.yaml b/.github/workflows/lintdiff-code.yaml index e8307a9ad6d2..11e0e9c187df 100644 --- a/.github/workflows/lintdiff-code.yaml +++ b/.github/workflows/lintdiff-code.yaml @@ -42,7 +42,7 @@ jobs: const { join } = await import('path'); // TODO: Logger - const changedFiles = await getChangedFiles({ cwd: 'after'}); + const changedFiles = await getChangedFiles({ cwd: 'after', paths: ['specification'] }); console.log('Changed files:', changedFiles); const filePath = join(process.cwd(), 'changed-files.txt'); diff --git a/.github/workflows/src/arm-incremental-typespec.js b/.github/workflows/src/arm-incremental-typespec.js index e71619e7d297..14cd9173090a 100644 --- a/.github/workflows/src/arm-incremental-typespec.js +++ b/.github/workflows/src/arm-incremental-typespec.js @@ -24,6 +24,7 @@ debug.enable("simple-git"); export default async function incrementalTypeSpec({ core }) { const options = { cwd: process.env.GITHUB_WORKSPACE, + paths: ["specification"], logger: new CoreLogger(core), }; diff --git a/eng/tools/lint-diff/src/processChanges.ts b/eng/tools/lint-diff/src/processChanges.ts index c57bec9648b2..447c06f8ba4c 100644 --- a/eng/tools/lint-diff/src/processChanges.ts +++ b/eng/tools/lint-diff/src/processChanges.ts @@ -1,7 +1,7 @@ import { join, relative, resolve, sep } from "path"; import { readFile } from "fs/promises"; import { pathExists } from "./util.js"; -import { specification, readme, swagger } from "@azure-tools/specs-shared/changed-files"; +import { readme, swagger } from "@azure-tools/specs-shared/changed-files"; import { SpecModel } from "@azure-tools/specs-shared/spec-model"; import { ReadmeAffectedTags } from "./lintdiff-types.js"; import deepEqual from "deep-eql"; @@ -19,12 +19,9 @@ export async function getRunList( // Read changed files, exclude any files that should be ignored const ignoreFilesWith = ["/examples/", "/quickstart-templates/", "/scenarios/"]; - const changedSpecFiles = (await readFileList(changedFilesPath)).filter((file) => { - // File is in specification/ folder - if (!specification(file)) { - return false; - } + // Changed files should already be filtered to the top-level "specification" folder (see lintdiff-code.yaml) + const changedSpecFiles = (await readFileList(changedFilesPath)).filter((file) => { // File is not ignored for (const ignore of ignoreFilesWith) { if (file.includes(ignore)) { diff --git a/eng/tools/oav-runner/src/runner.ts b/eng/tools/oav-runner/src/runner.ts index a9c4e191a57f..1e396bc6d309 100644 --- a/eng/tools/oav-runner/src/runner.ts +++ b/eng/tools/oav-runner/src/runner.ts @@ -4,16 +4,16 @@ import * as oav from "oav"; import * as path from "path"; import * as fs from "fs"; +import { example, getChangedFiles, swagger } from "@azure-tools/specs-shared/changed-files"; //getChangedFiles, import { Swagger } from "@azure-tools/specs-shared/swagger"; -import { includesFolder } from "@azure-tools/specs-shared/path"; -import { getChangedFiles } from "@azure-tools/specs-shared/changed-files"; //getChangedFiles, import { ReportableOavError } from "./formatting.js"; export async function preCheckFiltering( rootDirectory: string, fileList?: string[], ): Promise { - const changedFiles = fileList ?? (await getChangedFiles({ cwd: rootDirectory })); + const changedFiles = + fileList ?? (await getChangedFiles({ cwd: rootDirectory, paths: ["specification"] })); const swaggerFiles = await processFilesToSpecificationList(rootDirectory, changedFiles); @@ -125,24 +125,6 @@ async function getFiles(rootDirectory: string, directory: string): Promise d.includes("specification" + path.sep)); } -function example(file: string): boolean { - return ( - typeof file === "string" && - file.toLowerCase().endsWith(".json") && - includesFolder(file, "examples") - ); -} - -function swagger(file: string): boolean { - return ( - typeof file === "string" && - file.toLowerCase().endsWith(".json") && - (includesFolder(file, "data-plane") || includesFolder(file, "resource-manager")) && - includesFolder(file, "specification") && - !includesFolder(file, "examples") - ); -} - export async function processFilesToSpecificationList( rootDirectory: string, files: string[], @@ -154,10 +136,6 @@ export async function processFilesToSpecificationList( // files from get-changed-files are relative to the root of the repo, // though that context is passed into this from cli arguments. for (const file of files) { - if (!file.startsWith("specification/")) { - continue; - } - const absoluteFilePath = path.join(rootDirectory, file); // if the file is an example, we need to find the swagger file that references it diff --git a/eng/tools/openapi-diff-runner/src/command-helpers.ts b/eng/tools/openapi-diff-runner/src/command-helpers.ts index d3d45c17d78e..fbad6d1a7362 100644 --- a/eng/tools/openapi-diff-runner/src/command-helpers.ts +++ b/eng/tools/openapi-diff-runner/src/command-helpers.ts @@ -133,6 +133,7 @@ export async function getSwaggerDiffs( baseCommitish: options.baseCommitish, cwd: options.cwd, headCommitish: options.headCommitish, + paths: ["specification"], }); // Filter each array to only include Swagger files using the swagger filter from changed-files.js diff --git a/eng/tools/openapi-diff-runner/test/command-helpers.test.ts b/eng/tools/openapi-diff-runner/test/command-helpers.test.ts index da40bb8c4819..50a3d1228838 100644 --- a/eng/tools/openapi-diff-runner/test/command-helpers.test.ts +++ b/eng/tools/openapi-diff-runner/test/command-helpers.test.ts @@ -1,6 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { existsSync, mkdirSync, readFileSync, writeFileSync, rmSync } from "node:fs"; -import path from "node:path"; import { fileURLToPath } from "node:url"; import { createContextFromParsedArgs, @@ -186,7 +185,6 @@ function setupCommonMocks() { // Mock dependencies vi.mock("node:fs"); -vi.mock("node:path"); vi.mock("node:url"); vi.mock("../src/utils/common-utils.js"); vi.mock("../src/utils/oad-message-processor.js"); @@ -206,7 +204,6 @@ describe("command-helpers", () => { const mockReadFileSync = vi.mocked(readFileSync); const mockWriteFileSync = vi.mocked(writeFileSync); const mockRmSync = vi.mocked(rmSync); - const mockPath = vi.mocked(path); const mockFileURLToPath = vi.mocked(fileURLToPath); const mockGetChangedFilesStatuses = vi.mocked(getChangedFilesStatuses); @@ -214,9 +211,6 @@ describe("command-helpers", () => { vi.clearAllMocks(); // Setup default mocks - mockPath.resolve.mockImplementation((...paths) => paths.join("/")); - mockPath.join.mockImplementation((...paths) => paths.join("/")); - mockPath.dirname.mockImplementation((p) => p.split("/").slice(0, -1).join("/")); mockFileURLToPath.mockReturnValue("/path/to/file.js"); // Mock console methods to avoid test output noise @@ -370,6 +364,7 @@ describe("command-helpers", () => { baseCommitish: TEST_CONSTANTS.BRANCHES.MAIN, cwd: TEST_CONSTANTS.PATHS.TEST_PATH, headCommitish: TEST_CONSTANTS.COMMITS.HEAD, + paths: ["specification"], }); }); @@ -451,6 +446,7 @@ describe("command-helpers", () => { baseCommitish: undefined, cwd: undefined, headCommitish: undefined, + paths: ["specification"], }); }); diff --git a/eng/tools/sdk-suppressions/src/common.ts b/eng/tools/sdk-suppressions/src/common.ts index 8fb57b0d62eb..6d6120fa0e2f 100644 --- a/eng/tools/sdk-suppressions/src/common.ts +++ b/eng/tools/sdk-suppressions/src/common.ts @@ -6,7 +6,7 @@ import { getChangedFiles } from "@azure-tools/specs-shared/changed-files"; * @description get the changed files in the current PR */ export async function getSDKSuppressionsChangedFiles() { - const changedFiles = await getChangedFiles(); + const changedFiles = await getChangedFiles({ paths: ["specification"] }); const sdkSuppressionsFiles = changedFiles.filter((file) => file.endsWith("sdk-suppressions.yaml"), ); diff --git a/eng/tools/summarize-impact/src/PRContext.ts b/eng/tools/summarize-impact/src/PRContext.ts index 1038892c9724..b2dbab5df102 100644 --- a/eng/tools/summarize-impact/src/PRContext.ts +++ b/eng/tools/summarize-impact/src/PRContext.ts @@ -4,6 +4,7 @@ import * as fs from "fs"; import { parseMarkdown } from "@azure-tools/openapi-tools-common"; import * as amd from "@azure/openapi-markdown"; +import { includesFolder } from "@azure-tools/specs-shared/path"; import { SpecModel } from "@azure-tools/specs-shared/spec-model"; import { Readme } from "@azure-tools/specs-shared/readme"; import { @@ -11,7 +12,6 @@ import { typespec, example, readme, - specification, } from "@azure-tools/specs-shared/changed-files"; import { LabelContext } from "./labelling-types.js"; @@ -168,7 +168,7 @@ export class PRContext { if (visitedFolder.has(dir)) { return; } - while (specification(dir)) { + while (includesFolder(dir, "specification")) { if (visitedFolder.has(dir)) { break; } diff --git a/eng/tools/summarize-impact/src/cli.ts b/eng/tools/summarize-impact/src/cli.ts index 75641b3b6093..c939311a3c42 100644 --- a/eng/tools/summarize-impact/src/cli.ts +++ b/eng/tools/summarize-impact/src/cli.ts @@ -92,7 +92,7 @@ export async function main() { const targetDirectory = opts.targetDirectory as string; const sourceGitRoot = await getRoot(sourceDirectory); const targetGitRoot = await getRoot(targetDirectory); - const fileList = await getChangedFilesStatuses({ cwd: sourceGitRoot }); + const fileList = await getChangedFilesStatuses({ cwd: sourceGitRoot, paths: ["specification"] }); const sha = opts.sha as string; const sourceBranch = opts.sourceBranch as string; const targetBranch = opts.targetBranch as string;