Remote sources fix watch #1071
Remote sources fix watch #1071
Conversation
eysi09
force-pushed
the
remote-sources-fix-watch
branch
3 times, most recently
from
398c0c5 to
a9d924a
Compare
garden-service/src/util/fs.ts
Outdated
| @@ -21,7 +21,8 @@ import { VcsHandler } from "../vcs/vcs" | |||
| const VALID_CONFIG_FILENAMES = ["garden.yml", "garden.yaml"] | |||
| const metadataFilename = "metadata.json" | |||
| export const defaultDotIgnoreFiles = [".gitignore", ".gardenignore"] | |||
| export const fixedExcludes = [".git", ".garden", "debug-info-*"] | |||
| // FIXME: We should use `garden.gardenDirPath` instead of ".garden" since the dir name can be variable | |||
There was a problem hiding this comment.
I don't quite understand this comment
There was a problem hiding this comment.
We stopped using .garden as a global constant and removed all references to it at some point. Instead the path is a property on the Garden class that we set when we initialise it. So you could technically do new Garden({ gardenDirPath: ".foo" }). We use this for the system Garden class (although it's nested within .garden).
There was a problem hiding this comment.
I see. I'd then rather explicitly add that path to the excludes list in Garden.factory() or the constructor, and keep the fixedExcludes constant as-is.
garden-service/src/watch.ts
Outdated
| // The process hangs after tests if we don't do this | ||
| registerCleanupFunction("stop watcher", () => { | ||
| // Export so that we can clean up the global watcher instance when running tests | ||
| export function cleanUp() { |
There was a problem hiding this comment.
I'd suggest a more descriptive name in that case
|
|
||
| if (watcher === undefined) { | ||
| watcher = watch(projectRoot, { | ||
| watcher = watch(paths, { |
There was a problem hiding this comment.
This basically means that the watcher will not work right if you add a new source while watching. That was okay(ish) when it was just the project root, but may be problematic now. I think it's fairly simple to solve though. We just need to keep track of which paths are currently being watched (or read that from the existing watcher if it exists) and add/remove as necessary.
There was a problem hiding this comment.
Yeah, you can just get a list of currently watched paths (if any), and reconcile that against the paths we want to watch: https://github.com/paulmillr/chokidar#methods--events
There was a problem hiding this comment.
Do you mean when you run the link / unlink commands while Garden is already running in watch mode?
There was a problem hiding this comment.
No, more thinking of when you add a source in your project config while watching for changes.
There was a problem hiding this comment.
Ah, didn't think of that. However, you'd still need to link it (even if it's local), which is a separate command. So the assumption is still that the user would kill the running process, run garden link and then start it again.
There was a problem hiding this comment.
Linking basically updates the .garden/local-config.yml file and I'm not sure if a running process will handle changes to that.
There was a problem hiding this comment.
We could also treat that as a config change and watch that file, I suppose
There was a problem hiding this comment.
Yeah I guess. Wondering if there are any unwanted side effects since Garden updates this file programmatically. And not just for setting/removing linked sources. But I guess in any case you'd want to trigger a config change event.
There was a problem hiding this comment.
We could also special-case the particular variables in the config. But I'd suggest creating a separate issue for that.
|
Here's the issue for the watcher paths thing: #1082 |
eysi09
force-pushed
the
remote-sources-fix-watch
branch
from
24c718d to
8f918c4
Compare
|
Just realised I didn't push my commit for this:
It's there now and I'll squash if you approve. |
eysi09
force-pushed
the
remote-sources-fix-watch
branch
from
716671a to
a186f60
Compare
What this PR does / why we need it:
First of all, it fixes an issue where Garden wasn't watching linked sources (when you do
garden link my-module /to/some/path). We fix this by simply including all linked paths when initialising theWatcherclass.Second, it refactors the
test-project-ext-module-sourcesandtest-project-ext-project-sourcestest projects. Before, they would containmock-dot-gardenandmock-local-pathdirectories. We then did a lot of stubbing to make Garden think these were the.gardendir and/some/local/project/dir, respectively.Instead of all this, we now initialise these projects with a proper
.gardendirectory that contains the sources and git init them, just like Garden would expect in the "real world". We also remove themock-local-pathdirs and instead usetest-project-local-*-sourcesprojects when testing the linking functionality.Which issue(s) this PR fixes:
Fixes #965