fix(hot-reload): fix path handling for Windows and add tests

garden-service/src/plugins/kubernetes/hot-reload.ts

@@ -8,10 +8,11 @@
 
 import * as Bluebird from "bluebird"
 import * as execa from "execa"
+import normalizePath = require("normalize-path")
 import { V1Deployment, V1DaemonSet, V1StatefulSet } from "@kubernetes/client-node"
 import { ContainerModule, ContainerHotReloadSpec } from "../container/config"
 import { RuntimeError, ConfigurationError } from "../../exceptions"
-import { resolve as resolvePath, normalize, dirname } from "path"
+import { resolve as resolvePath, dirname } from "path"
 import { deline } from "../../util/string"
 import { set } from "lodash"
 import { Service } from "../../types/service"
@@ -189,8 +190,11 @@ export async function hotReloadContainer(
  */
 export function makeCopyCommand(syncTargets: string[]) {
   const commands = syncTargets.map((target) => {
-    const syncCopySource = normalize(`${target}/`)
-    const syncVolumeTarget = normalize(`/.garden/hot_reload/${target}/`)
+    // Note that we're using `normalizePath` as opposed to `path.normalize` since the latter will produce
+    // Win32 style paths on Windows, whereas the command produced runs inside a container that expects
+    // POSIX style paths.
+    const syncCopySource = normalizePath(`${target}/`, false)
+    const syncVolumeTarget = normalizePath(`/.garden/hot_reload/${target}/`, false)
     return `mkdir -p ${dirname(syncVolumeTarget)} && cp -r ${syncCopySource} ${syncVolumeTarget}`
   })
   return commands.join(" && ")
@@ -200,11 +204,11 @@ export function removeTrailingSlashes(path: string) {
   return path.replace(/\/*$/, "")
 }
 
-function rsyncSourcePath(modulePath: string, sourcePath: string) {
+export function rsyncSourcePath(modulePath: string, sourcePath: string) {
   const path = resolvePath(modulePath, sourcePath)
-    .replace(/\/*$/, "/") // ensure (exactly one) trailing slash
 
   return normalizeLocalRsyncPath(path)
+    .replace(/\/*$/, "/") // ensure (exactly one) trailing slash
 }
 
 /**
@@ -246,6 +250,9 @@ export async function syncToService(
     return Bluebird.map(hotReloadSpec.sync, ({ source, target }) => {
       const src = rsyncSourcePath(service.sourceModule.path, source)
       const destination = `rsync://localhost:${portForward.localPort}/volume/${rsyncTargetPath(target)}`
+
+      log.debug(`Hot-reloading from ${src} to ${destination}`)
+
       return execa("rsync", ["-vrpztgo", src, destination])
     })
   } catch (error) {

garden-service/test/helpers.ts

@@ -407,6 +407,10 @@ export function withDefaultGlobalOpts(opts: any) {
   return extend(mapValues(GLOBAL_OPTIONS, (opt) => opt.defaultValue), opts)
 }
 
+export function setPlatform(platform: string) {
+  Object.defineProperty(process, "platform", { value: platform })
+}
+
 export function freezeTime(date?: Date) {
   if (!date) {
     date = new Date()

garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts

@@ -1,11 +1,14 @@
+import { platform } from "os"
 import { expect } from "chai"
-import { HotReloadableResource } from "../../../../../src/plugins/kubernetes/hot-reload"
+import * as td from "testdouble"
+import { HotReloadableResource, rsyncSourcePath } from "../../../../../src/plugins/kubernetes/hot-reload"
 
 import {
   removeTrailingSlashes,
   makeCopyCommand,
   configureHotReload,
 } from "../../../../../src/plugins/kubernetes/hot-reload"
+import { setPlatform } from "../../../../helpers"
 
 describe("configureHotReload", () => {
   it("should correctly augment a resource manifest with containers and volume for hot reloading", async () => {
@@ -140,23 +143,122 @@ describe("removeTrailingSlashes", () => {
   }
 })
 
+describe("rsyncSourcePath", () => {
+  const currentPlatform = platform()
+
+  context("platform uses POSIX style paths", () => {
+    const modulePath = "/module/path"
+
+    before(() => {
+      setPlatform("darwin")
+    })
+
+    beforeEach(() => {
+      if (currentPlatform === "win32") {
+        // Mock the path.resolve function if testing for POSIX style platforms on a Windows platform.
+        const path = require("path")
+        const resolve = td.replace(path, "resolve")
+        td.when(resolve(modulePath, "foo")).thenReturn(`${modulePath}/foo`)
+        td.when(resolve(modulePath, "foo/")).thenReturn(`${modulePath}/foo`)
+        td.when(resolve(modulePath, "foo/bar")).thenReturn(`${modulePath}/foo/bar`)
+        td.when(resolve(modulePath, "foo/bar/")).thenReturn(`${modulePath}/foo/bar`)
+        td.when(resolve(modulePath, "foo/bar//")).thenReturn(`${modulePath}/foo/bar`)
+      }
+    })
+
+    after(() => {
+      setPlatform(currentPlatform)
+    })
+
+    const paths = [
+      ["foo", "/module/path/foo/"],
+      ["foo/", "/module/path/foo/"],
+      ["foo/bar", "/module/path/foo/bar/"],
+      ["foo/bar/", "/module/path/foo/bar/"],
+      ["foo/bar//", "/module/path/foo/bar/"],
+    ]
+    for (const path of paths) {
+      it(`returns the full path with a trailing slash for ${path[0]}`, () => {
+        expect(rsyncSourcePath(modulePath, path[0])).to.eql(path[1])
+      })
+    }
+  })
+
+  context("platform uses Win32 style paths", () => {
+    const modulePath = "C:\\module\\path"
+
+    before(() => {
+      setPlatform("win32")
+    })
+
+    beforeEach(() => {
+      if (currentPlatform !== "win32") {
+        // Mock the path.resolve function when testing for Windows on a non-windows platform.
+        const path = require("path")
+        const resolve = td.replace(path, "resolve")
+        td.when(resolve(modulePath, "foo")).thenReturn(`${modulePath}\\foo`)
+        td.when(resolve(modulePath, "foo/")).thenReturn(`${modulePath}\\foo`)
+        td.when(resolve(modulePath, "foo/bar")).thenReturn(`${modulePath}\\foo\\bar`)
+        td.when(resolve(modulePath, "foo/bar/")).thenReturn(`${modulePath}\\foo\\bar`)
+        td.when(resolve(modulePath, "foo/bar//")).thenReturn(`${modulePath}\\foo\\bar`)
+      }
+    })
+
+    after(() => {
+      setPlatform(currentPlatform)
+    })
+
+    const paths = [
+      ["foo", "/cygdrive/c/module/path/foo/"],
+      ["foo/", "/cygdrive/c/module/path/foo/"],
+      ["foo/bar", "/cygdrive/c/module/path/foo/bar/"],
+      ["foo/bar/", "/cygdrive/c/module/path/foo/bar/"],
+      ["foo/bar//", "/cygdrive/c/module/path/foo/bar/"],
+    ]
+
+    for (const p of paths) {
+      it(`returns the full path with a trailing slash for ${p[0]}`, () => {
+        expect(rsyncSourcePath(modulePath, p[0])).to.eql(p[1])
+      })
+    }
+  })
+})
+
 describe("makeCopyCommand", () => {
+  const resA = "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/"
+  const resB = "mkdir -p /.garden/hot_reload/app/src && cp -r /app/src/foo/ /.garden/hot_reload/app/src/foo/"
+  const resC = "mkdir -p /.garden/hot_reload/app && cp -r /app/src1/ /.garden/hot_reload/app/src1/ && " +
+    "mkdir -p /.garden/hot_reload/app && cp -r /app/src2/ /.garden/hot_reload/app/src2/"
 
   it("ensures a trailing slash in the copy source and target", () => {
-    const cmd = "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/"
-    expect(makeCopyCommand(["/app/"])).to.eql(cmd)
-    expect(makeCopyCommand(["/app"])).to.eql(cmd)
+    expect(makeCopyCommand(["/app/"])).to.eql(resA)
+    expect(makeCopyCommand(["/app"])).to.eql(resA)
   })
 
   it("correctly handles target paths with more than one path component", () => {
-    const cmd = "mkdir -p /.garden/hot_reload/app/src && cp -r /app/src/foo/ /.garden/hot_reload/app/src/foo/"
-    expect(makeCopyCommand(["/app/src/foo"])).to.eql(cmd)
+    expect(makeCopyCommand(["/app/src/foo"])).to.eql(resB)
   })
 
   it("correctly handles multiple target paths", () => {
-    const cmd = "mkdir -p /.garden/hot_reload/app && cp -r /app/src1/ /.garden/hot_reload/app/src1/ && " +
-      "mkdir -p /.garden/hot_reload/app && cp -r /app/src2/ /.garden/hot_reload/app/src2/"
-    expect(makeCopyCommand(["/app/src1", "/app/src2/"])).to.eql(cmd)
+    expect(makeCopyCommand(["/app/src1", "/app/src2/"])).to.eql(resC)
   })
 
+  context("platform is Windows", () => {
+    const currentPlatform = platform()
+
+    before(() => {
+      setPlatform("win32")
+    })
+
+    after(() => {
+      setPlatform(currentPlatform)
+    })
+
+    it("should return the same value as on platforms that use POSIX style paths", () => {
+      expect(makeCopyCommand(["/app/"])).to.eql(resA)
+      expect(makeCopyCommand(["/app"])).to.eql(resA)
+      expect(makeCopyCommand(["/app/src/foo"])).to.eql(resB)
+      expect(makeCopyCommand(["/app/src1", "/app/src2/"])).to.eql(resC)
+    })
+  })
 })