From 3b85c350157d6ea66dbe2758c5113d8ca004837e Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Wed, 27 Feb 2019 18:22:04 +0100 Subject: [PATCH] fix(vcs): untracked files didn't update version timestamp correctly Needed to refactor slightly to add tests for this. --- .circleci/config.yml | 5 + garden-service/src/vcs/base.ts | 32 ++++++- garden-service/src/vcs/git.ts | 91 ++++++++----------- garden-service/test/helpers.ts | 7 +- .../test/src/commands/update-remote.ts | 2 +- garden-service/test/src/garden.ts | 34 +++---- garden-service/test/src/vcs/base.ts | 14 ++- garden-service/test/src/vcs/git.ts | 53 ++++++++++- 8 files changed, 158 insertions(+), 80 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index bb563392f5..9cb4cafcae 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -184,6 +184,11 @@ jobs: docker_layer_caching: true - npm_install - *attach-workspace + - run: + name: Configure git (needed for some tests) + command: | + git config --global user.name "Garden CI" + git config --global user.email "admin@garden.io" - run: name: Make sure generated docs are up-to-date command: npm run check-docs diff --git a/garden-service/src/vcs/base.ts b/garden-service/src/vcs/base.ts index 840e8613c2..4509be9bc9 100644 --- a/garden-service/src/vcs/base.ts +++ b/garden-service/src/vcs/base.ts @@ -13,7 +13,7 @@ import * as Joi from "joi" import { validate } from "../config/common" import { join } from "path" import { GARDEN_VERSIONFILE_NAME } from "../constants" -import { pathExists, readFile, writeFile } from "fs-extra" +import { pathExists, readFile, writeFile, stat } from "fs-extra" import { ConfigurationError } from "../exceptions" import { ExternalSourceType, @@ -83,9 +83,35 @@ export abstract class VcsHandler { constructor(protected projectRoot: string) { } abstract name: string - abstract async getTreeVersion(path: string): Promise + abstract async getLatestCommit(path: string): Promise + abstract async getDirtyFiles(path: string): Promise abstract async ensureRemoteSource(params: RemoteSourceParams): Promise - abstract async updateRemoteSource(params: RemoteSourceParams) + abstract async updateRemoteSource(params: RemoteSourceParams): Promise + + async getTreeVersion(path: string) { + const commitHash = await this.getLatestCommit(path) + const dirtyFiles = await this.getDirtyFiles(path) + + let latestDirty = 0 + + // for dirty trees, we append the last modified time of last modified or added file + if (dirtyFiles.length) { + const stats = await Bluebird.filter(dirtyFiles, (file: string) => pathExists(file)) + .map((file: string) => stat(file)) + + let mtimes = stats.map((s) => Math.round(s.mtime.getTime() / 1000)) + let latest = mtimes.sort().slice(-1)[0] + + if (latest > latestDirty) { + latestDirty = latest + } + } + + return { + latestCommit: commitHash, + dirtyTimestamp: latestDirty || null, + } + } async resolveTreeVersion(path: string): Promise { // the version file is used internally to specify versions outside of source control diff --git a/garden-service/src/vcs/git.ts b/garden-service/src/vcs/git.ts index f92c8d87d8..e47ddb710a 100644 --- a/garden-service/src/vcs/git.ts +++ b/garden-service/src/vcs/git.ts @@ -7,21 +7,12 @@ */ import execa = require("execa") -import { join } from "path" -import { ensureDir, pathExists, stat } from "fs-extra" -import Bluebird = require("bluebird") +import { join, resolve } from "path" +import { ensureDir, pathExists } from "fs-extra" import { NEW_MODULE_VERSION, VcsHandler, RemoteSourceParams } from "./base" import { ConfigurationError, RuntimeError } from "../exceptions" -export const helpers = { - gitCli: (cwd: string): (cmd: string, args: string[]) => Promise => { - return async (cmd, args) => { - return execa.stdout("git", [cmd, ...args], { cwd }) - } - }, -} - export function getCommitIdFromRefList(refList: string): string { try { return refList.split("\n")[0].split("\t")[0] @@ -47,70 +38,66 @@ export function parseGitUrl(url: string) { export class GitHandler extends VcsHandler { name = "git" - async getTreeVersion(path: string) { - const git = helpers.gitCli(path) + private gitCli(cwd: string) { + return async (...args: string[]) => { + return execa.stdout("git", args, { cwd }) + } + } + + async getLatestCommit(path: string) { + const git = this.gitCli(path) - let commitHash try { - commitHash = await git("rev-list", [ + return await git( + "rev-list", "--max-count=1", "--abbrev-commit", "--abbrev=10", "HEAD", - ]) || NEW_MODULE_VERSION + ) || NEW_MODULE_VERSION } catch (err) { if (err.code === 128) { // not in a repo root, use default version - commitHash = NEW_MODULE_VERSION + return NEW_MODULE_VERSION } else { throw err } } + } + + async getDirtyFiles(path: string) { + const git = this.gitCli(path) + let modifiedFiles: string[] - let latestDirty = 0 - let modifiedFiles: string[] = [] + const repoRoot = await git("rev-parse", "--show-toplevel") try { - modifiedFiles = (await git("diff-index", ["--name-only", "HEAD", path])).split("\n") + modifiedFiles = (await git("diff-index", "--name-only", "HEAD", path)) + .split("\n") + .filter((f) => f.length > 0) + .map(file => resolve(repoRoot, file)) } catch (err) { if (err.code === 128) { - // no commit in repo, use default version - commitHash = NEW_MODULE_VERSION + // no commit in repo + modifiedFiles = [] } else { throw err } } - const newFiles = (await git("ls-files", ["--other", "--exclude-standard", path])).split("\n") + const newFiles = (await git("ls-files", "--other", "--exclude-standard", path)) + .split("\n") + .filter((f) => f.length > 0) + .map(file => resolve(path, file)) - const dirtyFiles: string[] = modifiedFiles.concat(newFiles).filter((f) => f.length > 0) - - // for dirty trees, we append the last modified time of last modified or added file - if (dirtyFiles.length) { - const repoRoot = await git("rev-parse", ["--show-toplevel"]) - const stats = await Bluebird.map(dirtyFiles, file => join(repoRoot, file)) - .filter((file: string) => pathExists(file)) - .map((file: string) => stat(file)) - - let mtimes = stats.map((s) => Math.round(s.mtime.getTime() / 1000)) - let latest = mtimes.sort().slice(-1)[0] - - if (latest > latestDirty) { - latestDirty = latest - } - } - - return { - latestCommit: commitHash, - dirtyTimestamp: latestDirty || null, - } + return modifiedFiles.concat(newFiles) } // TODO Better auth handling async ensureRemoteSource({ url, name, log, sourceType }: RemoteSourceParams): Promise { const remoteSourcesPath = join(this.projectRoot, this.getRemoteSourcesDirname(sourceType)) await ensureDir(remoteSourcesPath) - const git = helpers.gitCli(remoteSourcesPath) + const git = this.gitCli(remoteSourcesPath) const absPath = join(this.projectRoot, this.getRemoteSourcePath(name, url, sourceType)) const isCloned = await pathExists(absPath) @@ -120,7 +107,7 @@ export class GitHandler extends VcsHandler { const { repositoryUrl, hash } = parseGitUrl(url) try { - await git("clone", ["--depth=1", `--branch=${hash}`, repositoryUrl, absPath]) + await git("clone", "--depth=1", `--branch=${hash}`, repositoryUrl, absPath) } catch (err) { entry.setError() throw new RuntimeError(`Downloading remote ${sourceType} failed with error: \n\n${err}`, { @@ -137,23 +124,23 @@ export class GitHandler extends VcsHandler { async updateRemoteSource({ url, name, sourceType, log }: RemoteSourceParams) { const absPath = join(this.projectRoot, this.getRemoteSourcePath(name, url, sourceType)) - const git = helpers.gitCli(absPath) + const git = this.gitCli(absPath) const { repositoryUrl, hash } = parseGitUrl(url) await this.ensureRemoteSource({ url, name, sourceType, log }) const entry = log.info({ section: name, msg: "Getting remote state", status: "active" }) - await git("remote", ["update"]) + await git("remote", "update") - const remoteCommitId = getCommitIdFromRefList(await git("ls-remote", [repositoryUrl, hash])) - const localCommitId = getCommitIdFromRefList(await git("show-ref", ["--hash", hash])) + const remoteCommitId = getCommitIdFromRefList(await git("ls-remote", repositoryUrl, hash)) + const localCommitId = getCommitIdFromRefList(await git("show-ref", "--hash", hash)) if (localCommitId !== remoteCommitId) { entry.setState(`Fetching from ${url}`) try { - await git("fetch", ["--depth=1", "origin", hash]) - await git("reset", ["--hard", `origin/${hash}`]) + await git("fetch", "--depth=1", "origin", hash) + await git("reset", "--hard", `origin/${hash}`) } catch (err) { entry.setError() throw new RuntimeError(`Updating remote ${sourceType} failed with error: \n\n${err}`, { diff --git a/garden-service/test/helpers.ts b/garden-service/test/helpers.ts index 56a407c6ff..cac95dfca1 100644 --- a/garden-service/test/helpers.ts +++ b/garden-service/test/helpers.ts @@ -33,7 +33,6 @@ import { RunTaskParams, SetSecretParams, } from "../src/types/plugin/params" -import { helpers } from "../src/vcs/git" import { ModuleVersion } from "../src/vcs/base" import { GARDEN_DIR_NAME } from "../src/constants" import { EventBus, Events } from "../src/events" @@ -365,8 +364,8 @@ export const cleanProject = async (projectRoot: string) => { return remove(join(projectRoot, GARDEN_DIR_NAME)) } -export function stubGitCli() { - td.replace(helpers, "gitCli", () => async () => "") +export function stubGitCli(garden: Garden) { + td.replace(garden.vcs, "gitCli", () => async () => "") } /** @@ -374,7 +373,7 @@ export function stubGitCli() { * or test-project-ext-project-sources as project root. */ export function stubExtSources(garden: Garden) { - stubGitCli() + stubGitCli(garden) const getRemoteSourcesDirname = td.replace(garden.vcs, "getRemoteSourcesDirname") td.when(getRemoteSourcesDirname("module")).thenReturn(join("mock-dot-garden", "sources", "module")) diff --git a/garden-service/test/src/commands/update-remote.ts b/garden-service/test/src/commands/update-remote.ts index 19638fc860..3167e29d20 100644 --- a/garden-service/test/src/commands/update-remote.ts +++ b/garden-service/test/src/commands/update-remote.ts @@ -24,7 +24,7 @@ describe("UpdateRemoteCommand", () => { beforeEach(async () => { garden = await makeTestGarden(projectRoot) log = garden.log - stubGitCli() + stubGitCli(garden) }) const projectRoot = getDataDir("test-project-ext-project-sources") diff --git a/garden-service/test/src/garden.ts b/garden-service/test/src/garden.ts index 50985f3242..ba38a6f620 100644 --- a/garden-service/test/src/garden.ts +++ b/garden-service/test/src/garden.ts @@ -194,7 +194,7 @@ describe("Garden", () => { it("should resolve module path to external sources dir if module has a remote source", async () => { const projectRoot = resolve(dataDir, "test-project-ext-module-sources") const garden = await makeTestGarden(projectRoot) - stubGitCli() + stubGitCli(garden) const module = (await (garden).loadModuleConfigs("./module-a"))[0] const repoUrlHash = hashRepoUrl(module!.repositoryUrl!) @@ -260,46 +260,48 @@ describe("Garden", () => { describe("loadExtSourcePath", () => { let projectRoot: string - const makeGardenContext = async (root) => { - const ctx = await makeTestGarden(root) - stubGitCli() - return ctx + const makeGarden = async (root) => { + const garden = await makeTestGarden(root) + stubGitCli(garden) + return garden } afterEach(async () => { - await cleanProject(projectRoot) + if (projectRoot) { + await cleanProject(projectRoot) + } }) it("should return the path to the project source if source type is project", async () => { projectRoot = getDataDir("test-project-ext-project-sources") - const ctx = await makeGardenContext(projectRoot) + const garden = await makeGarden(projectRoot) const repositoryUrl = "https://github.com/org/repo.git#master" - const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "source-a", sourceType: "project" }) + const path = await garden.loadExtSourcePath({ repositoryUrl, name: "source-a", sourceType: "project" }) const repoUrlHash = hashRepoUrl(repositoryUrl) expect(path).to.equal(join(projectRoot, ".garden", "sources", "project", `source-a--${repoUrlHash}`)) }) it("should return the path to the module source if source type is module", async () => { projectRoot = getDataDir("test-project-ext-module-sources") - const ctx = await makeGardenContext(projectRoot) + const garden = await makeGarden(projectRoot) const repositoryUrl = "https://github.com/org/repo.git#master" - const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "module-a", sourceType: "module" }) + const path = await garden.loadExtSourcePath({ repositoryUrl, name: "module-a", sourceType: "module" }) const repoUrlHash = hashRepoUrl(repositoryUrl) expect(path).to.equal(join(projectRoot, ".garden", "sources", "module", `module-a--${repoUrlHash}`)) }) it("should return the local path of the project source if linked", async () => { projectRoot = getDataDir("test-project-ext-project-sources") - const ctx = await makeGardenContext(projectRoot) + const garden = await makeGarden(projectRoot) const localPath = join(projectRoot, "mock-local-path", "source-a") const linked: LinkedSource[] = [{ name: "source-a", path: localPath, }] - await ctx.localConfigStore.set(["linkedProjectSources"], linked) + await garden.localConfigStore.set(["linkedProjectSources"], linked) - const path = await ctx.loadExtSourcePath({ + const path = await garden.loadExtSourcePath({ name: "source-a", repositoryUrl: "https://github.com/org/repo.git#master", sourceType: "project", @@ -310,16 +312,16 @@ describe("Garden", () => { it("should return the local path of the module source if linked", async () => { projectRoot = getDataDir("test-project-ext-module-sources") - const ctx = await makeGardenContext(projectRoot) + const garden = await makeGarden(projectRoot) const localPath = join(projectRoot, "mock-local-path", "module-a") const linked: LinkedSource[] = [{ name: "module-a", path: localPath, }] - await ctx.localConfigStore.set(["linkedModuleSources"], linked) + await garden.localConfigStore.set(["linkedModuleSources"], linked) - const path = await ctx.loadExtSourcePath({ + const path = await garden.loadExtSourcePath({ name: "module-a", repositoryUrl: "https://github.com/org/repo.git#master", sourceType: "module", diff --git a/garden-service/test/src/vcs/base.ts b/garden-service/test/src/vcs/base.ts index d226f2263c..143690023c 100644 --- a/garden-service/test/src/vcs/base.ts +++ b/garden-service/test/src/vcs/base.ts @@ -7,6 +7,14 @@ class TestVcsHandler extends VcsHandler { name = "test" private testVersions: TreeVersions = {} + async getLatestCommit() { + return NEW_MODULE_VERSION + } + + async getDirtyFiles() { + return [] + } + async getTreeVersion(path: string) { return this.testVersions[path] || { latestCommit: NEW_MODULE_VERSION, @@ -18,12 +26,12 @@ class TestVcsHandler extends VcsHandler { this.testVersions[path] = version } - async ensureRemoteSource(_params): Promise { + async ensureRemoteSource(): Promise { return "" } - async updateRemoteSource(_params) { - return "" + async updateRemoteSource() { + return } } diff --git a/garden-service/test/src/vcs/git.ts b/garden-service/test/src/vcs/git.ts index 9b8c6fc062..d413ced013 100644 --- a/garden-service/test/src/vcs/git.ts +++ b/garden-service/test/src/vcs/git.ts @@ -1,8 +1,58 @@ import { expect } from "chai" import dedent = require("dedent") +import * as tmp from "tmp-promise" +import { createFile, writeFile, realpath } from "fs-extra" +import { join, resolve } from "path" import { expectError } from "../../helpers" -import { getCommitIdFromRefList, parseGitUrl } from "../../../src/vcs/git" +import { getCommitIdFromRefList, parseGitUrl, GitHandler } from "../../../src/vcs/git" + +describe("GitHandler", () => { + let tmpDir: tmp.DirectoryResult + let tmpPath: string + let git + let handler: GitHandler + + beforeEach(async () => { + tmpDir = await tmp.dir({ unsafeCleanup: true }) + tmpPath = await realpath(tmpDir.path) + handler = new GitHandler(tmpPath) + git = (handler).gitCli(tmpPath) + await git("init") + }) + + afterEach(async () => { + await tmpDir.cleanup() + }) + + describe("getDirtyFiles", () => { + it("should work with no commits in repo", async () => { + expect(await handler.getDirtyFiles(tmpPath)).to.eql([]) + }) + + it("should return modified files as absolute paths", async () => { + const path = resolve(tmpPath, "foo.txt") + + await createFile(path) + await git("add", ".") + await git("commit", "-m", "foo") + + expect(await handler.getDirtyFiles(tmpPath)).to.eql([]) + + await writeFile(path, "my change") + + expect(await handler.getDirtyFiles(tmpPath)).to.eql([path]) + }) + + it("should return untracked files as absolute paths", async () => { + await createFile(join(tmpPath, "foo.txt")) + + expect(await handler.getDirtyFiles(tmpPath)).to.eql([ + resolve(tmpPath, "foo.txt"), + ]) + }) + }) +}) describe("git", () => { describe("getCommitIdFromRefList", () => { @@ -31,6 +81,7 @@ describe("git", () => { expect(getCommitIdFromRefList(refList)).to.equal("abcde") }) }) + describe("parseGitUrl", () => { it("should return the url part and the hash part from a github url", () => { const url = "https://github.com/org/repo.git#branch"