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/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. 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: ".", diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index aa17db8453..f8d4c648e3 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) => 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) => 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) => join("**", deepDirName, "**", "*"), + // FIXME-GITREPOHANDLER: shouldn't just '**/deepdir' work well too? + 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, @@ -427,26 +428,30 @@ 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", + exclusionBuilder: () => excludedFilenameTxt, }, { - name: "with globs", - pathBuilder: (path: string) => join("**", path), + name: "by exact filename with prefix globs", + exclusionBuilder: () => join("**", excludedFilenameTxt), + }, + { + name: "by filename with wildcard extension without prefix globs", + exclusionBuilder: () => excludedFilenameWildcard, + }, + { + name: "by filename with wildcard extension with prefix globs", + exclusionBuilder: () => join("**", excludedFilenameWildcard), }, ] 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) @@ -472,7 +477,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { path: tmpPath, scanRoot: undefined, include: undefined, - exclude: [testParam.pathBuilder("foo.*")], + exclude: [testParam.exclusionBuilder()], log, }) ) @@ -490,19 +495,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}`, }, ] @@ -517,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") @@ -551,7 +549,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, }) ) @@ -568,19 +566,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("/")}`, }, ] @@ -595,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") @@ -629,7 +620,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, }) ) @@ -1380,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", () => {