refactor(multi-repo): require tag or branch in repository URLs

BREAKING CHANGES:

Repository URLs now must contain a hash part pointing to a specific
branch or tag. E.g: https://github.com/org/repo.git#master

All garden.yml config files that contain a repositoryUrl
key must be updated accordingly.

docs/reference/config.md

@@ -35,11 +35,10 @@ module:
 
   description:
 
-  # A remote repository URL to fetch the module from. Garden will read the garden.yml config from
-  # the local module. Currently only supports git servers.
+  # A remote respository URL. Currently only supports git servers. Must contain a hash part
+  # pointing to a specific branch or tag with the format: <git remote url>#<branch|tag>
   #
-  # Example: "<git remote url>#<branch|tag> or
-  # git+https://github.com/organization/some-module.git#v2.0"
+  # Example: "git+https://github.com/org/repo.git#v2.0"
   #
   # Optional.
   repositoryUrl:
@@ -189,11 +188,10 @@ project:
       # Required.
       name:
 
-      # A remote respository URL. Currently only supports git servers. Use hash notation (#) to
-      # point to a specific branch or tag
+      # A remote respository URL. Currently only supports git servers. Must contain a hash part
+      # pointing to a specific branch or tag with the format: <git remote url>#<branch|tag>
       #
-      # Example: "<git remote url>#<branch|tag> or
-      # git+https://github.com/organization/some-module.git#v2.0"
+      # Example: "git+https://github.com/org/repo.git#v2.0"
       #
       # Required.
       repositoryUrl:
@@ -225,11 +223,10 @@ module:
 
   description:
 
-  # A remote repository URL to fetch the module from. Garden will read the garden.yml config from
-  # the local module. Currently only supports git servers.
+  # A remote respository URL. Currently only supports git servers. Must contain a hash part
+  # pointing to a specific branch or tag with the format: <git remote url>#<branch|tag>
   #
-  # Example: "<git remote url>#<branch|tag> or
-  # git+https://github.com/organization/some-module.git#v2.0"
+  # Example: "git+https://github.com/org/repo.git#v2.0"
   #
   # Optional.
   repositoryUrl:
@@ -359,11 +356,10 @@ module:
 
   description:
 
-  # A remote repository URL to fetch the module from. Garden will read the garden.yml config from
-  # the local module. Currently only supports git servers.
+  # A remote respository URL. Currently only supports git servers. Must contain a hash part
+  # pointing to a specific branch or tag with the format: <git remote url>#<branch|tag>
   #
-  # Example: "<git remote url>#<branch|tag> or
-  # git+https://github.com/organization/some-module.git#v2.0"
+  # Example: "git+https://github.com/org/repo.git#v2.0"
   #
   # Optional.
   repositoryUrl:

garden-service/src/config/common.ts

@@ -85,10 +85,10 @@ export const joiRepositoryUrl = () => Joi
     ],
   })
   .description(
-    "A remote respository URL. Currently only supports git servers. Use hash notation (#) to point to" +
-    " a specific branch or tag",
+    "A remote respository URL. Currently only supports git servers. Must contain a hash part" +
+    " pointing to a specific branch or tag with the format: <git remote url>#<branch|tag>",
   )
-  .example("<git remote url>#<branch|tag> or git+https://github.com/organization/some-module.git#v2.0")
+  .example("git+https://github.com/org/repo.git#v2.0")
 
 export function isPrimitive(value: any) {
   return typeof value === "string" || typeof value === "number" || typeof value === "boolean"

garden-service/src/config/module.ts

@@ -80,12 +80,7 @@ export const baseModuleSpecSchema = Joi.object()
       .description("The name of this module.")
       .example("my-sweet-module"),
     description: Joi.string(),
-    repositoryUrl: joiRepositoryUrl()
-      .description(
-        "A remote repository URL to fetch the module from. Garden will read the garden.yml config" +
-        " from the local module." +
-        " Currently only supports git servers.",
-      ),
+    repositoryUrl: joiRepositoryUrl(),
     variables: joiVariables()
       .description("Variables that this module can reference and expose as environment variables.")
       .example({ "my-variable": "some-value" }),

garden-service/src/vcs/git.ts

@@ -12,6 +12,7 @@ import { ensureDir, pathExists, stat } from "fs-extra"
 import Bluebird = require("bluebird")
 
 import { NEW_MODULE_VERSION, VcsHandler, RemoteSourceParams } from "./base"
+import { ConfigurationError } from "../exceptions"
 
 export const helpers = {
   gitCli: (cwd: string): (cmd: string, args: string[]) => Promise<string> => {
@@ -21,17 +22,25 @@ export const helpers = {
   },
 }
 
-function getGitUrlParts(url: string) {
-  const parts = url.split("#")
-  return { repositoryUrl: parts[0], hash: parts[1] }
+export function getCommitIdFromRefList(refList: string): string {
+  try {
+    return refList.split("\n")[0].split("\t")[0]
+  } catch (err) {
+    return refList
+  }
 }
 
-function parseRefList(res: string): string {
-  const refList = res.split("\n").map(str => {
-    const parts = str.split("\n")
-    return { commitId: parts[0], ref: parts[1] }
-  })
-  return refList[0].commitId
+export function parseGitUrl(url: string) {
+  const parts = url.split("#")
+  const parsed = { repositoryUrl: parts[0], hash: parts[1] }
+  if (!parsed.hash) {
+    throw new ConfigurationError(
+      "Repository URLs must contain a hash part pointing to a specific branch or tag" +
+      " (e.g. https://github.com/org/repo.git#master)",
+      { repositoryUrl: url },
+    )
+  }
+  return parsed
 }
 
 // TODO Consider moving git commands to separate (and testable) functions
@@ -94,14 +103,9 @@ export class GitHandler extends VcsHandler {
 
     if (!isCloned) {
       const entry = logEntry.info({ section: name, msg: `Fetching from ${url}`, status: "active" })
-      const { repositoryUrl, hash } = getGitUrlParts(url)
-
-      const cmdOpts = ["--depth=1"]
-      if (hash) {
-        cmdOpts.push("--branch=hash")
-      }
+      const { repositoryUrl, hash } = parseGitUrl(url)
 
-      await git("clone", [...cmdOpts, repositoryUrl, absPath])
+      await git("clone", ["--depth=1", `--branch=${hash}`, repositoryUrl, absPath])
 
       entry.setSuccess()
     }
@@ -112,25 +116,21 @@ export class GitHandler extends VcsHandler {
   async updateRemoteSource({ url, name, sourceType, logEntry }: RemoteSourceParams) {
     const absPath = join(this.projectRoot, this.getRemoteSourcePath(name, url, sourceType))
     const git = helpers.gitCli(absPath)
-    const { repositoryUrl, hash } = getGitUrlParts(url)
+    const { repositoryUrl, hash } = parseGitUrl(url)
 
     await this.ensureRemoteSource({ url, name, sourceType, logEntry })
 
     const entry = logEntry.info({ section: name, msg: "Getting remote state", status: "active" })
     await git("remote", ["update"])
 
-    const listRemoteArgs = hash ? [repositoryUrl, hash] : [repositoryUrl]
-    const showRefArgs = hash ? [hash] : []
-    const remoteCommitId = parseRefList(await git("ls-remote", listRemoteArgs))
-    const localCommitId = parseRefList(await git("show-ref", ["--hash", ...showRefArgs]))
+    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}`)
 
-      const fetchArgs = hash ? ["origin", hash] : ["origin"]
-      const resetArgs = hash ? [`origin/${hash}`] : ["origin"]
-      await git("fetch", ["--depth=1", ...fetchArgs])
-      await git("reset", ["--hard", ...resetArgs])
+      await git("fetch", ["--depth=1", "origin", hash])
+      await git("reset", ["--hard", `origin/${hash}`])
 
       entry.setSuccess("Source updated")
     } else {

garden-service/test/data/test-project-ext-module-sources/module-a/garden.yml

@@ -1,6 +1,6 @@
 module:
   name: module-a
-  repositoryUrl: https://my-git-server.com/module-a.git
+  repositoryUrl: https://my-git-server.com/module-a.git#master
   type: test
   services:
     - name: service-a

garden-service/test/data/test-project-ext-module-sources/module-b/garden.yml

@@ -1,6 +1,6 @@
 module:
   name: module-b
-  repositoryUrl: https://my-git-server.com/module-b.git
+  repositoryUrl: https://my-git-server.com/module-b.git#master
   type: test
   services:
     - name: service-b

garden-service/test/data/test-project-ext-module-sources/module-c/garden.yml

@@ -1,6 +1,6 @@
 module:
   name: module-c
-  repositoryUrl: https://my-git-server.com/module-c.git
+  repositoryUrl: https://my-git-server.com/module-c.git#master
   type: test
   services:
     - name: service-c

garden-service/test/data/test-project-ext-project-sources/garden.yml

@@ -2,11 +2,11 @@ project:
   name: test-project-ext-project-sources
   sources:
     - name: source-a
-      repositoryUrl: https://my-git-server.com/source-a.git
+      repositoryUrl: https://my-git-server.com/source-a.git#master
     - name: source-b
-      repositoryUrl: https://my-git-server.com/source-b.git
+      repositoryUrl: https://my-git-server.com/source-b.git#master
     - name: source-c
-      repositoryUrl: https://my-git-server.com/source-c.git
+      repositoryUrl: https://my-git-server.com/source-c.git#master
   environmentDefaults:
     variables:
       some: variable

garden-service/test/src/garden.ts

@@ -548,7 +548,7 @@ describe("Garden", () => {
     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 repositoryUrl = "foo"
+      const repositoryUrl = "https://github.com/org/repo.git#master"
       const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "source-a", sourceType: "project" })
       const repoUrlHash = hashRepoUrl(repositoryUrl)
       expect(path).to.equal(join(projectRoot, ".garden", "sources", "project", `source-a--${repoUrlHash}`))
@@ -557,7 +557,7 @@ describe("Garden", () => {
     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 repositoryUrl = "foo"
+      const repositoryUrl = "https://github.com/org/repo.git#master"
       const path = await ctx.loadExtSourcePath({ repositoryUrl, name: "module-a", sourceType: "module" })
       const repoUrlHash = hashRepoUrl(repositoryUrl)
       expect(path).to.equal(join(projectRoot, ".garden", "sources", "module", `module-a--${repoUrlHash}`))
@@ -574,7 +574,11 @@ describe("Garden", () => {
       }]
       await ctx.localConfigStore.set(["linkedProjectSources"], linked)
 
-      const path = await ctx.loadExtSourcePath({ name: "source-a", repositoryUrl: "", sourceType: "project" })
+      const path = await ctx.loadExtSourcePath({
+        name: "source-a",
+        repositoryUrl: "https://github.com/org/repo.git#master",
+        sourceType: "project",
+      })
 
       expect(path).to.equal(join(projectRoot, "mock-local-path", "source-a"))
     })
@@ -590,7 +594,11 @@ describe("Garden", () => {
       }]
       await ctx.localConfigStore.set(["linkedModuleSources"], linked)
 
-      const path = await ctx.loadExtSourcePath({ name: "module-a", repositoryUrl: "", sourceType: "module" })
+      const path = await ctx.loadExtSourcePath({
+        name: "module-a",
+        repositoryUrl: "https://github.com/org/repo.git#master",
+        sourceType: "module",
+      })
 
       expect(path).to.equal(join(projectRoot, "mock-local-path", "module-a"))
     })

garden-service/test/src/vcs/git.ts

@@ -0,0 +1,44 @@
+import { expect } from "chai"
+import dedent = require("dedent")
+
+import { expectError } from "../../helpers"
+import { getCommitIdFromRefList, parseGitUrl } from "../../../src/vcs/git"
+
+describe("git", () => {
+  describe("getCommitIdFromRefList", () => {
+    it("should get the commit id from a list of commit ids and refs", () => {
+      const refList = dedent`
+      abcde	ref/heads/master
+      1234	ref/heads/master
+      foobar	ref/heads/master
+      `
+      expect(getCommitIdFromRefList(refList)).to.equal("abcde")
+    })
+    it("should get the commit id from a list of commit ids without refs", () => {
+      const refList = dedent`
+      abcde
+      1234	ref/heads/master
+      foobar	ref/heads/master
+      `
+      expect(getCommitIdFromRefList(refList)).to.equal("abcde")
+    })
+    it("should get the commit id from a single commit id / ref pair", () => {
+      const refList = "abcde	ref/heads/master"
+      expect(getCommitIdFromRefList(refList)).to.equal("abcde")
+    })
+    it("should get the commit id from single commit id without a ref", () => {
+      const refList = "abcde"
+      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"
+      expect(parseGitUrl(url)).to.eql({ repositoryUrl: "https://github.com/org/repo.git", hash: "branch" })
+    })
+    it("should throw a configuration error if the hash part is missing", async () => {
+      const url = "https://github.com/org/repo.git"
+      await expectError(() => parseGitUrl(url), "configuration")
+    })
+  })
+})