From 829e8cd9dacc0692fbead02451cfcbbfd745e9b7 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Dec 2023 15:29:58 +0100 Subject: [PATCH 1/7] test(git): re-enable failing tests --- core/test/unit/src/vcs/git.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index aa17db8453..67616b1934 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -441,11 +441,11 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { for (const testParam of testParams) { it(testParam.name, async () => { // FIXME - if (handler.name === "git-repo") { - if (testParam.name === "without globs") { - return - } - } + // if (handler.name === "git-repo") { + // if (testParam.name === "without globs") { + // return + // } + // } // matches file exclusion pattern -> should be excluded const excludedByFilename = resolve(tmpPath, "foo.txt") @@ -518,11 +518,11 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { for (const testParam of testParams) { it(testParam.name, async () => { // FIXME - if (handler.name === "git-repo") { - if (testParam.name === "with prefix globs") { - return - } - } + // if (handler.name === "git-repo") { + // if (testParam.name === "with prefix globs") { + // return + // } + // } // doesn't match file exclusion pattern -> should be included const notExcludedByFilename = resolve(tmpPath, "bar.txt") @@ -596,11 +596,11 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { for (const testParam of testParams) { it(testParam.name, async () => { // FIXME - if (handler.name === "git-repo") { - if (testParam.name === "without globs" || testParam.name === "with prefix globs") { - return - } - } + // if (handler.name === "git-repo") { + // if (testParam.name === "without globs" || testParam.name === "with prefix globs") { + // return + // } + // } // doesn't match file exclusion pattern -> should be included const notExcludedByFilename = resolve(tmpPath, "bar.txt") From f4a44ab05c22b5effc67d25d69c482fbd4805ce4 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 11 Dec 2023 10:49:55 +0100 Subject: [PATCH 2/7] test(git): fine-grained test cases for file exclusion --- core/test/unit/src/vcs/git.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index 67616b1934..8d484b7479 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -427,14 +427,25 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { // the exclusion paths with and without glob prefix **/ works in the same way. context("when only exclude filter is specified", () => { context("should filter out files that match the exclude filter", () => { + const excludedFilenameNoExt = "foo" + const excludedFilenameTxt = `${excludedFilenameNoExt}.txt` + const excludedFilenameWildcard = `${excludedFilenameNoExt}.*` const testParams = [ { - name: "without globs", - pathBuilder: (path: string) => path, + name: "by exact filename without globs", + pathBuilder: () => excludedFilenameTxt, }, { - name: "with globs", - pathBuilder: (path: string) => join("**", path), + name: "by exact filename with prefix globs", + pathBuilder: () => join("**", excludedFilenameTxt), + }, + { + name: "by filename with wildcard extension without prefix globs", + pathBuilder: () => excludedFilenameWildcard, + }, + { + name: "by filename with wildcard extension with prefix globs", + pathBuilder: () => join("**", excludedFilenameWildcard), }, ] @@ -472,7 +483,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { path: tmpPath, scanRoot: undefined, include: undefined, - exclude: [testParam.pathBuilder("foo.*")], + exclude: [testParam.pathBuilder()], log, }) ) From d6c72d9b508349c65ac3802f822734019a6050f9 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Wed, 6 Dec 2023 15:51:27 +0100 Subject: [PATCH 3/7] chore(test): add missing explicit types in helper functions --- core/test/unit/src/vcs/git.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index 8d484b7479..9c841e71e5 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -362,16 +362,16 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const testParams = [ { name: "when directory is included by exact relative path", - pathBuilder: (subDirName: string, deepDirName) => join(subDirName, deepDirName), + pathBuilder: (subDirName: string, deepDirName: string) => join(subDirName, deepDirName), }, { name: "when directory is included by relative path with globs", - pathBuilder: (subDirName: string, deepDirName) => join(subDirName, deepDirName, "**", "*"), + pathBuilder: (subDirName: string, deepDirName: string) => join(subDirName, deepDirName, "**", "*"), }, { name: "when directory is included by name with globs", // FIXME: shouldn't just '**/deepdir' work well too? - pathBuilder: (_subDirName: string, deepDirName) => join("**", deepDirName, "**", "*"), + pathBuilder: (_subDirName: string, deepDirName: string) => join("**", deepDirName, "**", "*"), }, ] From 834a12fbb22c5168268f647a8b69de1197d82650 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 11 Dec 2023 11:33:03 +0100 Subject: [PATCH 4/7] perf: evaluate exclude/include filter matching only if necessary --- core/src/vcs/git.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index 271da0c9ad..6f228fdc8e 100644 --- a/core/src/vcs/git.ts +++ b/core/src/vcs/git.ts @@ -367,9 +367,11 @@ export class GitHandler extends VcsHandler { return } - const passesExclusionFilter = matchPath(filePath, undefined, exclude) - if (hasIncludes && !passesExclusionFilter) { - return + if (hasIncludes) { + const passesExclusionFilter = matchPath(filePath, undefined, exclude) + if (!passesExclusionFilter) { + return + } } // We push to the output array if it passes through the exclude filters. From 32520478ba19c38eed9960865428986765979750 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 11 Dec 2023 12:22:38 +0100 Subject: [PATCH 5/7] chore(test): rename some helpers --- core/test/unit/src/vcs/git.ts | 39 ++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index 9c841e71e5..f25b0a6f36 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -362,16 +362,17 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const testParams = [ { name: "when directory is included by exact relative path", - pathBuilder: (subDirName: string, deepDirName: string) => join(subDirName, deepDirName), + inclusionBuilder: (subDirName: string, deepDirName: string) => join(subDirName, deepDirName), }, { name: "when directory is included by relative path with globs", - pathBuilder: (subDirName: string, deepDirName: string) => join(subDirName, deepDirName, "**", "*"), + inclusionBuilder: (subDirName: string, deepDirName: string) => + join(subDirName, deepDirName, "**", "*"), }, { name: "when directory is included by name with globs", // FIXME: shouldn't just '**/deepdir' work well too? - pathBuilder: (_subDirName: string, deepDirName: string) => join("**", deepDirName, "**", "*"), + inclusionBuilder: (_subDirName: string, deepDirName: string) => join("**", deepDirName, "**", "*"), }, ] @@ -386,7 +387,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const path = resolve(deepDir, "foo.txt") await createFile(path) - const include = [testParam.pathBuilder(subdirName, deepDirName)] + const include = [testParam.inclusionBuilder(subdirName, deepDirName)] const files = ( await handler.getFiles({ path: tmpPath, @@ -433,19 +434,19 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const testParams = [ { name: "by exact filename without globs", - pathBuilder: () => excludedFilenameTxt, + exclusionBuilder: () => excludedFilenameTxt, }, { name: "by exact filename with prefix globs", - pathBuilder: () => join("**", excludedFilenameTxt), + exclusionBuilder: () => join("**", excludedFilenameTxt), }, { name: "by filename with wildcard extension without prefix globs", - pathBuilder: () => excludedFilenameWildcard, + exclusionBuilder: () => excludedFilenameWildcard, }, { name: "by filename with wildcard extension with prefix globs", - pathBuilder: () => join("**", excludedFilenameWildcard), + exclusionBuilder: () => join("**", excludedFilenameWildcard), }, ] @@ -483,7 +484,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { path: tmpPath, scanRoot: undefined, include: undefined, - exclude: [testParam.pathBuilder()], + exclude: [testParam.exclusionBuilder()], log, }) ) @@ -501,19 +502,19 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const testParams = [ { name: "without globs", - pathBuilder: (subDirName: string) => subDirName, + exclusionBuilder: (subDirName: string) => subDirName, }, { name: "with prefix globs", - pathBuilder: (subDirName: string) => join("**", subDirName), + exclusionBuilder: (subDirName: string) => join("**", subDirName), }, { name: "with full globs", - pathBuilder: (subDirName: string) => join("**", subDirName, "**", "*"), + exclusionBuilder: (subDirName: string) => join("**", subDirName, "**", "*"), }, { name: "with redundant relative path", - pathBuilder: (subDirName: string) => `./${subDirName}`, + exclusionBuilder: (subDirName: string) => `./${subDirName}`, }, ] @@ -562,7 +563,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { path: tmpPath, scanRoot: undefined, include: undefined, // when include: [], getFiles() always returns an empty result - exclude: [testParam.pathBuilder(excludedDirName)], + exclude: [testParam.exclusionBuilder(excludedDirName)], log, }) ) @@ -579,19 +580,19 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { const testParams = [ { name: "without globs", - pathBuilder: (...subDirNames: string[]) => subDirNames.at(-1)!, + exclusionBuilder: (...subDirNames: string[]) => subDirNames.at(-1)!, }, { name: "with prefix globs", - pathBuilder: (...subDirNames: string[]) => join("**", subDirNames.at(-1)!), + exclusionBuilder: (...subDirNames: string[]) => join("**", subDirNames.at(-1)!), }, { name: "with full globs", - pathBuilder: (...subDirNames: string[]) => join("**", subDirNames.at(-1)!, "**", "*"), + exclusionBuilder: (...subDirNames: string[]) => join("**", subDirNames.at(-1)!, "**", "*"), }, { name: "with redundant relative path", - pathBuilder: (...subDirNames: string[]) => `./${subDirNames.join("/")}`, + exclusionBuilder: (...subDirNames: string[]) => `./${subDirNames.join("/")}`, }, ] @@ -640,7 +641,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { path: tmpPath, scanRoot: undefined, include: undefined, // when include: [], getFiles() always returns an empty result - exclude: [testParam.pathBuilder(notExcludedDirName, excludedSubDirectoryName)], + exclude: [testParam.exclusionBuilder(notExcludedDirName, excludedSubDirectoryName)], log, }) ) From 0587044f4e86a7b1a02ee650c1fa833aac9dd04f Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 11 Dec 2023 13:05:13 +0100 Subject: [PATCH 6/7] test: fix failing test More robust way of the default `Dockerfile` exclusion. It doesn't have to be a first element in the array of files. --- core/test/unit/src/plugins/container/container.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/core/test/unit/src/plugins/container/container.ts b/core/test/unit/src/plugins/container/container.ts index 627883ad5a..c8aea6c8ec 100644 --- a/core/test/unit/src/plugins/container/container.ts +++ b/core/test/unit/src/plugins/container/container.ts @@ -40,9 +40,9 @@ import { DEFAULT_TEST_TIMEOUT_SEC, GardenApiVersion, } from "../../../../../src/constants.js" -import { resolve } from "path" +import { join, resolve } from "path" import type { ConvertModuleParams } from "../../../../../src/plugin/handlers/Module/convert.js" -import { omit } from "lodash-es" +import { omit, remove } from "lodash-es" import type { GardenTask } from "../../../../../src/types/task.js" import { taskFromConfig } from "../../../../../src/types/task.js" import type { GardenService } from "../../../../../src/types/service.js" @@ -151,7 +151,11 @@ describe("plugins.container", () => { it("returns the dummy Build action if no Dockerfile and an exec Build is needed", async () => { const module = graph.getModule("module-a") as ContainerModule - module.version.files.pop() // remove automatically picked up Dockerfile + + // remove automatically picked up Dockerfile + const defaultDockerfilePath = join(module.path, defaultDockerfileName) + remove(module.version.files, (f) => f === defaultDockerfilePath) + const dummyBuild: ExecBuildConfig = { internal: { basePath: ".", From c15d394eedbc317917ef4a96e4e49762dbc65e60 Mon Sep 17 00:00:00 2001 From: Vladimir Vagaytsev Date: Mon, 11 Dec 2023 14:11:37 +0100 Subject: [PATCH 7/7] fix(git): fix exclude filter in repo Git scan mode Align `exclude` filter behavior between `subtree` and `repo` Git repository scan modes. Now both `GitHandler` and `GitRepoHandler` behave identically if any exclusions are configured. chore: amend --- core/src/vcs/git-repo.ts | 13 ++++++++++++- core/test/unit/src/vcs/git.ts | 25 +++---------------------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/core/src/vcs/git-repo.ts b/core/src/vcs/git-repo.ts index f022e59fa5..8690c44f1e 100644 --- a/core/src/vcs/git-repo.ts +++ b/core/src/vcs/git-repo.ts @@ -16,7 +16,7 @@ import { normalize, sep } from "path" const { pathExists } = fsExtra -type ScanRepoParams = Pick +type ScanRepoParams = Pick interface GitRepoGetFilesParams extends GetFilesParams { scanFromProjectRoot: boolean @@ -88,6 +88,15 @@ export class GitRepoHandler extends GitHandler { path: scanRoot, pathDescription: pathDescription || "repository", failOnPrompt, + // This method delegates to the old "subtree" Git scan mode that use `--exclude` and `--glob-pathspecs`. + // We need to pass the exclude-filter to ensure that all excluded files and dirs are excluded properly, + // i.e. with the properly expanded globs. + // Otherwise, the "repo" Git scan mode may return some excluded files + // which should be skipped by design and are skipped by the "subtree" mode. + // Thus, some excluded files can appear in the resulting fileTree. + // It happens because the new "repo" mode does not use `--glob-pathspecs` flag + // and does some explicit glob patterns augmentation that misses some edge-cases. + exclude: params.exclude, }) const moduleFiles = fileTree.getFilesAtPath(path) @@ -124,6 +133,8 @@ export class GitRepoHandler extends GitHandler { /** * Scans the given repo root and caches the list of files in the tree cache. * Uses an async lock to ensure a repo root is only scanned once. + * + * Delegates to {@link GitHandler.getFiles}. */ async scanRepo(params: ScanRepoParams): Promise { const { log, path } = params diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index f25b0a6f36..f8d4c648e3 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -371,7 +371,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { }, { name: "when directory is included by name with globs", - // FIXME: shouldn't just '**/deepdir' work well too? + // FIXME-GITREPOHANDLER: shouldn't just '**/deepdir' work well too? inclusionBuilder: (_subDirName: string, deepDirName: string) => join("**", deepDirName, "**", "*"), }, ] @@ -452,13 +452,6 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { for (const testParam of testParams) { it(testParam.name, async () => { - // FIXME - // if (handler.name === "git-repo") { - // if (testParam.name === "without globs") { - // return - // } - // } - // matches file exclusion pattern -> should be excluded const excludedByFilename = resolve(tmpPath, "foo.txt") await createFile(excludedByFilename) @@ -529,13 +522,6 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { */ for (const testParam of testParams) { it(testParam.name, async () => { - // FIXME - // if (handler.name === "git-repo") { - // if (testParam.name === "with prefix globs") { - // return - // } - // } - // doesn't match file exclusion pattern -> should be included const notExcludedByFilename = resolve(tmpPath, "bar.txt") @@ -607,13 +593,6 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { */ for (const testParam of testParams) { it(testParam.name, async () => { - // FIXME - // if (handler.name === "git-repo") { - // if (testParam.name === "without globs" || testParam.name === "with prefix globs") { - // return - // } - // } - // doesn't match file exclusion pattern -> should be included const notExcludedByFilename = resolve(tmpPath, "bar.txt") @@ -1392,6 +1371,8 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { }) } +// FIXME-GITREPOHANDLER: revisit these tests and disk-based configs, +// inspect the scenarios when both include and exclude filters are defined.s const getTreeVersionTests = (gitScanMode: GitScanMode) => { const gitHandlerCls = getGitHandlerCls(gitScanMode) describe("getTreeVersion", () => {