From 6d30b7cd1a7b898de418c3d192ea95b93d5edc7e Mon Sep 17 00:00:00 2001
From: Jon Edvald <edvald@gmail.com>
Date: Thu, 7 Nov 2019 20:09:06 +0100
Subject: [PATCH] fix: don't use module exclude field for optimization + add
 test (TBS)

Turns out using module exclude fields would need more elaborate logic,
in the case of overlapping module sources. So I removed that
optimization for now.
---
 docs/guides/configuration-files.md             |  4 ++--
 docs/reference/config.md                       |  9 +++++++++
 docs/reference/module-types/container.md       |  4 ++++
 docs/reference/module-types/exec.md            |  4 ++++
 docs/reference/module-types/helm.md            |  4 ++++
 docs/reference/module-types/kubernetes.md      |  4 ++++
 docs/reference/module-types/maven-container.md |  4 ++++
 docs/reference/module-types/openfaas.md        |  4 ++++
 docs/reference/module-types/terraform.md       |  4 ++++
 garden-service/package.json                    |  4 ++--
 garden-service/src/config/module.ts            |  7 ++++++-
 garden-service/src/config/project.ts           | 17 ++++++++++++++---
 garden-service/src/watch.ts                    | 15 +++++++++------
 garden-service/test/mocha.opts                 |  2 +-
 14 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/docs/guides/configuration-files.md b/docs/guides/configuration-files.md
index baae4c7397..3c05594bb4 100644
--- a/docs/guides/configuration-files.md
+++ b/docs/guides/configuration-files.md
@@ -353,7 +353,7 @@ Here we only scan the `modules` directory, but exclude the `modules/tmp` directo
 
 If you specify a list with `include`, only those patterns are included. If you then specify one or more `exclude` patterns, those are filtered out of the ones matched by `include`. If you _only_ specify `exclude`, those patterns will be filtered out of all paths in the project directory.
 
-The `modules.exclude` field is also used to limit the number of files and directories Garden watches for changes while running. Use that if you have a large number of files/directories in your project that you do not need to watch, or if you are seeing excessive CPU/RAM usage.
+The `modules.exclude` field is also used to limit the number of files and directories Garden watches for changes while running. Use that if you have a large number of files/directories in your project that you do not need to watch, or if you are seeing excessive CPU/RAM usage. The `modules.include` field has no effect on which paths Garden watches for changes.
 
 #### Module include/exclude
 
@@ -377,7 +377,7 @@ Here we only include the `Dockerfile` and all the `.py` files under `my-sources/
 
 If you specify a list with `include`, only those files/patterns are included. If you then specify one or more `exclude` files or patterns, those are filtered out of the files matched by `include`. If you _only_ specify `exclude`, those patterns will be filtered out of all files in the module directory.
 
-The `exclude` field on modules is also used to limit the number of files and directories Garden watches for changes while running. Use that if you have a large number of files/directories in your module that you do not need to watch, or if you are seeing excessive CPU/RAM usage.
+Note that the module `include` and `exclude` fields have no effect on which paths Garden watches for changes. Use the project `modules.exclude` field (described above) for that purpose.
 
 #### .ignore files
 
diff --git a/docs/reference/config.md b/docs/reference/config.md
index 3168b4a92a..5c2fa15d1a 100644
--- a/docs/reference/config.md
+++ b/docs/reference/config.md
@@ -178,6 +178,10 @@ Note that you can also _exclude_ path using the `exclude` field or by placing `.
 source tree, which use the same format as `.gitignore` files. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories) for details.
 
+Unlike the `exclude` field, the paths/globs specified here have _no effect_ on which files and directories
+Garden watches for changes. Use the `exclude` field to affect those, if you have large directories that
+should not be watched for changes.
+
 Also note that specifying an empty list here means _no paths_ should be included.
 
 | Type            | Required |
@@ -198,6 +202,7 @@ modules:
 [modules](#modules) > exclude
 
 Specify a list of POSIX-style paths or glob patterns that should be excluded when scanning for modules.
+The filters here also affect which files and directories are watched for changes. So if you have a large number of directories in your project that should not be watched, you should specify them here. The `include` field does _not_ affect which files are watched.
 Note that you can also explicitly _include_ files using the `include` field. If you also specify the `include` field, the paths/patterns specified here are filtered from the files matched by `include`. See the [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories) for details.
 
 | Type            | Required |
@@ -527,6 +532,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/container.md b/docs/reference/module-types/container.md
index dcf7f6d98a..4b80b34917 100644
--- a/docs/reference/module-types/container.md
+++ b/docs/reference/module-types/container.md
@@ -103,6 +103,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/exec.md b/docs/reference/module-types/exec.md
index 753740fd1c..09a1e43271 100644
--- a/docs/reference/module-types/exec.md
+++ b/docs/reference/module-types/exec.md
@@ -106,6 +106,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/helm.md b/docs/reference/module-types/helm.md
index 64ccebde1b..3d86499be8 100644
--- a/docs/reference/module-types/helm.md
+++ b/docs/reference/module-types/helm.md
@@ -98,6 +98,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/kubernetes.md b/docs/reference/module-types/kubernetes.md
index eb6a845a5f..b6f7518df7 100644
--- a/docs/reference/module-types/kubernetes.md
+++ b/docs/reference/module-types/kubernetes.md
@@ -106,6 +106,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/maven-container.md b/docs/reference/module-types/maven-container.md
index af11d4aa0a..466c137560 100644
--- a/docs/reference/module-types/maven-container.md
+++ b/docs/reference/module-types/maven-container.md
@@ -108,6 +108,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/openfaas.md b/docs/reference/module-types/openfaas.md
index 24527e7244..647e564218 100644
--- a/docs/reference/module-types/openfaas.md
+++ b/docs/reference/module-types/openfaas.md
@@ -98,6 +98,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/docs/reference/module-types/terraform.md b/docs/reference/module-types/terraform.md
index 90bcf2bd33..d83f78090d 100644
--- a/docs/reference/module-types/terraform.md
+++ b/docs/reference/module-types/terraform.md
@@ -112,6 +112,10 @@ Note that you can also explicitly _include_ files using the `include` field. If
 `include` field, the files/patterns specified here are filtered from the files matched by `include`. See the
 [Configuration Files guide](https://docs.garden.io/guides/configuration-files#including-excluding-files-and-directories)for details.
 
+Unlike the `modules.exclude` field in the project config, the filters here have _no effect_ on which files
+and directories are watched for changes. Use the project `modules.exclude` field to affect those, if you have
+large directories that should not be watched for changes.
+
 | Type            | Required |
 | --------------- | -------- |
 | `array[string]` | No       |
diff --git a/garden-service/package.json b/garden-service/package.json
index deb4f29526..db6790a630 100644
--- a/garden-service/package.json
+++ b/garden-service/package.json
@@ -218,7 +218,7 @@
     "e2e": "cd test/e2e && ../../bin/garden test",
     "e2e-project": "node build/test/e2e/e2e-project.js",
     "pkg": "./bin/build-pkg.sh",
-    "test": "node_modules/.bin/mocha",
+    "test": "node_modules/.bin/mocha --opts test/mocha.opts",
     "tsc": "tsc -p . --outDir build --declaration --declarationMap --sourceMap",
     "view-report": "open coverage/index.html",
     "watch": "npm run tsc -- -w"
@@ -230,4 +230,4 @@
     ]
   },
   "gitHead": "b0647221a4d2ff06952bae58000b104215aed922"
-}
+}
\ No newline at end of file
diff --git a/garden-service/src/config/module.ts b/garden-service/src/config/module.ts
index d6dd49cf1b..38f5bcac23 100644
--- a/garden-service/src/config/module.ts
+++ b/garden-service/src/config/module.ts
@@ -140,7 +140,12 @@ export const baseModuleSpecSchema = joi
 
         Note that you can also explicitly _include_ files using the \`include\` field. If you also specify the
         \`include\` field, the files/patterns specified here are filtered from the files matched by \`include\`. See the
-        [Configuration Files guide](${includeGuideLink})for details.`
+        [Configuration Files guide](${includeGuideLink})for details.
+
+        Unlike the \`modules.exclude\` field in the project config, the filters here have _no effect_ on which files
+        and directories are watched for changes. Use the project \`modules.exclude\` field to affect those, if you have
+        large directories that should not be watched for changes.
+        `
       )
       .example([["tmp/**/*", "*.log"], {}]),
     repositoryUrl: joiRepositoryUrl().description(
diff --git a/garden-service/src/config/project.ts b/garden-service/src/config/project.ts
index 995da91bee..e27ac94479 100644
--- a/garden-service/src/config/project.ts
+++ b/garden-service/src/config/project.ts
@@ -166,12 +166,17 @@ const projectModulesSchema = joi.object().keys({
     .array()
     .items(joi.string().posixPath({ subPathOnly: true }))
     .description(
-      dedent`Specify a list of POSIX-style paths or globs that should be scanned for Garden modules.
+      dedent`
+        Specify a list of POSIX-style paths or globs that should be scanned for Garden modules.
 
         Note that you can also _exclude_ path using the \`exclude\` field or by placing \`.gardenignore\` files in your
         source tree, which use the same format as \`.gitignore\` files. See the
         [Configuration Files guide](${includeGuideLink}) for details.
 
+        Unlike the \`exclude\` field, the paths/globs specified here have _no effect_ on which files and directories
+        Garden watches for changes. Use the \`exclude\` field to affect those, if you have large directories that
+        should not be watched for changes.
+
         Also note that specifying an empty list here means _no paths_ should be included.`
     )
     .example([["modules/**/*"], {}]),
@@ -179,11 +184,17 @@ const projectModulesSchema = joi.object().keys({
     .array()
     .items(joi.string().posixPath({ subPathOnly: true }))
     .description(
-      deline`Specify a list of POSIX-style paths or glob patterns that should be excluded when scanning for modules.
+      deline`
+        Specify a list of POSIX-style paths or glob patterns that should be excluded when scanning for modules.
+
+        The filters here also affect which files and directories are watched for changes. So if you have a large number
+        of directories in your project that should not be watched, you should specify them here. The \`include\` field
+        does _not_ affect which files are watched.
 
         Note that you can also explicitly _include_ files using the \`include\` field. If you also specify the
         \`include\` field, the paths/patterns specified here are filtered from the files matched by \`include\`. See the
-        [Configuration Files guide](${includeGuideLink}) for details.`
+        [Configuration Files guide](${includeGuideLink}) for details.
+      `
     )
     .example([["public/**/*", "tmp/**/*"], {}]),
 })
diff --git a/garden-service/src/watch.ts b/garden-service/src/watch.ts
index 9a0e0f4d37..34287dab77 100644
--- a/garden-service/src/watch.ts
+++ b/garden-service/src/watch.ts
@@ -13,13 +13,13 @@ import { Module } from "./types/module"
 import { Garden } from "./garden"
 import { LogEntry } from "./logger/log-entry"
 import { sleep } from "./util/util"
-import { some, flatten } from "lodash"
+import { some } from "lodash"
 import { isConfigFilename, matchPath } from "./util/fs"
 import Bluebird from "bluebird"
 import { InternalError } from "./exceptions"
 
 // How long we wait between processing added files and directories
-const DEFAULT_BUFFER_INTERVAL = 500
+const DEFAULT_BUFFER_INTERVAL = 400
 
 export type ChangeHandler = (module: Module | null, configChanged: boolean) => Promise<void>
 
@@ -60,6 +60,7 @@ export class Watcher {
     if (this.watcher) {
       this.log.debug(`Watcher: Clearing handlers`)
       this.watcher.removeAllListeners()
+      this.watcher.close()
       delete this.watcher
     }
   }
@@ -87,17 +88,19 @@ export class Watcher {
       // See https://github.com/garden-io/garden/issues/1269.
       // TODO: see if we can extract paths from dotignore files as well (we'd have to deal with negations etc. somehow).
       const projectExcludes = this.garden.moduleExcludePatterns.map((p) => resolve(this.garden.projectRoot, p))
-      const moduleExcludes = flatten(this.modules.map((m) => (m.exclude || []).map((p) => resolve(m.path, p))))
+      // TODO: filter paths based on module excludes as well
+      //       (requires more complex logic to handle overlapping module sources).
+      // const moduleExcludes = flatten(this.modules.map((m) => (m.exclude || []).map((p) => resolve(m.path, p))))
 
       this.watcher = watch(this.paths, {
         ignoreInitial: true,
         ignorePermissionErrors: true,
         persistent: true,
         awaitWriteFinish: {
-          stabilityThreshold: 500,
-          pollInterval: 200,
+          stabilityThreshold: 300,
+          pollInterval: 100,
         },
-        ignored: [...projectExcludes, ...moduleExcludes],
+        ignored: [...projectExcludes],
       })
 
       this.watcher
diff --git a/garden-service/test/mocha.opts b/garden-service/test/mocha.opts
index 9069a537de..c1b6b9fc54 100644
--- a/garden-service/test/mocha.opts
+++ b/garden-service/test/mocha.opts
@@ -5,7 +5,7 @@
 --exclude test/,src/,.garden
 --watch-extensions js,json,pegjs
 --reporter spec
---timeout 20000
+--timeout 3000
 --exit
 build/test/setup.js
 build/test/unit/**/*.js