Skip to content

Commit

Permalink
fix(git): fix exclude filter in repo Git scan mode (#5526)
Browse files Browse the repository at this point in the history 
* test(git): re-enable failing tests

* test(git): fine-grained test cases for file exclusion

* chore(test): add missing explicit types in helper functions

* perf: evaluate exclude/include filter matching only if necessary

* chore(test): rename some helpers

* 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.

* 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
  • Loading branch information
@vvagaytsev
vvagaytsev authored Dec 22, 2023
1 parent 396f6d8 commit 5ef9998
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 48 deletions.
13 changes: 12 additions & 1 deletion core/src/vcs/git-repo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { normalize, sep } from "path"

const { pathExists } = fsExtra

type ScanRepoParams = Pick<GetFilesParams, "log" | "path" | "pathDescription" | "failOnPrompt">
type ScanRepoParams = Pick<GetFilesParams, "log" | "path" | "pathDescription" | "failOnPrompt" | "exclude">

interface GitRepoGetFilesParams extends GetFilesParams {
scanFromProjectRoot: boolean
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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<FileTree> {
const { log, path } = params
Expand Down
8 changes: 5 additions & 3 deletions core/src/vcs/git.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 7 additions & 3 deletions core/test/unit/src/plugins/container/container.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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: ".",
Expand Down
75 changes: 34 additions & 41 deletions core/test/unit/src/vcs/git.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, "**", "*"),
},
]

Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -472,7 +477,7 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => {
path: tmpPath,
scanRoot: undefined,
include: undefined,
exclude: [testParam.pathBuilder("foo.*")],
exclude: [testParam.exclusionBuilder()],
log,
})
)
Expand All @@ -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}`,
},
]

Expand All @@ -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")

Expand Down Expand Up @@ -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,
})
)
Expand All @@ -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("/")}`,
},
]

Expand All @@ -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")

Expand Down Expand Up @@ -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,
})
)
Expand Down Expand Up @@ -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", () => {
Expand Down

0 comments on commit 5ef9998

Please sign in to comment.